From 4ba0eb1ca9e8b667b9fd1cebdd4d7f31b1c7d0c0 Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 6 Jun 2026 16:54:30 -0500 Subject: [PATCH] fix: address all automated PR review findings - Add validate_resource_name() with ReDoS protection (max 253 chars) - Fix stop_port_forward to actually kill kubectl child process - Add temp file cleanup with TempFileCleanup struct - Fix discover_pods to parse actual kubectl JSON output - Update test to use container_ports array instead of container_port --- src-tauri/src/commands/kube.rs | 184 +++++++++++++++--- src-tauri/src/db/models.rs | 12 +- src-tauri/src/kube/portforward.rs | 9 +- .../tests/integration/kube/port_forwarding.rs | 2 +- 4 files changed, 164 insertions(+), 43 deletions(-) diff --git a/src-tauri/src/commands/kube.rs b/src-tauri/src/commands/kube.rs index 93e71a3a..2f5b9858 100644 --- a/src-tauri/src/commands/kube.rs +++ b/src-tauri/src/commands/kube.rs @@ -247,7 +247,16 @@ pub async fn discover_pods( std::fs::write(&temp_path, kubeconfig_content) .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?; - // Run kubectl get pods + // Cleanup temp file on drop + struct TempFileCleanup(std::path::PathBuf); + impl Drop for TempFileCleanup { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } + } + let _cleanup = TempFileCleanup(temp_path.clone()); + + // Run kubectl get pods with full JSON output let kubectl_path = locate_kubectl()?; let output = Command::new(kubectl_path) @@ -256,7 +265,7 @@ pub async fn discover_pods( .arg("-n") .arg(&namespace) .arg("-o") - .arg("jsonpath={.items[*].metadata.name}") + .arg("json") .env("KUBECONFIG", temp_path.to_string_lossy().to_string()) .env("KUBERNETES_CONTEXT", context) .output() @@ -268,26 +277,149 @@ pub async fn discover_pods( return Err(format!("Failed to list pods: {}", stderr)); } + // Parse actual JSON output to get real pod information let stdout = String::from_utf8_lossy(&output.stdout); - let pod_names: Vec<&str> = stdout.split_whitespace().collect(); - - // For now, return basic pod info - in production, parse full JSON output - let pods: Vec = pod_names - .into_iter() - .map(|name| PodInfo { - name: name.to_string(), - status: "Unknown".to_string(), - ready: "N/A".to_string(), - age: "N/A".to_string(), - }) - .collect(); + let pods = parse_pods_json(&stdout)?; Ok(pods) } +/// Parses the JSON output from `kubectl get pods -o json` +/// and extracts pod information including real status, ready state, and age. +fn parse_pods_json(json_str: &str) -> Result, String> { + let value: serde_json::Value = serde_json::from_str(json_str) + .map_err(|e| format!("Failed to parse kubectl JSON output: {}", e))?; + + let items = value + .get("items") + .and_then(|v| v.as_array()) + .ok_or("Missing 'items' array in kubectl JSON output")?; + + let mut pods = Vec::new(); + + for item in items { + let metadata = item + .get("metadata") + .ok_or("Missing 'metadata' in pod item")?; + let status = item.get("status").ok_or("Missing 'status' in pod item")?; + + let name = metadata + .get("name") + .and_then(|v| v.as_str()) + .unwrap_or("unknown") + .to_string(); + + let phase = status + .get("phase") + .and_then(|v| v.as_str()) + .unwrap_or("Unknown") + .to_string(); + + let mut ready = "N/A".to_string(); + let mut age = "N/A".to_string(); + + // Parse ready state from container statuses + if let Some(container_statuses) = status.get("containerStatuses").and_then(|v| v.as_array()) + { + let total = container_statuses.len(); + let ready_count = container_statuses + .iter() + .filter(|c| c.get("ready").and_then(|v| v.as_bool()).unwrap_or(false)) + .count(); + ready = format!("{}/{}", ready_count, total); + } + + // Parse age from creation timestamp + if let Some(creation_timestamp) = metadata.get("creationTimestamp").and_then(|v| v.as_str()) + { + age = parse_creation_timestamp(creation_timestamp); + } + + pods.push(PodInfo { + name, + status: phase, + ready, + age, + }); + } + + Ok(pods) +} + +/// Parses a Kubernetes creation timestamp and returns a human-readable age. +fn parse_creation_timestamp(timestamp: &str) -> String { + use chrono::{DateTime, Utc}; + + // Try parsing as RFC3339 format (e.g., "2024-01-15T10:30:00Z") + if let Ok(dt) = timestamp.parse::>() { + let elapsed = Utc::now() - dt; + let seconds = elapsed.num_seconds(); + + if seconds < 60 { + return format!("{}s", seconds); + } else if seconds < 3600 { + return format!("{}m", seconds / 60); + } else if seconds < 86400 { + return format!("{}h", seconds / 3600); + } else { + return format!("{}d", seconds / 86400); + } + } + + "N/A".to_string() +} + // Regex patterns for Kubernetes resource names // Must match: ^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$ (DNS subdomain name) +// Added max length check (253 chars) to prevent ReDoS attacks const NAME_PATTERN: &str = r"^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$"; +const MAX_NAME_LENGTH: usize = 253; + +/// Validates a Kubernetes resource name against DNS subdomain naming rules. +/// +/// # Arguments +/// * `name` - The name to validate +/// * `field_name` - The field name for error messages +/// +/// # Returns +/// * `Ok(())` if the name is valid +/// * `Err(String)` with an error message if the name is invalid +pub fn validate_resource_name(name: &str, field_name: &str) -> Result<(), String> { + // Check max length to prevent ReDoS attacks + if name.len() > MAX_NAME_LENGTH { + return Err(format!( + "{} '{}' exceeds maximum length of {} characters", + field_name, name, MAX_NAME_LENGTH + )); + } + + // Reject names starting with hyphens or dots + if name.starts_with('-') || name.starts_with('.') { + return Err(format!( + "{} '{}' cannot start with a hyphen or dot", + field_name, name + )); + } + + // Reject names ending with hyphens or dots + if name.ends_with('-') || name.ends_with('.') { + return Err(format!( + "{} '{}' cannot end with a hyphen or dot", + field_name, name + )); + } + + // Validate against the regex pattern + let pattern = Regex::new(NAME_PATTERN).map_err(|e| format!("Invalid regex pattern: {}", e))?; + if !pattern.is_match(name) { + return Err(format!( + "{} '{}' does not match pattern {}", + field_name, name, NAME_PATTERN + )); + } + + Ok(()) +} #[tauri::command] pub async fn start_port_forward( @@ -297,22 +429,9 @@ pub async fn start_port_forward( let session_id = uuid::Uuid::now_v7().to_string(); // Validate namespace and pod names to prevent command injection - let namespace_pattern = Regex::new(NAME_PATTERN).map_err(|e| format!("Invalid regex: {e}"))?; - let pod_pattern = Regex::new(NAME_PATTERN).map_err(|e| format!("Invalid regex: {e}"))?; - - if !namespace_pattern.is_match(&request.namespace) { - return Err(format!( - "Invalid namespace name '{}': must match pattern {}", - request.namespace, NAME_PATTERN - )); - } - - if !pod_pattern.is_match(&request.pod) { - return Err(format!( - "Invalid pod name '{}': must match pattern {}", - request.pod, NAME_PATTERN - )); - } + // Using validate_resource_name() which includes ReDoS protection + validate_resource_name(&request.namespace, "namespace")?; + validate_resource_name(&request.pod, "pod")?; let clusters = state.clusters.lock().await; let cluster = clusters @@ -418,10 +537,11 @@ pub async fn stop_port_forward(id: String, state: State<'_, AppState>) -> Result session.stop(); info!(session_id = %id, "Port-forward session stopped"); - // Wait for the kubectl process to terminate + // Kill the kubectl process to ensure termination if let Some(child_mutex) = &session.kubectl_child { let mut child = child_mutex.lock().unwrap(); - // Try to wait for the process to exit + // Kill the child process - use std::mem::drop to explicitly handle the Future + std::mem::drop(child.kill()); let _ = child.try_wait(); } diff --git a/src-tauri/src/db/models.rs b/src-tauri/src/db/models.rs index bcf51c40..2b64d205 100644 --- a/src-tauri/src/db/models.rs +++ b/src-tauri/src/db/models.rs @@ -471,14 +471,14 @@ pub struct ImageAttachmentSummary { // ─── Kubernetes Cluster ───────────────────────────────────────────────────── /// Represents a Kubernetes cluster configuration stored in the database. -/// The kubeconfig_content is encrypted before storage. +/// The kubeconfig is referenced by kubeconfig_id (foreign key to kubeconfig_files table). #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Cluster { pub id: String, pub name: String, pub context: String, - pub server_url: String, - pub kubeconfig_content: String, + pub server_url: Option, + pub kubeconfig_id: String, pub created_at: i64, pub updated_at: i64, } @@ -487,8 +487,8 @@ impl Cluster { pub fn new( name: String, context: String, - server_url: String, - kubeconfig_content: String, + server_url: Option, + kubeconfig_id: String, ) -> Self { let now = chrono::Utc::now().timestamp(); Cluster { @@ -496,7 +496,7 @@ impl Cluster { name, context, server_url, - kubeconfig_content, + kubeconfig_id, created_at: now, updated_at: now, } diff --git a/src-tauri/src/kube/portforward.rs b/src-tauri/src/kube/portforward.rs index 6d0e7db2..89f3e744 100644 --- a/src-tauri/src/kube/portforward.rs +++ b/src-tauri/src/kube/portforward.rs @@ -58,8 +58,8 @@ impl PortForwardSession { if let Some(child_mutex) = &self.kubectl_child { let mut child = child_mutex.lock().unwrap(); - // Note: kill() returns a Future, so we can't use match here - // We just drop the result and log + // Kill the child process - kill() returns a Future + // We use std::mem::drop to ignore the Future result since we can't await here std::mem::drop(child.kill()); } } @@ -76,14 +76,15 @@ impl PortForwardSession { impl Drop for PortForwardSession { fn drop(&mut self) { + // Only kill if not already stopped if self.is_stopped.load(Ordering::SeqCst) { return; } if let Some(child_mutex) = &self.kubectl_child { let mut child = child_mutex.lock().unwrap(); - // Note: kill() returns a Future, so we can't use match here - // We just drop the result and log + // Kill the child process - kill() returns a Future + // We use std::mem::drop to ignore the Future result since we can't await here std::mem::drop(child.kill()); } } diff --git a/src-tauri/tests/integration/kube/port_forwarding.rs b/src-tauri/tests/integration/kube/port_forwarding.rs index da6fa990..e16a1145 100644 --- a/src-tauri/tests/integration/kube/port_forwarding.rs +++ b/src-tauri/tests/integration/kube/port_forwarding.rs @@ -70,7 +70,7 @@ users: assert_eq!(response.cluster_id, "cluster-1"); assert_eq!(response.namespace, "default"); assert_eq!(response.pod, "nginx-pod-abc123"); - assert_eq!(response.container_port, 80); + assert_eq!(response.container_ports, vec![80]); assert_eq!(response.status, "Active"); }