Fix the file command surface: argument injection, unbounded reads, unchecked paths
`list_container_files` could delete the user's files. It builds `["find", path, "-mindepth", …]`, and GNU find ends its starting-point list at the first argument beginning with `-`, so a `path` of `-delete` gave it zero starting points (defaulting to `.`, which for an exec that sets no working_dir is the container's WorkingDir — the bind-mounted project) and an expression starting with `-delete`. Verified against a live container on findutils 4.10.0: files and empty directories went out of the host bind mount, and because `exec_oneshot` discards the exit code the panel then reported an empty folder. The rest of the module had the same shape of hole: * Every path parameter — `path`, `from_path`, `parent_path`, `container_dir`, `container_path`, `host_path` — arrived over IPC unchecked. There is now one validator for container paths (absolute, no `..`, no NUL, length-capped), a second for the ones that *change* something (contained in /workspace, /home/claude or /tmp), and one for host paths, which refuses traversal, system locations and hidden components. The `save()`/`open()` dialog in front of these commands is a UI convention, not a boundary. * `download_container_file` passed `None` for the fetch cap, so the cap was inert: the whole transfer was buffered in host RAM twice, and the directory refusal came *after* the buffer, so `/` meant buffering the container's filesystem before erroring. Downloads now stream through a bounded channel into the tar reader, which refuses a non-regular entry and an oversize one before the host file is created at all. Verified against a real container: a 300 MiB download peaks at 10 MiB RSS, a 9 GiB sparse file is refused in 0.01s with nothing written. * Both download paths (file and backup) used to create — i.e. truncate — the user's destination up front and delete it on a stream error, which is precisely the wrong order for a path that already holds something. They now write beside it and rename on success. * `upload_file_to_container` silently clobbered: no existence check anywhere in the stack. It now refuses by default with a FILE_EXISTS marker the frontend turns into a Replace/Skip prompt, and takes an `overwrite` flag for the retry. * `exec_oneshot_inner` read an undeterminable exit status as 0, so rename and mkdir reported success for an exec nobody could read the outcome of. It fails closed now. * A tab in a filename forged the type/size/permission columns of a listing row, and a newline forged a whole row. `find` now prints the name last with NUL-terminated records. While verifying the size ceiling against a real container, the tar header's size field turned out to be unusable past ustar's 8 GiB octal limit — Docker's Go writer puts the real size in a PAX record — so both readers take it from `entry.size()` instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
File diff suppressed because it is too large
Load Diff
@@ -196,6 +196,12 @@ pub async fn upload_host_file_to_terminal(
|
||||
host_path: String,
|
||||
state: State<'_, AppState>,
|
||||
) -> Result<String, String> {
|
||||
// The drop target is a host path chosen by the webview, not by the OS drag
|
||||
// itself, so it gets the same host-read policy as the Files pane's upload:
|
||||
// absolute, no traversal, and nothing out of a hidden directory
|
||||
// (`~/.ssh`, `~/.aws`) or a system location.
|
||||
let host_path = crate::commands::file_commands::validate_host_read_path(&host_path)?;
|
||||
|
||||
let container_id = state.exec_manager.get_container_id(&session_id).await?;
|
||||
|
||||
let meta = tokio::fs::metadata(&host_path)
|
||||
|
||||
@@ -611,11 +611,29 @@ async fn exec_oneshot_inner(
|
||||
|
||||
// The output stream draining doesn't strictly guarantee inspect_exec has the
|
||||
// final exit_code populated yet, so poll until the exec reports finished.
|
||||
let exit_code = wait_for_exec_exit(&exec.id).await.unwrap_or(0);
|
||||
let exit_code = require_exit_code(wait_for_exec_exit(&exec.id).await)?;
|
||||
|
||||
Ok((combined, exit_code))
|
||||
}
|
||||
|
||||
/// Turn "the exit code could not be determined" into an error rather than a 0.
|
||||
///
|
||||
/// `unwrap_or(0)` is how a rename that never happened reported success: callers
|
||||
/// branch on `code != 0`, so an unreadable status silently became "it worked",
|
||||
/// the UI closed its rename box and the file had not moved. An exec whose
|
||||
/// outcome cannot be established has not been established to have succeeded —
|
||||
/// fail closed and let the caller surface it.
|
||||
///
|
||||
/// The `test -e` probe in `rename_container_path` also fails closed under this:
|
||||
/// it propagates the error instead of reading an undeterminable status as
|
||||
/// "the destination does not exist".
|
||||
fn require_exit_code(code: Option<i64>) -> Result<i64, String> {
|
||||
code.ok_or_else(|| {
|
||||
"Could not determine whether the command finished (Docker did not report an exit status)"
|
||||
.to_string()
|
||||
})
|
||||
}
|
||||
|
||||
/// Poll `inspect_exec` until the exec reports finished and return its exit code.
|
||||
/// Returns `None` if the code can't be determined (inspect error, or the exec
|
||||
/// doesn't report finished within ~1s — which shouldn't happen once its output
|
||||
@@ -666,6 +684,16 @@ mod tests {
|
||||
assert!(buf.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_undeterminable_exit_status_is_an_error_not_a_zero() {
|
||||
// The bug this guards: `unwrap_or(0)` made every caller that branches on
|
||||
// `code != 0` — rename, mkdir — report success for an exec whose outcome
|
||||
// nobody could read.
|
||||
assert_eq!(require_exit_code(Some(0)).unwrap(), 0);
|
||||
assert_eq!(require_exit_code(Some(1)).unwrap(), 1);
|
||||
assert!(require_exit_code(None).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_bridge_budget_is_far_smaller_than_the_general_one() {
|
||||
// The auth bridge re-reads container-controlled procfs every 2s, so it
|
||||
|
||||
Reference in New Issue
Block a user