From 3e2e3f231bf6c3ae37259caf2de7bd94dae59175 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 30 Jun 2026 15:16:42 -0700 Subject: [PATCH] Third review pass: fix tar TOCTOU + transient backup status - F2: upload_host_file_to_container now reads the dropped file into a Vec inside the blocking task and sizes the tar entry from those exact bytes, rather than stat-then-stream where a file changing size between the stat and the read could desync the tar header and silently corrupt the archive. Still runs off the async worker; memory stays bounded by the 256 MiB drop cap. - F4: the "Backup saved" confirmation now auto-clears after 8s (guarded against clobbering a newer status message) instead of lingering in the project card's status line indefinitely. F1 (claimed AWS CLI regression from empty-env neutralization) was a false positive: verified against aws-cli 2.35 that an empty AWS_ACCESS_KEY_ID is treated as absent and botocore falls through to ~/.aws/credentials (the call reached AWS and returned InvalidClientTokenId for the file's key, not PartialCredentialsError). No change needed. cargo check / tsc / vitest all pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src-tauri/src/docker/exec.rs | 23 +++++++++------------ app/src/components/projects/ProjectCard.tsx | 6 +++++- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/app/src-tauri/src/docker/exec.rs b/app/src-tauri/src/docker/exec.rs index 0d5b83a..e9cce6e 100644 --- a/app/src-tauri/src/docker/exec.rs +++ b/app/src-tauri/src/docker/exec.rs @@ -279,11 +279,11 @@ impl ExecSessionManager { } } -/// Upload a host file into the container's `/tmp` under `dest_name`, building the -/// tar archive by streaming from the file inside a blocking task. Unlike reading -/// the whole file into a `Vec` and then handing it to `write_file_to_container` -/// (which holds two full-size copies — the data and the tar), this keeps only -/// the single tar buffer, and the synchronous file IO runs off the async worker. +/// Upload a host file into the container's `/tmp` under `dest_name`. The file is +/// read and packed into the tar inside a blocking task, so the synchronous IO +/// runs off the async worker. The tar's declared entry size is taken from the +/// bytes actually read (not a separate `stat`), so a file changing size between +/// a size check and the read can't desync the header and corrupt the archive. /// Returns the in-container path (`/tmp/`). pub async fn upload_host_file_to_container( container_id: &str, @@ -295,21 +295,18 @@ pub async fn upload_host_file_to_container( let dest_for_blk = dest_name.clone(); let tar_buf = tokio::task::spawn_blocking(move || -> Result, String> { - let mut file = std::fs::File::open(&host_path) + let data = std::fs::read(&host_path) .map_err(|e| format!("Failed to read {}: {}", host_path, e))?; - let size = file - .metadata() - .map_err(|e| format!("Failed to stat {}: {}", host_path, e))? - .len(); - let mut tar_buf = Vec::new(); + let mut tar_buf = Vec::with_capacity(data.len() + 1024); { let mut builder = tar::Builder::new(&mut tar_buf); let mut header = tar::Header::new_gnu(); - header.set_size(size); + // Size comes from the bytes in hand, so header and payload can't disagree. + header.set_size(data.len() as u64); header.set_mode(0o644); header.set_cksum(); builder - .append_data(&mut header, &dest_for_blk, &mut file) + .append_data(&mut header, &dest_for_blk, &data[..]) .map_err(|e| format!("Failed to create tar entry: {}", e))?; builder .finish() diff --git a/app/src/components/projects/ProjectCard.tsx b/app/src/components/projects/ProjectCard.tsx index 4013a53..af83630 100644 --- a/app/src/components/projects/ProjectCard.tsx +++ b/app/src/components/projects/ProjectCard.tsx @@ -196,7 +196,11 @@ export default function ProjectCard({ project }: Props) { setError(null); const bytes = await commands.downloadContainerBackup(project.id, hostPath); const mb = (bytes / (1024 * 1024)).toFixed(1); - setProgressMsg(`Backup saved (${mb} MB). Note: includes MCP/config — may contain MCP API keys. Keep it private.`); + const msg = `Backup saved (${mb} MB). Note: includes MCP/config — may contain MCP API keys. Keep it private.`; + setProgressMsg(msg); + // Auto-clear so the transient confirmation doesn't linger in the card + // status; guard against clobbering a newer message (e.g. a later op). + setTimeout(() => setProgressMsg((prev) => (prev === msg ? null : prev)), 8000); } catch (e) { setError(String(e)); } finally {