feat(kube): Implement complete kubectl port-forward runtime #72
No reviewers
Labels
No Label
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sarman/tftsr-devops_investigation#72
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "feature/kubernetes-management"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
This PR implements the complete kubectl port-forward runtime for the Kubernetes management feature in v1.1.0.
What's Implemented
Backend (Rust)
Frontend (React)
Tests
Database Migrations
Testing
All tests pass:
Next Steps
After merge:
03ee6ff9b8toc94a25f66fAutomated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR implements the Kubernetes port-forward runtime with kubectl subprocess management. A critical security issue was found: the app spawns kubectl commands with user-controlled namespace/pod arguments without validation or escaping, enabling command injection. Additionally,
start_port_forwardandstop_port_forwardhandle session lifecycle inconsistently—sessions are created but not stored in the database, and port forwards are not cleaned up if the kubectl process fails. Database migrations exist, but integration tests reveal port forwards persist after cluster removal, risking resource leaks.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:388-397 - Command injection vulnerability via unsanitized namespace/pod in kubectl arguments
Evidence:
Fix: Sanitize/validate
request.namespaceandrequest.pod(e.g., regex^[a-z0-9.-]+$or whitelist known-good pattern), and escape any shell metacharacters before passing toCommand.[WARNING] src-tauri/src/commands/kube.rs:392-417 - PortForwardSession is created but not persisted to database, causing data loss on restart
Evidence:
Fix: Immediately after session creation, insert into database table
port_forwardsand restore sessions on app startup (session_recovery.rs exists but is not wired tostart_port_forward).[WARNING] src-tauri/src/commands/kube.rs:383-386 - Local port is reserved via
TcpListenerbut no error handling for subsequent port conflict; kubectl may fail if port is reused quicklyEvidence:
Fix: Immediately bind and hold the port (e.g., convert to
TcpStreamor keeplisteneralive) until kubectl starts, or use a port allocation loop with retries.[WARNING] src-tauri/src/commands/kube.rs:451-458 -
stop_port_forwardonly sets internal state; does not ensure kubectl process is terminated and removed fromport_forwardsmap on successEvidence:
Fix: On successful
stop(), remove the session fromport_forwardsmap and wait for kubectl process to terminate (join on child if needed) before returning.[WARNING] src-tauri/src/commands/kube.rs:478-488 -
list_port_forwardsonly returns first port fromVec<u16>, discarding multi-port support dataEvidence:
Fix: Update
PortForwardResponseto useVec<u16>for ports, or explicitly document single-port limitation and remove unusedVecfields.[WARNING] src-tauri/src/kube/portforward.rs:169-173 -
Dropimplementation kills kubectl child on session drop without checkingis_stopped, leading to possible race kill on manual stopEvidence:
Fix: Flip logic to
if !self.is_stopped.load(...) { ... }before killing, or ensurestop()setsis_stoppedbefore child drop.[WARNING] src-tauri/tests/integration/kube/multi_cluster.rs:292-304 - Integration test documents that cluster removal does NOT cascade delete port forwards; this mismatch between DB foreign keys and app behavior causes resource leaks
Evidence:
Fix: Either enforce DB-level cascade (add
ON DELETE CASCADE) and remove orphaned in-memory entries, or explicitly delete associated port forwards inremove_cluster.Verdict: REQUEST CHANGES
Critical command injection and persistence issues must be addressed before merging.
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements the frontend data structures and CLI command infrastructure for port forwarding, but the critical runtime execution (spawning
kubectlsubprocess) and database persistence for port forwards are missing or incomplete. The code compiles and tests pass in memory, but in production the port forward will not function because no actualkubectlchild process is stored or managed beyond initial allocation, and sessions are not recovered or cleaned up on restart.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:343–372 -
start_port_forwardallocates a local port but drops theTcpListenerbefore spawningkubectl, then creates a session withkubectl_child: None, then only afterward tries to insert intoport_forwardsand set the child on the retrieved session. SincePortForwardSession::new()does not store the child handle and the mutable lookup is performed afternew(), the child handle is never persisted.Evidence:
let _session = crate::kube::PortForwardSession::new(PortForwardSessionConfig { ... }); ... let session_mut = port_forwards.get_mut(&session_id).unwrap(); session_mut.kubectl_child = Some(child_mutex);— the session returned bynew()is a local temp value whosekubectl_childfield is never initialized; the one stored inport_forwardsis created separately (inAppState), so this code writes to a newly created session that didn’t exist before the lookup, and the local one with the allocated port is discarded.Fix: Either store the child handle in
PortForwardSession::new()(extend config) or immediately afternew()call, capture the session from the map and setkubectl_childbefore dropping it, and ensure theport_forwardsmap entry matches the session that holds the child.[BLOCKER] src-tauri/src/kube/portforward.rs:24–55 -
PortForwardSession::stop()andDropimplementations kill the child if present, butkubectl_childis alwaysNoneat runtime because it was never set bystart_port_forward. Therefore, no actual kubectl subprocess will be terminated, leading to orphaned processes.Evidence:
kubectl_child: Option<Arc<std::sync::Mutex<tokio::process::Child>>>field, defaultNoneinnew(), and no code initializes it before storing the session inAppState.start_port_forwardsets it on the wrong local instance and never reads from the map to confirm.Fix: Initialize
kubectl_childinPortForwardSession::new()viaPortForwardSessionConfig, and ensurestart_port_forwardconstructs the session with the child handle before storing it.[WARNING] src-tauri/src/commands/kube.rs:261–265 - Port allocation is racy:
TcpListener::bind("127.0.0.1:0")reserves a port, but dropping the listener and spawningkubectllater gives no guarantee that the port is still free.Evidence:
let listener = TcpListener::bind("127.0.0.1:0")?; let local_port = listener.local_addr()?.port(); drop(listener);— between dropping andkubectlbinding, another process could grab the same port.Fix: Bind the port inside the
kubectlinvocation by passing0to kubectl or by using a socket bound in the command arguments (e.g.,kubectl port-forward ... :0), or hold the listener open untilkubectlstarts (advanced).[WARNING] src-tauri/tests/integration/kube/multi_cluster.rs:244–260 -
test_remove_cluster_cascades_to_port_forwardsdocuments that removal does not cascade, but the database schema includesON DELETE CASCADE. The in-memory integration tests use a separateAppStatemap and do not exercise the database cascade, so the behavior is unverified at the DB level.Evidence: comment:
// Note: Current implementation doesn't cascade delete port forwards... This may be intentional for debugging or may need to be fixedand the map-based test shows port forwards persist.Fix: Add a DB integration test or ensure the command handler deletes related port forwards before/after deleting the cluster, and verify with an explicit integration test.
[SUGGESTION] src-tauri/src/commands/kube.rs:268–289 - The
start_port_forwardimplementation writes kubeconfig to a temp file but never cleans it up, leaving many files in/tmpacross multiple port forwards.Evidence:
std::fs::write(&temp_path, kubeconfig_content.as_ref())?;— temp_path is never removed.Fix: Use
std::fs::File::createand pass the file handle toCommand, or register a cleanup task, or usestd::env::temp_dir()and remove on session stop.Verdict: REQUEST CHANGES
c94a25f66fto10f31114dc10f31114dctoa3f714ef2dAutomated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR claims to implement a "complete kubectl port-forward runtime", but the backend code still does not actually spawn kubectl port-forward subprocesses or properly handle runtime execution. Multiple code paths and tests reference missing kubectl execution and runtime state management. There are also inconsistencies in data models (Cluster vs ClusterClient, memory-only clusters), missing database persistence, and no cleanup of kubectl child processes in error scenarios.
Findings
start_port_forwardallocates local ports and creates a session but never callsCommand::new(kubectl_path).spawn()to run kubectl port-forward.Evidence:
let child = Command::new(kubectl_path)...spawn()is never executed; the line is only partially implemented and does not assign to a child variable used later.Fix: Execute the kubectl command and store the child in the session properly.
Evidence:
let listener = TcpListener::bind("127.0.0.1:0")...drop(listener);thenlet child = Command::new(...).spawn()is missing; local_port is unused.Fix: Execute kubectl with allocated port and store child handle in session.
ClusterClientusesArc<String>for kubeconfig content but never referenceskubeconfig_idor persists to database;Clusterin mod.rs useskubeconfig_idfield, creating model inconsistency.Evidence:
struct ClusterClient { kubeconfig_content: Arc<String>, ... }vsstruct Cluster { kubeconfig_id: String, ... }.Fix: Align models or document why two representations exist.
PortForwardSessionis created withlet _session = PortForwardSession::new(...)and immediately dropped; its child handle is never stored becausestate.port_forwards.lock()is not used to retrieve the session.Evidence:
let _session = crate::kube::PortForwardSession::new(...)then{ let mut port_forwards = state.port_forwards.lock().await; let session_mut = port_forwards.get_mut(&session_id).unwrap(); session_mut.kubectl_child = Some(child_mutex); }. But_sessionis dropped immediately after creation, soport_forwards.get_mutwill fail unless session was added elsewhere.Fix: Add session to map before spawning subprocess or store child handle directly.
Dropimplementation kills child process but ignores kill errors and does not check if process is alive; may cause zombie processes.Evidence:
std::mem::drop(child.kill())ignores all errors and does not verify child status.Fix: Log errors from kill and handle gracefully.
remove_cluster_cascades_to_port_forwardsdocuments that port forwards persist after cluster removal and explicitly states "Current implementation doesn't cascade delete port forwards", which is a data integrity risk.Evidence:
// Note: Current implementation doesn't cascade delete port forwards...// This may be intentional for debugging or may need to be fixed.Fix: Implement cascade delete on cluster removal or add foreign key constraints.
test_cluster_connectionwrites kubeconfig to temp file but does not clean it up, leading to potential temp file accumulation.Evidence:
let temp_path = temp_dir.join(format!("kubeconfig-{}.yaml", cluster_id)); std::fs::write(&temp_path, kubeconfig_content)...with noremove_fileorDrophandling.Fix: Delete temp file after use or use
tempfile::tempdir.extract_contextandextract_server_urluseserde_yaml::from_strwithout validation, but only take first item in array; malformed but syntactically valid YAML may silently use wrong values.Evidence:
contexts[0],clusters[0]used without checking array length or key presence.Fix: Return errors if arrays are empty or keys missing instead of panicking on index access.
Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements a runtime for kubectl port forwarding in the Kubernetes management feature. It adds database migrations, updates data models, implements cluster and port forward session structs, and provides IPC commands. While the foundational code is in place, there is a critical logic error in port-forward session creation that leads to incorrect storage and retrieval of child processes, along with a data integrity issue in the
clusterstable model definition.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:327 - Session creation happens after
drop(listener), but the code tries to mutate the session after creation by assigningkubectl_child, which is stored inport_forwardsmap. However, thePortForwardSession::newcall creates a local copy that is not inserted intostate.port_forwards. The subsequentget_mutwill panic at runtime.Evidence:
Fix: Insert the session into
state.port_forwardsbefore trying to mutate it, or build and insert a single session object with the child handle assigned from the start.[BLOCKER] src-tauri/src/db/models.rs:463 - The
Clustermodel includeskubeconfig_content: String, but the database tableclusters(migration 029) includeskubeconfig_id(FK tokubeconfig_files) and does not store content inline. This mismatch means any DB-backed cluster operations will fail.Evidence:
Fix: Align the model to match the schema: remove
kubeconfig_contentand addkubeconfig_id: String.[WARNING] src-tauri/src/commands/kube.rs:320-321 - The
local_portallocation usesTcpListener::bind("127.0.0.1:0"), butdrop(listener)releases the port, allowing the OS to reuse it beforekubectlspawns. This creates a race condition wherekubectlmay fail to bind.Evidence:
Fix: Reserve the port by keeping the listener alive for the duration of
kubectlspawn (e.g., passlistenerto the session and store it alongside the child), or use a safer port reservation mechanism.[WARNING] src-tauri/src/kube/portforward.rs:86-89 - The
Dropimplementation kills the child process unconditionally, ignoring whether the session was intentionally stopped. This may cause premature termination if the child exits on its own while the session is still active.Evidence:
Fix: Check if the child process is still running (e.g., via
try_wait()) before attempting to kill.Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR implements core Kubernetes management features, including database migrations, cluster/port-forward data models, and IPC commands. However, critical runtime execution of
kubectl port-forwardis incomplete, and several high-severity issues exist including command injection vulnerabilities, missing proper error handling, and data inconsistency risks from missing foreign key constraints in memory-only cluster management.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:381-393 - Command injection risk via unsanitized namespace/pod names in
start_port_forwardregex validation.Evidence:
const NAME_PATTERN: &str = r"^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$";used to validate namespace/pod, butRegex::new(NAME_PATTERN)is created once, not validated against actual input length limits (CVE-2021-45765-like ReDoS).Fix: Enforce maximum name length (253 chars for DNS subdomain), validate with precompiled static regex, and reject names starting/ending with hyphens/dots:
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$.[BLOCKER] src-tauri/src/commands/kube.rs:423 -
stop_port_forwarddoes not kill kubectl child process on error path.Evidence:
session.stop()called, buttry_wait()only logs and drops error; no actualkill()orwait()performed before session removal.Fix: Call
child.kill()(nottry_wait()) and await termination inDropandstop().[WARNING] src-tauri/src/commands/kube.rs:444 - Memory leak risk: kubectl subprocess not cleaned up on app shutdown.
Evidence:
Dropimplementation only kills child if session explicitly stopped; not triggered on app termination.Fix: Implement
tauri::plugin::LifecycleListenerforon_window_eventwithWindowEvent::CloseRequestedto signalStopon all sessions before exit.[BLOCKER] src-tauri/src/commands/kube.rs:170 -
remove_clustercascade deletes port_forwards in memory, butAppState.port_forwardsnot persisted, causing orphaned processes if app crashes.Evidence: Line 170-181 removes from
state.port_forwards, butPortForwardmodel supports DB persistence (migrations 029-030) yet not used inadd_cluster/remove_cluster.Fix: Persist clusters/port_forwards to database immediately; use foreign key constraints (CASCADE DELETE) in DB to ensure consistency.
[BLOCKER] src-tauri/src/db/migrations.rs:366-373 - Missing NOT NULL constraint on
kubeconfig_contentin clusters table allows NULL values despite logic assuming non-null.Evidence:
kubeconfig_id TEXT NOT NULL,exists butkubeconfig_contentcolumn not defined in DB schema; Rust modelClusteruseskubeconfig_content: String(non-optional).Fix: Add
kubeconfig_content TEXT NOT NULLto029_create_clustersmigration SQL.[BLOCKER] src-tauri/src/kube/portforward.rs:56 -
Dropimplementation kills kubectl child even if already stopped, causing double-free/panic.Evidence:
if self.is_stopped.load(Ordering::SeqCst) { return; }butkubectl_childkill called outside this check viastd::mem::drop(child.kill()).Fix: Guard kill call:
if let Some(child_mutex) = &self.kubectl_child { if !self.is_stopped.load(Ordering::SeqCst) { /* kill */ } }.[WARNING] src-tauri/src/commands/kube.rs:244-270 -
test_cluster_connectionwrites kubeconfig to temp dir without cleanup, leading to file descriptor exhaustion.Evidence:
temp_dir.join(format!("kubeconfig-{}.yaml", cluster_id))written but never deleted.Fix: Use
std::fs::File::create(&temp_path).and_then(|f| f.set_len(0))withtempfile::tempfileor deferstd::fs::remove_file(&temp_path).[WARNING] src-tauri/src/commands/kube.rs:336-368 -
discover_podsreturns hardcoded pod statuses ("Unknown", "N/A") instead of parsing JSON output.Evidence:
pod_names.into_iter().map(|name| PodInfo { name, status: "Unknown".to_string(), ... })ignores actualkubectl get pods -o jsonoutput.Fix: Use
kubectl get pods -o jsonpath='{.items[*].{name:metadata.name,status:status.phase,ready:status.containerStatuses[0].ready,age:metadata.creationTimestamp}}'and parse JSON.[BLOCKER] src-tauri/src/kube/client.rs:35 -
ClusterClientusesArc<String>for kubeconfig_content but no encryption despite database model supporting encryption.Evidence:
ClusterClientnot used in IPC paths;add_clusterincommands/kube.rsstores cluster in memory only; DB modelClusterhaskubeconfig_content: Stringbut not used.Fix: Remove
ClusterClientin-memory usage, migrate to database-backed cluster management exclusively.[WARNING] src-tauri/tests/integration/kube/port_forwarding.rs:64 - Test uses
container_port(singular) but API expectscontainer_ports: Vec<u16>(array).Evidence:
assert_eq!(response.container_port, 80);butPortForwardResponsedefinescontainer_ports: Vec<u16>.Fix: Update test to
assert_eq!(response.container_ports.as_slice(), &[80]);.Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements the kubectl port-forward runtime by spawning a subprocess via
Command::new(kubectl_path).spawn(). However, it fails to wait for or capture the subprocess’s exit status or stderr output, and it usesstd::mem::drop(child.kill())which ignores the asynchronousKillfuture result and does not wait for process termination. This leads to unhandled errors, orphaned processes, and silent failures.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:532-540 -
child.kill()called insideDrop::dropandstop()discards the asyncKillfuture usingstd::mem::drop, preventing any wait or error handling.Evidence:
Fix: Use
tokio::task::spawn_blockingor replaceDrop/stop()with async cleanup that awaits the child termination (e.g.,child.wait().awaitorchild.kill().awaitthenwait().await). In the current async command context, spawn cleanup in a detached task or use a synchronous wrapper.[BLOCKER] src-tauri/src/commands/kube.rs:500 -
Childis stored wrapped inArc<Mutex<Child>>, butChildis notSend/Sync, causing undefined behavior when shared across threads.Evidence:
Fix: Use
tokio::sync::Mutex<Child>and ensureChildis only accessed on the tokio runtime thread. Alternatively, wrap withJoinHandle<()>after spawning cleanup task and holdJoinHandleinstead.[BLOCKER] src-tauri/src/commands/kube.rs:530-531, 548 - No error propagation or capture from
kubectlsubprocess; stderr/stdout are ignored and errors (e.g., pod not found, port conflict, permission denied) will be silent.Evidence:
The
output()call captures output but only happens indiscover_podsandtest_cluster_connection, not instart_port_forward. For port-forward, the child is spawned withoutoutput()orstatus()awaited, so kubectl errors never surface.Fix: For port-forward, spawn the process, then in a background task, wait for status and update session state (e.g., set error status on non-zero exit). Also capture stderr for the
PortForwardResponse.error_message.[WARNING] src-tauri/src/commands/kube.rs:303-304 -
validate_resource_nameusesRegex::newwithout caching, causing repeated JIT compilation for each port-forward request.Evidence:
Fix: Precompile
NAME_PATTERNas alazy_static!oronce_cell::sync::Lazy<Regex>.[WARNING] src-tauri/src/commands/kube.rs:524-534 - Port-forward uses temp kubeconfig file but does not ensure cleanup on exit in all paths (e.g., panic/early-return). While the
discover_podsfunction wraps temp path in aTempFileCleanup,start_port_forwarddoes not.Evidence:
Fix: Use
std::fs::read_dir(temp_dir).filter(...).for_each(|e| std::fs::remove_file(e.path()).ok());on startup or register a cleanup hook withraiipattern.[WARNING] src-tauri/tests/integration/kube/cluster_management.rs:278-290, port_forwarding.rs:180-192 - Tests
start_port_forwarddo not assert that the subprocess exists or was started; they only assert successful IPC response. The implementation has no way to verify runtime behavior.Evidence:
Fix: Add a mock
Commandframework (e.g.,whichmock,mock_commandcrate) or stublocate_kubectlto return a dummy script that creates a marker file to verify invocation.[WARNING] src-tauri/src/kube/portforward.rs:23, 87, 103 -
std::sync::Mutex<Child>used with tokio asyncChildis unsafe;Child.kill()is async and must be awaited.Evidence:
Fix: Replace
std::sync::Mutexwithtokio::sync::Mutex, store aJoinHandle<Option<ExitStatus>>that runschild.kill().await; child.wait().await, and only drop that handle inDrop.Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR implements a kubectl port-forward runtime with session management, validation, and database persistence for Kubernetes clusters. However, a critical issue exists where port forward sessions store child processes as
Arc<Mutex<Child>>, but theDropimplementation forPortForwardSessioncallschild.kill()in a non-async context, causing a panic due to unawaited futures.Findings
[BLOCKER] src-tauri/src/kube/portforward.rs:54 - Calling
child.kill()without awaiting insideDropcauses panicEvidence:
std::mem::drop(child.kill());(line 54 in portforward.rs and line 56 in same file, line 96)Fix: Use
tokio::task::spawn_blockingor ensure theChild.kill()future is awaited in an async context; currently,kill()returns aFuturethat is dropped immediately without resolution, which is unsafe.[BLOCKER] src-tauri/src/commands/kube.rs:437 - kubectl subprocess spawned but child handle stored before process actually starts
Evidence:
let child = Command::new(kubectl_path).args(&args)...spawn()...?; let child_mutex = Arc::new(std::sync::Mutex::new(child));(line 434-437), butPortForwardSession::newis called beforekubectl_childis assigned (line 456 vs 449).Fix: Ensure
PortForwardSessionis only inserted afterkubectl_childis set, or use a two-phase initialization.[BLOCKER] src-tauri/src/commands/kube.rs:452 -
validate_resource_namecall after getting cluster reference risks leaking cluster references if validation failsEvidence:
validate_resource_name(&request.namespace, "namespace")?; validate_resource_name(&request.pod, "pod")?;before lock guard is used, butclusters.lock().awaitis not held during validation (lines 441-443 vs 444).Fix: Validate before acquiring cluster lock to reduce hold time and avoid partial state exposure.
[WARNING] src-tauri/src/kube/portforward.rs:96 - Duplicate
kill()logic: bothstop()andDropkill the child processEvidence:
std::mem::drop(child.kill());appears in bothstop()(line 56) andDrop(line 96) implementations.Fix: Only kill in
Drop; ensurestop()only updates status and setsis_stoppedflag.[WARNING] src-tauri/src/commands/kube.rs:290 - Temp file for kubeconfig not cleaned up on error
Evidence:
std::fs::write(&temp_path, ...)?;at line 283, butstd::fs::remove_file(&temp_path)is only done insideDropofTempFileCleanup, which may not run if early return occurs.Fix: Wrap
temp_pathinTempFileCleanupbefore writing, or usedefer!macro to guarantee cleanup.[WARNING] src-tauri/src/commands/kube.rs:317-321 -
validate_resource_nameuses regex withMAX_NAME_LENGTH, but regex itself can be inefficient for long inputsEvidence:
Regex::new(NAME_PATTERN).map_err(...)?;called on every validation (lines 317-321).Fix: Compile regex once as
lazy_static!orconst Regexto avoid repeated compilation.Verdict: REQUEST CHANGES
Critical race and panic risks in process spawning and lifetime management must be resolved before merge.
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements a complete kubectl port-forward runtime. A critical data loss and resource leak issue exists: temp kubeconfig files created during port-forward operations are not reliably cleaned up if an error occurs before the
TempFileCleanupis registered, and sessions not stopped explicitly will leak child processes. Additionally, port forward sessions are stored only in-memory, risking data loss on app restart.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:360–375 - Temp kubeconfig file cleanup has a race condition: if an error occurs between
writeandTempFileCleanupconstruction, the temp file will leak.Evidence:
Fix: Use
std::fs::writeandstd::fs::remove_filein a singlestd::panic::catch_unwindblock, or usestd::fs::File::createwithDropcleanup in the same expression scope, or at minimum, movetemp_pathinto the cleanup struct after writing and ensuredropis not elided.[BLOCKER] src-tauri/src/kube/portforward.rs:198–202 - PortForwardSession Drop impl only attempts cleanup in sync context but cannot kill child processes reliably. Child processes will remain running after app exit or session drop.
Evidence:
Fix: Add explicit
stop_async().awaitcall before drop (not possible in Drop), or implement an explicitclose()method that must be called before dropping, or ensurestop_async()is always invoked before the session is dropped by the caller.[BLOCKER] src-tauri/src/commands/kube.rs:59–71 - Clusters are stored in memory only (
state.clusters) and not persisted to the database. Port forwards are also stored in memory only (state.port_forwards). This causes complete data loss on app restart.Evidence:
Fix: Implement database-backed persistence for
clustersandport_forwardstables using the new029_create_clustersand030_create_port_forwardsmigrations and models indb/models.rs.[WARNING] src-tauri/src/commands/kube.rs:173–185 -
test_cluster_connectioncreates a temp kubeconfig file but does not validate the kubeconfig content or context before using it; invalid content will cause a misleading kubectl error rather than a clear validation error.Evidence:
Fix: Add a validation step (e.g., parse and check required keys) before writing the temp file and running kubectl.
[WARNING] src-tauri/src/commands/kube.rs:488–508 -
discover_podsreturnsPodInfoparsed from kubectl JSON, but ifcontainerStatusesorcreationTimestampare missing/invalid, default values like "N/A" are returned without logging or error, reducing diagnostic value.Evidence:
Fix: Consider logging warnings when parsing fails or returning an explicit error.
[SUGGESTION] src-tauri/src/commands/kube.rs:348–356 -
start_port_forwardalways starts a new temp kubeconfig file per call instead of reusing the stored cluster kubeconfig (which might be a large string). This increases memory and disk I/O.Evidence:
Fix: Consider creating a single temp file per cluster and caching it, or writing the kubeconfig once and reusing it across operations within the same session, with cleanup on session close.
Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR implements a complete kubectl port-forward runtime for Kubernetes management. While the architecture and frontend UI are well-structured, there is a critical security vulnerability in the shell command classifier that allows destructive Windows commands to be misclassified as safe (Tier 1), potentially allowing malicious commands to bypass safety checks.
Findings
[BLOCKER] src-tauri/src/shell/classifier.rs:1047 - Windows
stop-process,stop-service,stop-computer, and other mutating commands are incorrectly classified as Tier 1 (safe/read-only) because they match thetier1_generallist, but they are actually mutating operations that can terminate critical system processes or services.Evidence:
let tier1_general = [ ... "stop-process", "stop-service", "stop-computer", ... ]— these commands modify system state and must be Tier 2 at minimum.Fix: Remove
"stop-process","stop-service","stop-computer","start-process","start-service","start-computer","restart-computer","suspend-process","suspend-service","resume-process","resume-service","wait-process","wait-service","wait-computer","invoke-item"fromtier1_generaland place them intier2_general.[BLOCKER] src-tauri/src/shell/classifier.rs:1047 - Windows PowerShell
get-prefixed commands likeget-wmiobject,get-process,get-service, etc., are classified as Tier 1, butget-wmiobjectand similar cmdlets can be used to invoke arbitrary WMI methods that perform destructive actions (e.g.,get-wmiobject win32_process -filter "name='cmd.exe'" | invoke-wmimethod -name create -argumentlist "calc.exe").Evidence:
let tier1_general = [ ... "get-wmiobject", ... ]— these should be Tier 2 unless further restricted to pure enumeration.Fix: Remove
"get-wmiobject","get-ciminstance","invoke-wmimethod"and similar WMI cmdlets fromtier1_general. Either move them totier2_generalor add subcommand validation (e.g., only allowget-wmiobject -listorget-wmiobject -class Win32_*).[WARNING] src-tauri/src/commands/kube.rs:231 - The
validate_resource_namefunction has a potential ReDoS (Regular Expression Denial of Service) vulnerability despite the length check: the regexr"^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$"can be exploited with long strings containing repeated hyphens/dots (e.g.,"a--...--a"). While the 253-char limit helps, the regex itself is vulnerable.Evidence:
lazy_static! { static ref NAME_PATTERN_REGEX: Regex = Regex::new(r"^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$").unwrap(); }— using^([a-z0-9](-?[a-z0-9])*)?$would be safer, or useregex::RegexBuilderwithmatch_limit.Fix: Replace the regex pattern with
r"^([a-z0-9](-[a-z0-9]*)*[a-z0-9])?$"and addmatch_limit(10_000)to prevent catastrophic backtracking, or use a custom parser.[WARNING] src-tauri/src/commands/kube.rs:522-526 - Temp file cleanup via
TempFileCleanupstruct is vulnerable to panic safety issues ifstd::fs::writepanics: the struct is not yet constructed, soDropwon’t run. Ifwritefails beforelet _cleanup = TempFileCleanup(...), the temp file is leaked.Evidence:
Fix: Construct
TempFileCleanupbefore writing, or usestd::panic::catch_unwind, or replace withstd::panic::AssertUnwindSafewrapper.Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements Kubernetes port forwarding with full kubectl subprocess execution, database persistence for clusters/port_forwards, and comprehensive resource name validation. All critical functionality is in place, including runtime execution, security hardening against command injection, and proper cleanup of child processes and temporary kubeconfig files.
Findings
[BLOCKER] src-tauri/src/commands/kube.rs:322-323 - Missing validation of cluster_id causes denial-of-service and undefined behavior
Evidence:
let cluster = clusters.get(&request.cluster_id).ok_or_else(|| format!("Cluster {} not found", request.cluster_id))?;Fix: Add
validate_resource_name(&request.cluster_id, "cluster_id")?to prevent invalid cluster IDs from causing panics or unexpected behavior.[BLOCKER] src-tauri/src/commands/kube.rs:332-353 - Cluster connection test creates temp file but lacks validation and fallback handling for kubeconfig activation
Evidence:
let temp_path = temp_dir.join(format!("kubeconfig-{}.yaml", cluster_id));followed bystd::fs::write(&temp_path, kubeconfig_content)Fix: Add
validate_resource_name(&cluster_id, "cluster_id")?and wrap temp file operations instd::panic::catch_unwindor use RAII pattern to ensure cleanup.[BLOCKER] src-tauri/src/commands/kube.rs:405-412 - discover_pods uses cluster_id without prior validation and hardcodes namespace without validation
Evidence:
let namespace = &namespace;andvalidate_resource_name(&namespace, "namespace")?;appear aftercluster_idis used in path constructionFix: Reorder validation: validate cluster_id before use, validate namespace before use, and add
validate_resource_name(&namespace, "namespace")?beforelet namespace = &namespace;.[BLOCKER] src-tauri/src/commands/kube.rs:538-543 - start_port_forward does not validate cluster_id, allows empty namespace/pod after regex validation
Evidence:
validate_resource_name(&request.namespace, "namespace")?; validate_resource_name(&request.pod, "pod")?;butcluster_idis passed directly tocluster.get(&request.cluster_id)Fix: Add
validate_resource_name(&request.cluster_id, "cluster_id")?beforelet cluster = clusters.get(&request.cluster_id)[WARNING] src-tauri/src/kube/portforward.rs:181-184 - ChildWaitHandle cleanup on error may leave zombie processes
Evidence:
session.child_wait_handle = None;instop_async()without ensuring the child is killed before clearing the handleFix: Move child kill before clearing handle:
if let Some(ref child_wait_handle) = self.child_wait_handle { let guard = child_wait_handle.lock().await; let child_opt = guard.child.lock().await.take(); if let Some(mut child) = child_opt { let _ = child.kill().await; } } session.child_wait_handle = None;[BLOCKER] src-tauri/src/commands/kube.rs:405-412 - discover_pods does not validate cluster_id before using it in temp path and error messages
Evidence:
let cluster = clusters.get(&cluster_id).ok_or_else(|| format!("Cluster {} not found", cluster_id))?;appears afternamespaceandpodare validated butcluster_iditself is not validatedFix: Add
validate_resource_name(&cluster_id, "cluster_id")?beforeclusters.get(&cluster_id)[WARNING] src-tauri/src/commands/kube.rs:366-370 - test_cluster_connection uses cluster_id in temp path without validation
Evidence:
let temp_path = temp_dir.join(format!("kubeconfig-{}.yaml", cluster_id));Fix: Add
validate_resource_name(&cluster_id, "cluster_id")?before temp path constructionVerdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements a complete kubectl port-forward runtime with robust security, but contains one critical data leak vulnerability: kubeconfig content is written to a temp file using
as_ref()instead of dereferencing theArc<String>, causing the raw kubeconfig YAML (including potential credentials) to be written as an empty/invalid file. This is a high-severity security issue.Findings
[BLOCKER] src-tauri/src/commands/kube.rs:271 - Kubeconfig content is incorrectly written using
kubeconfig_content.as_ref()instead ofkubeconfig_content.as_ref().as_bytes(), resulting in writing an empty/invalid file instead of the actual kubeconfigEvidence:
Fix: Change to
kubeconfig_content.as_bytes()orkubeconfig_content.as_str().as_bytes()[BLOCKER] src-tauri/src/commands/kube.rs:386 - Same kubeconfig write vulnerability in
discover_pods()Evidence:
Fix: Change to
kubeconfig_content.as_bytes()orkubeconfig_content.as_str().as_bytes()Verdict: REQUEST CHANGES
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR implements Kubernetes port-forward runtime functionality. Two critical security vulnerabilities were found: (1) command injection vulnerability via unvalidated namespace/pod fields, and (2) race condition in kubeconfig temp file cleanup that can leak sensitive files. Additionally, there's a data integrity issue in port-forward creation where the session is stored before kubectl command execution succeeds.
Findings
[BLOCKER] src-tauri/src/commands/kube.rs:234-235 - Command injection via namespace and pod fields in kubectl command
Evidence:
validate_resource_name(&request.namespace, "namespace")?; validate_resource_name(&request.pod, "pod")?;is called AFTERlet clusters = state.clusters.lock().await;but BEFORE temp file creation; however, theport-forwardcommand construction usesformat!("pod/{}", request.pod)without validating pod name format before being used inCommand::new(kubectl_path).args(&args)Fix: Move validation to the very beginning of the function before any operations, and ensure
validate_resource_nameis called with proper inputs before any command construction. Currently, the validation is present but the error path is not properly handled to prevent partial state creation.[BLOCKER] src-tauri/src/commands/kube.rs:271-276 - Race condition in temp kubeconfig file cleanup
Evidence:
struct TempFileCleanup(std::path::PathBuf); impl Drop for TempFileCleanup { fn drop(&mut self) { let _ = std::fs::remove_file(&self.0); } }is used, butlet _cleanup = TempFileCleanup(temp_path.clone());is created BEFORE writing to the file. If the write fails, the cleanup still runs and removes the (nonexistent) file, but more critically, if the kubectl command is spawned and the child process outlives the cleanup, the temp file will be deleted while kubectl still holds a handle to it on Windows, causing undefined behavior.Fix: Ensure the temp file is only deleted after the kubectl process completes (successfully or not). The current
PortForwardSession::close()andDropimplementations do clean up, but there's a window between process spawn and session storage where failure leaves orphaned files.[WARNING] src-tauri/src/commands/kube.rs:299 - Incomplete error handling for dynamic port allocation
Evidence:
let local_port = if request.local_port > 0 { request.local_port } else { 0 };— when local_port is 0, kubectl allocates dynamically, but the actual allocated port is never retrieved from kubectl's stdout and stored in the session. This makes it impossible for the frontend to know which local port to connect to.Fix: Capture kubectl's stderr/stdout to extract the dynamically allocated port (kubectl prints something like "Forwarding from 127.0.0.1:8080 -> 8080") and update the session's local_ports accordingly.
[WARNING] src-tauri/src/kube/portforward.rs:38 - ChildWaitHandle struct defines
pub child_id: String, but this field is never used anywhere in the codebaseEvidence: In
spawn_child_waiter,child_idis set asformat!("{}-{}", self.id, self.pod), butChildWaitHandleis only accessed inclose()andstop_async()methods where it's not used.Fix: Remove the unused
child_idfield or add telemetry/logging that uses it.[SUGGESTION] src-tauri/src/commands/kube.rs:542-549 - Duplicated
TempFileCleanuppattern is repeated 4 times (add_cluster, test_cluster_connection, discover_pods, start_port_forward)Evidence: Four separate blocks with identical
TempFileCleanupstruct andDropimplementationFix: Refactor into a helper function
with_temp_kubeconfig(content, |path| ...)to reduce code duplication and improve maintainability.Verdict: REQUEST CHANGES
✅ Completed Fixes:
📝 Pending Items (Not Critical):
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements the Kubernetes kubectl port-forward runtime with substantial additions: runtime kubectl command execution, database persistence for clusters and port forwards, input validation, session management, and cleanup logic. After thorough review, a critical bug and several high-priority issues were identified.
Findings
[BLOCKER] src-tauri/src/kube/client.rs:28 - Cluster struct incorrectly defines
kubeconfig_id: Stringfield but the database migration and models usekubeconfig_id TEXT NOT NULLreferencingkubeconfig_files(id)— this causes schema mismatch when loading clusters from DBEvidence:
src-tauri/src/kube/client.rs:28declarespub kubeconfig_id: StringwithoutOption, but migration 029 andClustermodel (db/models.rs) both definekubeconfig_id TEXT NOT NULL. However, theClusterClientconstructor acceptskubeconfig_content: Arc<String>which implies kubeconfig is embedded, not referenced by ID — conflicting with database design.Fix: Decide between two approaches: (1) Keep in-memory
ClusterClientwith kubeconfig content and do not store in database, OR (2) Store clusters in database withkubeconfig_idand fetch kubeconfig content separately. Current implementation mixes both patterns causing data inconsistency.[BLOCKER] src-tauri/src/commands/kube.rs:218-224 - Port-forward session cleanup logic is broken:
spawn_child_waiterstores temp path in session and relies on background task to delete, butclose()method also callschild.kill()and drops the child handle — if session is closed before child completes (e.g., manual stop), temp file is NOT cleaned upEvidence:
src-tauri/src/kube/portforward.rs:114-126: Background task deletes temp file only after child exits normally.src-tauri/src/kube/portforward.rs:152-160:stop_asynckills child but does NOT delete temp file.src-tauri/src/kube/portforward.rs:165-177:closekills child but temp file cleanup is deferred to background task only. If user manually stops before exit, temp file remains.Fix: Ensure temp file is deleted in
stop_async,close, andDropregardless of child state, e.g., move temp file deletion to acleanup_temp_file()method called by all paths.[BLOCKER] src-tauri/src/commands/kube.rs:283-287 -
start_port_forwarddoes NOT parse kubectl's dynamic port allocation output —local_port: 0is returned to frontend, so user sees no actual port even though kubectl binds oneEvidence:
src-tauri/src/commands/kube.rs:283:local_portdefaults to0per comment: “Let kubectl allocate dynamically (currently not captured)”, andPortForwardResponsereturns that0. kubectl outputs allocated ports to stderr on startup but no parsing logic exists.Fix: Capture kubectl stderr and parse dynamic port allocation pattern (e.g.,
Forwarding from 127.0.0.1:8080) to report actual local port.[WARNING] src-tauri/src/kube/portforward.rs:160-162 -
stop_asynconly kills child and does NOT update session status in database — port forwards become stale if app restartsEvidence:
src-tauri/src/commands/kube.rs:443showsstop_port_forwardcallssession.stop_async().awaitbut no database update occurs.src-tauri/src/commands/db.rs:873-905load_port_forwards only loads DB data, so stopped sessions are lost after restart.Fix: Add
update_port_forward_status(id, status, error_message)command that writes toport_forwardstable and call it instop_port_forwardandshutdown_port_forwards.[WARNING] src-tauri/src/commands/kube.rs:489 -
remove_clusterdeletes from memory butdelete_port_forwardlogic inremove_cluster(lines 173-184) is duplicated: memory cleanup occurs inremove_clusterbut database cleanup is only indelete_port_forward, causing inconsistencyEvidence:
src-tauri/src/commands/kube.rs:173-184deletes port forwards from memory but database cleanup happens indelete_port_forward. If cluster is removed directly viaremove_cluster, DB records remain orphaned.Fix: Move database cascade delete logic into
remove_cluster, or ensure DB schema uses foreign keys withON DELETE CASCADE— currently migration 030 hasFOREIGN KEY (cluster_id) REFERENCES clusters(id) ON DELETE CASCADEbutremove_clusterdoes not trigger DB delete.[WARNING] src-tauri/src/commands/kube.rs:318-356 & 401-440 - Temp file cleanup uses
TempFileCleanupstruct withDropbut panic safety is not guaranteed if panic occurs after temp file creation but beforeTempFileCleanupis instantiatedEvidence:
src-tauri/src/commands/kube.rs:318-356writes temp file BEFORElet _cleanup = TempFileCleanup(temp_path.clone()). Iffs::writepanics, no cleanup occurs. Same issue at 401-440.Fix: Create
TempFileCleanupBEFORE writing and pass path to it, e.g.,let cleanup = TempFileCleanup(temp_path.clone()); fs::write(&cleanup.0, ...)?;.[WARNING] src-tauri/src/commands/kube.rs:457-470 -
shutdown_port_forwardslogs success but does NOT return error ifstate.port_forwards.lock()fails — could hide concurrency issuesEvidence:
src-tauri/src/commands/kube.rs:461:let mut port_forwards = state.port_forwards.lock().await;— no error handling if lock poisoning occurs.Fix: Map error to user-friendly string and propagate.
[SUGGESTION] src-tauri/src/commands/kube.rs:222-228 — Comment says dynamic port allocation “requires parsing kubectl output which is complex and error-prone” — this is the primary blocker for v1.1.0
Evidence:
src-tauri/src/commands/kube.rs:225: “For now, users must specify a local port or use the default behavior”. This contradicts PR title “Implement complete kubectl port-forward runtime”.Fix: Implement stderr parsing to capture dynamic port allocation — add unit test for regex pattern matching kubectl’s “Forwarding from 127.0.0.1:8080” line.
Verdict: REQUEST CHANGES
The critical issues in cluster/model mismatch, temp file cleanup, and port reporting prevent safe merging. The remaining findings risk data inconsistency and poor UX. Address BLOCKER items before v1.1.0 release.
Automated PR Review could not be completed - LLM analysis failed or produced no output.
Automated PR Review could not be completed - LLM analysis failed or produced no output.
44863d7f9ftoe56a72a31aAutomated PR Review could not be completed - LLM analysis failed or produced no output.
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR implements the kubectl port-forward runtime with full subprocess execution, async cleanup, and database persistence. However, critical concurrency and error handling issues in the Child process management — specifically the use of
std::sync::Mutexwithtokio::process::Child, improper awaited kill, and missing temp file cleanup during async stop — introduce potential hangs, resource leaks, and panics under concurrent usage or errors.Findings
[BLOCKER] src-tauri/src/kube/portforward.rs:83 -
spawn_child_waiterusesArc<TokioMutex<Option<Child>>>but the storedchildis not cloned into the async task closure properly, leading to a race condition where the child can betake()before the async task runs, causing a panic onexpect("Child not set").Evidence:
let child_for_task = child_arc.clone(); ... let mut child = child_for_task.lock().await.take().expect("Child not set");— thetake()can returnNoneifstop_asyncorcloseruns concurrently before this task executes.[BLOCKER] src-tauri/src/kube/portforward.rs:105 - The temp kubeconfig file cleanup in
spawn_child_waiteroccurs only when the child exits (not on session stop or drop), butstop_asyncandclosenever clean it up immediately.Evidence:
if let Some(path) = &temp_path_clone { let _ = std::fs::remove_file(path); }is inside the asyncwait().awaitbranch, not instop_async/close. If user stops a session while kubectl is still running, the temp file is never deleted.[WARNING] src-tauri/src/kube/portforward.rs:135-140 -
stop_asyncawaitschild.kill().awaitbut discards the result; errors from killing or waited-on cleanup are silently ignored, masking subprocess failures.Evidence:
let _ = child.kill().await;— if kill fails (e.g., permission, already exited), the error is discarded without logging or state update.[WARNING] src-tauri/src/kube/portforward.rs:182 -
DropforPortForwardSessionavoids killing the child and assumes background task will handle cleanup, but if the session is dropped before the background task spawns or before being stored in theport_forwardsmap, the Child is never waited and temp files may leak.Evidence:
self.child_wait_handle = None;— no explicit kill, relies on background task being active, which is not guaranteed in early failure paths.[BLOCKER] src-tauri/src/commands/kube.rs:535-539 -
spawn_child_waiterstores child insideArc<TokioMutex<Option<Child>>>but then stores thatArcinside the session — the sameArcis used both in the async task and externally (e.g.,stop_async). If multiple callerslock()the mutex concurrently, it can deadlock becauseTokioMutexdoes not prevent re-entrancy by different tasks on the same thread.Evidence:
let child_arc = Arc::new(TokioMutex::new(Some(child))); ... let child_for_task = child_arc.clone();—child_arcis held in the session and also used in the async task. A concurrentstop_asyncthat locks andtake()-s the child while the wait task holds it can deadlock.[BLOCKER] src-tauri/src/commands/kube.rs:310 -
validate_resource_namedoes not sanitize thefield_nameparameter, allowing injection of newlines or control characters in error messages (UX/audit risk, not a direct security vulnerability but violates input validation best practices).Evidence:
return Err(format!("{} '{}' cannot start with a hyphen or dot", field_name, name));— iffield_namecontains"namespace\nmalicious payload", the error message includes the payload.[WARNING] src-tauri/src/commands/kube.rs:518-524 -
temp_kubeconfig_pathisSome(temp_path)but is never persisted in the database for session recovery; on app restart, orphaned kubectl processes and stale temp files accumulate.Evidence:
let session = PortForwardSession::new(PortForwardSessionConfig { ..., temp_kubeconfig_path: Some(temp_path), });— temp path is not stored inclusters/port_forwardstables.[SUGGESTION] src-tauri/src/commands/kube.rs:522-526 - Local port allocation comment notes
currently not captured, but failing to capture dynamic port allocation means the UI cannot show the actual port assigned by kubectl. The kubectlport-forwardoutput line is typicallyForwarding from 127.0.0.1:PORT -> CONTAINER_PORT. Capture this line from stderr/stdout and update sessionlocal_portsaccordingly.Evidence:
// Note: Dynamic port allocation (when local_port=0) currently returns 0— this is a known functional gap.Verdict: REQUEST CHANGES
Two real bugs confirmed and fixed in commit
3833d604.Fixed
Background task panic (portforward.rs:88-95)
Confirmed. The spawned task called take().expect("Child not set") — if stop_async or close ran first and drained the Arc, the task panicked. Replaced with if let Some(c) = take() { } else { return; } so a concurrent
stop causes a clean task exit rather than a panic.
Temp file leak in stop_async / close
Confirmed. Both methods drain the child from the shared Arc, which causes the background task to return early and skip its cleanup branch. Added std::fs::remove_file at the end of stop_async, close, and the Drop impl
to cover all exit paths.
Not Actioned — False Positives
TokioMutex deadlock
tokio::sync::Mutex only deadlocks when the same task re-acquires a lock it already holds. The background task and stop_async are different tasks — one waits for the other to release, which is the intended behaviour.
Not a deadlock.
field_name injection in validate_resource_name
field_name is always a hard-coded string literal ("namespace", "pod"). It is never user-controlled and is not a shell argument — it only appears in a Tauri error message returned to the frontend. Not an injection
surface.
Acknowledged — Not Blocking
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR implements the core Kubernetes port-forwarding runtime with kubectl subprocess management. While the architecture is generally sound and includes important security improvements (e.g., input validation), there are critical issues in the child process handling:
std::mem::dropis used incorrectly on theFuture<Output = ()>fromchild.kill(), andArc<Mutex<Child>>is used whereChildis notSend/Sync, risking undefined behavior. Additionally, temp file cleanup and error propagation have gaps.Findings
child.kill().awaitis called, but the return value is discarded withlet _ = child.kill().await;. Thekill()method returnsFuture<Output = ()>, but assigning it to_is not incorrect by itself. The real problem is insrc-tauri/src/commands/kube.rs:532-540wherechild.kill()is called and its return is dropped instead of awaited, but in this file it is awaited correctly. However, the more severe issue isArc<Mutex<Child>>used across async boundaries insrc-tauri/src/kube/portforward.rs:14andsrc-tauri/src/kube/portforward.rs:24.tokio::process::Childis notSendorSync, andstd::sync::Mutexdoes not make its contentsSend. StoringChildinArc<Mutex<Option<Child>>>as done here violates Rust's safety guarantees and can lead to data races or undefined behavior on async boundaries.Evidence:
pub struct ChildWaitHandle { pub join_handle: tokio::task::JoinHandle<()>, pub child: Arc<TokioMutex<Option<Child>>>, }Fix: Do not store
Childdirectly. UseJoinHandle<()>(spawn a background task that callschild.wait().await) and store only the join handle. Kill the background task by signaling via a flag. Do not store the child in shared state.stop_asyncandcloseboth callchild.kill().awaitbut they do not handle the result and do not propagate errors. This means failures to kill the subprocess are silently ignored. Additionally, the logic to take the child and then cleanup temp files can leave temp files uncleaned if the background task already ran.Evidence:
if let Some(mut child) = child_opt { let _ = child.kill().await; }Fix: Properly handle errors from
child.kill().awaitby logging or propagating them. Ensure temp file cleanup happens in a consistent order (either here or in the background task, but not both).TempFileCleanupis created but the destructor only runs if the variable is not dropped early. Instart_port_forward, the temp file path is created but theTempFileCleanupis not stored in the session struct, so it gets dropped immediately and cleanup happens before the kubectl process finishes. This causesKUBECONFIGto point to a deleted file, leading to kubectl failures or undefined behavior.Evidence:
let _cleanup = TempFileCleanup(temp_path.clone());followed bystd::fs::write(&temp_path, ...). The cleanup happens on drop of_cleanupat end of scope, which is before spawning the child and returning.Fix: Store
temp_pathin the session struct (e.g.,temp_kubeconfig_path), and perform cleanup only after the child process completes or is explicitly stopped.local_portis set to 0 whenrequest.local_port == 0, but the comment says "Let kubectl allocate dynamically (currently not captured)". If local_port is 0, kubectl allocates a random port but the application never captures it, so thePortForwardResponse.local_portsis[0]even though kubectl bound to a different port. This renders the local port information useless for the user.Evidence:
let local_port = if request.local_port > 0 { request.local_port } else { 0 };andlocal_ports: vec![local_port]Fix: Either require the user to specify a local port, or parse
kubectl's stderr to extract the allocated port and update the session state with it.stop_async, the code callschild.kill().awaitthen manually removes the temp file. However, if the background task is still running, it will try to remove the same file again in its cleanup code (line 150-151), causing a race condition and potential error noise.Evidence: In
stop_async:if let Some(ref path) = self.temp_kubeconfig_path { let _ = std::fs::remove_file(path); }and in the background task:if let Some(path) = &temp_path_clone { let _ = std::fs::remove_file(path); }Fix: Use a flag in the session to indicate cleanup has been done, and have the background task skip cleanup if the flag is set. Or, take full ownership of cleanup in either
stop_asyncor the background task, but not both.shutdown_port_forwardsremoves sessions from the map and callsclose().await, but it does not await the internalclose().awaitbefore removing from the map, and does not handle potential errors from the close operation. This could leave zombie processes ifclosefails.Evidence:
if let Some(mut session) = port_forwards.remove(&session_id) { session.close().await; }— theremovehappens beforeclosecompletes, sosessionis dropped afterclose, but there is no error handling.Fix:
awaitthe close and log any errors. Do not remove from map until after close completes if you need to guarantee cleanup.Verdict: REQUEST CHANGES
The identified issues are critical and must be addressed before merge. The use of
Childin shared async contexts violates Rust's safety guarantees and must be rewritten to avoid storingChildinArc<Mutex>. Temp file cleanup must be tied to the actual lifecycle of the kubectl process, and dynamic port allocation must either be properly captured or disabled with clear user guidance.