Judge a host path by where it leads, not by how it is spelled

`validate_host_path` was a string test. Nothing in the module called
`canonicalize`, `read_link` or `O_NOFOLLOW`, so a path whose components are
all visible could still land somewhere hidden: with `~/Downloads/pub` a
symlink to `~/.ssh`, a `host_path` of `~/Downloads/pub/authorized_keys` has no
hidden component, no `..` and no system root — and writes into `~/.ssh`. The
container end is not hypothetical: `/proc/self/mountinfo` inside a Triple-C
container spells the host's project paths out verbatim, so code in there knows
both where to plant the link and what host path to ask for. The same bypass
read host files back the other way.

So the policy now runs twice: once on the string, and once on what the OS says
the string resolves to. A write resolves the parent and keeps the caller's
leaf, because the leaf is never followed — the partial file is created with
`O_EXCL` and the download finishes with a rename, which replaces a link rather
than writing through it. A read resolves the whole path, because the whole
path is opened. On Linux the descriptor is then checked against the path that
was validated (`/proc/self/fd`), which is what closes the window between
resolving and opening; elsewhere that window stays open and the comment says so.

Also here:

  * The upload's overwrite guard is a guard again. `noOverwriteDirNonDir`
    refuses only dir-over-non-dir and the reverse — file-over-file extraction
    proceeds, which is exactly the `.credentials.json` case (verified against a
    live daemon). The probe and the write are now one `set -C` exclusive
    create, with the path travelling as `$0` rather than as script. The
    `FILE_EXISTS: <path> already exists` contract with the frontend is
    unchanged, and now pinned by a test — as is the claim the old comment made.
  * Windows normalisation stopped being a string swap: `\\?\`, `\\?\UNC\` and
    administrative shares all reach the same places and are compared as such,
    and the rules are pure functions over a string, so the Windows entries are
    exercised on any platform. The old test passed on Linux only because
    `Path::is_absolute` was false for a Windows path.
  * Container write roots are resolved inside the container too, and the
    comment no longer claims more than the check does.
  * A failed download can no longer delete a pre-existing file that happened to
    collide with the partial's name.
  * One-shot exec output is buffered as bytes and decoded once, so a filename
    split across two Docker frames survives; stdout and stderr are tellable
    apart, so `find`'s diagnostics stay out of the listing parser; and a
    directory too big to buffer is described as one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc

app/src-tauri/src/commands/terminal_commands.rs

@@ -199,8 +199,11 @@ pub async fn upload_host_file_to_terminal(
     // 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.
+    // (`~/.ssh`, `~/.aws`) or a system location — applied to the path with its
-    let host_path = crate::commands::file_commands::validate_host_read_path(&host_path)?;
+    // symlinks already resolved, so a visible directory that *leads* to `~/.ssh`
+    // is refused too. What comes back is that resolved path, and it is what
+    // gets opened.
+    let host_path = crate::commands::file_commands::resolve_host_read_path(&host_path).await?;
 
     let container_id = state.exec_manager.get_container_id(&session_id).await?;
 
@@ -212,8 +215,11 @@ pub async fn upload_host_file_to_terminal(
     }
 
     // Guard against ballooning host RAM: the file is packed into an in-memory
-    // tar before upload, so cap the size of a dropped file.
+    // tar before upload, so cap the size of a dropped file. The ceiling lives
-    const MAX_DROP_BYTES: u64 = 256 * 1024 * 1024; // 256 MiB
+    // with the code that does the reading, which re-applies it to the open
+    // descriptor — this check is here only so the refusal reads like a sentence
+    // instead of arriving after a 300 MB read.
+    use crate::docker::exec::MAX_DROP_BYTES;
     if meta.len() > MAX_DROP_BYTES {
         return Err(format!(
             "File too large to drop into the terminal ({:.0} MB; limit {} MB). Mount it into the project or use the Files panel instead.",

app/src-tauri/src/docker/exec.rs

@@ -322,6 +322,14 @@ impl ExecSessionManager {
     }
 }
 
+/// Ceiling on one host file packed into a container upload.
+///
+/// The file goes through host RAM twice — once as bytes, once inside the tar —
+/// so this is a memory bound, and it is checked against the *descriptor* that
+/// was opened rather than a `metadata` call that described whatever the path
+/// meant a moment earlier.
+pub const MAX_DROP_BYTES: u64 = 256 * 1024 * 1024;
+
 /// 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
@@ -340,8 +348,29 @@ pub async fn upload_host_file_to_container(
     let mtime = now_epoch_secs();
 
     let tar_buf = tokio::task::spawn_blocking(move || -> Result<Vec<u8>, String> {
-        let data = std::fs::read(&host_path)
+        // The caller resolved this path (`resolve_host_read_path`); opening it
+        // is a second trip through the same directories, so the descriptor is
+        // checked against the path that was validated before its bytes are
+        // packed into anything. Same policy as the Files pane's upload — this
+        // is the terminal's drop target, and the two must not differ.
+        let file = std::fs::File::open(&host_path)
             .map_err(|e| format!("Failed to read {}: {}", host_path, e))?;
+        crate::commands::file_commands::verify_opened_path(
+            &file,
+            std::path::Path::new(&host_path),
+        )?;
+        let mut data = Vec::new();
+        std::io::Read::read_to_end(
+            &mut std::io::Read::take(file, MAX_DROP_BYTES.saturating_add(1)),
+            &mut data,
+        )
+        .map_err(|e| format!("Failed to read {}: {}", host_path, e))?;
+        if data.len() as u64 > MAX_DROP_BYTES {
+            return Err(format!(
+                "File too large to upload (limit {} MB)",
+                MAX_DROP_BYTES / (1024 * 1024)
+            ));
+        }
         build_single_file_tar(&dest_for_blk, &data[..], 0o644, uid, gid, mtime)
     })
     .await
@@ -484,14 +513,31 @@ pub const MAX_ONESHOT_OUTPUT: usize = 8 * 1024 * 1024;
 /// past anything genuine, far short of a problem.
 pub const PROC_NET_OUTPUT_LIMIT: usize = 1024 * 1024;
 
-/// Append to `buf` while it stays inside `limit`. Returns `false` once the
+/// Marker on the "that command printed more than this will buffer" refusal.
-/// limit is exceeded, at which point the caller must stop reading.
+///
-fn push_capped(buf: &mut String, chunk: &str, limit: usize) -> bool {
+/// The byte count on its own is a fact about the transport, not about what the
+/// user did — "Command output exceeded 8388608 bytes" is not a sentence anybody
+/// can act on. A caller that knows what it was reading can recognise this and
+/// say the useful thing instead; see `list_container_files`, where the real
+/// cause is a directory with more entries than the panel can render.
+pub const OUTPUT_LIMIT_MARKER: &str = "OUTPUT_LIMIT";
+
+/// Append to `buf` while it stays inside `limit`, returning the range the chunk
+/// now occupies. `None` once the limit is exceeded, at which point the caller
+/// must stop reading — and nothing is appended, so a caller that ignored the
+/// answer cannot parse a half-read document.
+///
+/// Bytes rather than `str` on purpose: Docker frames a stream wherever it
+/// likes, so a chunk boundary can fall inside a UTF-8 sequence. Decoding each
+/// chunk on its own turned that into two replacement characters in the middle
+/// of a filename; the decode happens once, at the end, over the whole buffer.
+fn push_capped(buf: &mut Vec<u8>, chunk: &[u8], limit: usize) -> Option<(usize, usize)> {
     if buf.len() + chunk.len() > limit {
-        return false;
+        return None;
     }
-    buf.push_str(chunk);
+    let start = buf.len();
-    true
+    buf.extend_from_slice(chunk);
+    Some((start, buf.len()))
 }
 
 /// Run a one-shot (non-interactive) exec command in a container and collect stdout.
@@ -555,6 +601,65 @@ pub async fn exec_oneshot_as(
     exec_oneshot_inner(container_id, user, cmd, env, MAX_ONESHOT_OUTPUT).await
 }
 
+/// What a one-shot exec printed, with the two streams still tellable apart.
+///
+/// `combined` is stdout and stderr interleaved in arrival order — the shape
+/// every existing caller reads, and the right one for surfacing "why did that
+/// fail". `stdout_ranges` indexes the parts of it that came from stdout, so a
+/// caller that is *parsing* output can have just that without the buffer being
+/// held twice.
+struct OneshotOutput {
+    combined: Vec<u8>,
+    stdout_ranges: Vec<(usize, usize)>,
+    exit_code: i64,
+}
+
+impl OneshotOutput {
+    /// Everything the command printed, in the order it printed it.
+    fn text(&self) -> String {
+        String::from_utf8_lossy(&self.combined).into_owned()
+    }
+
+    /// stdout alone — for callers that parse it, where a diagnostic spliced in
+    /// mid-record is a parse error at best.
+    fn stdout(&self) -> String {
+        let mut out = Vec::with_capacity(self.combined.len());
+        for (start, end) in &self.stdout_ranges {
+            out.extend_from_slice(&self.combined[*start..*end]);
+        }
+        String::from_utf8_lossy(&out).into_owned()
+    }
+
+    /// stderr alone — the complement of [`Self::stdout`], i.e. the diagnostics.
+    fn stderr(&self) -> String {
+        let mut out = Vec::with_capacity(self.combined.len());
+        let mut cursor = 0usize;
+        for (start, end) in &self.stdout_ranges {
+            out.extend_from_slice(&self.combined[cursor..*start]);
+            cursor = *end;
+        }
+        out.extend_from_slice(&self.combined[cursor..]);
+        String::from_utf8_lossy(&out).into_owned()
+    }
+}
+
+/// [`exec_oneshot_as`] with the two streams kept apart, for callers that parse
+/// stdout.
+///
+/// `find`'s own diagnostics ("Permission denied") used to arrive inside the
+/// records its `-printf` was emitting. GNU `find` escapes tabs and newlines in
+/// those messages, so the listing parser held — but "the parser holds" is not
+/// the same as "the input is trustworthy", and the fix costs one enum match.
+pub async fn exec_oneshot_streams_as(
+    container_id: &str,
+    user: &str,
+    cmd: Vec<String>,
+    env: Vec<String>,
+) -> Result<(String, String, i64), String> {
+    let out = exec_oneshot_raw(container_id, user, cmd, env, MAX_ONESHOT_OUTPUT).await?;
+    Ok((out.stdout(), out.stderr(), out.exit_code))
+}
+
 async fn exec_oneshot_inner(
     container_id: &str,
     user: &str,
@@ -562,6 +667,17 @@ async fn exec_oneshot_inner(
     env: Vec<String>,
     limit: usize,
 ) -> Result<(String, i64), String> {
+    let out = exec_oneshot_raw(container_id, user, cmd, env, limit).await?;
+    Ok((out.text(), out.exit_code))
+}
+
+async fn exec_oneshot_raw(
+    container_id: &str,
+    user: &str,
+    cmd: Vec<String>,
+    env: Vec<String>,
+    limit: usize,
+) -> Result<OneshotOutput, String> {
     let docker = get_docker()?;
 
     let exec = docker
@@ -584,22 +700,31 @@ async fn exec_oneshot_inner(
         .await
         .map_err(|e| format!("Failed to start exec: {}", e))?;
 
-    let mut combined = String::new();
+    let mut combined: Vec<u8> = Vec::new();
+    let mut stdout_ranges: Vec<(usize, usize)> = Vec::new();
     match result {
         StartExecResults::Attached { mut output, .. } => {
             while let Some(msg) = output.next().await {
                 match msg {
                     Ok(data) => {
-                        let chunk = String::from_utf8_lossy(&data.into_bytes()).into_owned();
+                        let from_stdout = matches!(data, LogOutput::StdOut { .. });
-                        if !push_capped(&mut combined, &chunk, limit) {
+                        let bytes = data.into_bytes();
+                        match push_capped(&mut combined, &bytes, limit) {
+                            Some(range) => {
+                                if from_stdout {
+                                    stdout_ranges.push(range);
+                                }
+                            }
                             // Stop reading rather than truncate silently: every
                             // caller parses this output, and a half-read
                             // manifest or JSON array is worse than an error.
                             // Dropping `output` kills the exec's stream.
+                            None => {
                                 return Err(format!(
-                                "Command output exceeded {} bytes and was abandoned",
+                                    "{}: Command output exceeded {} bytes and was abandoned",
-                                limit
+                                    OUTPUT_LIMIT_MARKER, limit
-                            ));
+                                ))
+                            }
                         }
                     }
                     Err(e) => return Err(format!("Exec output error: {}", e)),
@@ -613,7 +738,11 @@ async fn exec_oneshot_inner(
     // final exit_code populated yet, so poll until the exec reports finished.
     let exit_code = require_exit_code(wait_for_exec_exit(&exec.id).await)?;
 
-    Ok((combined, exit_code))
+    Ok(OneshotOutput {
+        combined,
+        stdout_ranges,
+        exit_code,
+    })
 }
 
 /// Turn "the exit code could not be determined" into an error rather than a 0.
@@ -665,31 +794,86 @@ pub async fn wait_for_exec_exit(exec_id: &str) -> Option<i64> {
 mod tests {
     use super::*;
 
+    /// The frames a demultiplexed exec hands back, as `(is_stdout, bytes)`.
+    fn collect(frames: &[(bool, &[u8])]) -> OneshotOutput {
+        let mut combined = Vec::new();
+        let mut stdout_ranges = Vec::new();
+        for (from_stdout, bytes) in frames {
+            let range = push_capped(&mut combined, bytes, usize::MAX).unwrap();
+            if *from_stdout {
+                stdout_ranges.push(range);
+            }
+        }
+        OneshotOutput {
+            combined,
+            stdout_ranges,
+            exit_code: 0,
+        }
+    }
+
     #[test]
     fn output_under_the_limit_is_buffered_whole() {
-        let mut buf = String::new();
+        let mut buf = Vec::new();
-        assert!(push_capped(&mut buf, "hello ", 16));
+        assert_eq!(push_capped(&mut buf, b"hello ", 16), Some((0, 6)));
-        assert!(push_capped(&mut buf, "world", 16));
+        assert_eq!(push_capped(&mut buf, b"world", 16), Some((6, 11)));
-        assert_eq!(buf, "hello world");
+        assert_eq!(buf, b"hello world");
     }
 
     #[test]
     fn output_over_the_limit_is_refused_rather_than_truncated() {
         // The abandoned chunk must not land in the buffer either: a caller that
         // ignored the error would otherwise parse a half-read document.
-        let mut buf = String::new();
+        let mut buf = Vec::new();
-        assert!(push_capped(&mut buf, "0123456789", 12));
+        assert!(push_capped(&mut buf, b"0123456789", 12).is_some());
-        assert!(!push_capped(&mut buf, "0123456789", 12));
+        assert!(push_capped(&mut buf, b"0123456789", 12).is_none());
-        assert_eq!(buf, "0123456789");
+        assert_eq!(buf, b"0123456789");
     }
 
     #[test]
     fn a_single_oversized_chunk_is_refused() {
-        let mut buf = String::new();
+        let mut buf = Vec::new();
-        assert!(!push_capped(&mut buf, "0123456789", 4));
+        assert!(push_capped(&mut buf, b"0123456789", 4).is_none());
         assert!(buf.is_empty());
     }
 
+    #[test]
+    fn a_character_split_across_two_frames_survives_the_decode() {
+        // Docker frames a stream wherever it likes, and a filename is where
+        // that shows: decoding each chunk on its own turned the two halves of
+        // `ü` into two replacement characters in the middle of a name.
+        let out = collect(&[(true, &[0xc3]), (true, &[0xbc, b'.', b't', b'x', b't'])]);
+        assert_eq!(out.stdout(), "ü.txt");
+        assert_eq!(out.text(), "ü.txt");
+    }
+
+    #[test]
+    fn a_diagnostic_never_lands_in_the_stream_a_caller_parses() {
+        // `find`'s "Permission denied" used to arrive inside the records its
+        // `-printf` was emitting. Arrival order is still available for the
+        // error message; the parser gets stdout alone.
+        let out = collect(&[
+            (true, b"first"),
+            (false, b"find: /x: Permission denied\n"),
+            (true, b"second"),
+        ]);
+        assert_eq!(out.stdout(), "firstsecond");
+        assert_eq!(out.stderr(), "find: /x: Permission denied\n");
+        assert_eq!(out.text(), "firstfind: /x: Permission denied\nsecond");
+    }
+
+    #[test]
+    fn an_output_limit_refusal_is_marked_so_a_caller_can_reword_it() {
+        // "Command output exceeded 8388608 bytes" is a fact about a buffer.
+        // The marker is what lets `list_container_files` say "too many entries"
+        // instead, which is the thing that actually happened.
+        assert!(!OUTPUT_LIMIT_MARKER.is_empty());
+        let refusal = format!(
+            "{}: Command output exceeded {} bytes and was abandoned",
+            OUTPUT_LIMIT_MARKER, MAX_ONESHOT_OUTPUT
+        );
+        assert!(refusal.starts_with(OUTPUT_LIMIT_MARKER));
+    }
+
     #[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