Ship the Files tab container-side only
Four successive audits found the same thing: host filesystem paths crossing
IPC is where the criticals in this work live. The most recent one found the
`link(2)` upload reservation returning success against a *directory* (linking
into it, leaving permanent stray files, and via a symlink-to-directory writing
outside the validated write root), failing every upload permanently on any
filesystem without hard links, and the post-resolution credential check
weakened from a general rule to an eleven-name denylist.
Rather than fix that a fifth time, the Files tab ships as what it is good at:
a browser, viewer and renamer that never touches the host.
Removed: `upload_file_to_container`, `download_container_file`, and everything
that existed only for them — the whole reservation (`UPLOAD_RESERVATION_SCRIPT`,
`reserve_upload_destination`, the placeholder rollback, `exec_oneshot_as_within`
which had no other caller), `stream_container_file_to_host`, `ChannelReader`,
`save_to_host`, the download ceiling, and the collision marker with its
frontend contract. On the frontend: the upload button, the pane's
`onDragDropEvent` handler, both "Save to host…" affordances, `uploadPaths` /
`downloadFile` / the overwrite prompt, and `OverwriteConfirmModal`.
`lib/uploadErrors.ts` is now `lib/refusalText.ts` and keeps only the half that
turns any backend refusal into the sentence a person reads.
Kept, and not weakened: `upload_host_file_to_terminal` and
`download_container_backup`. They predate this work, their hardening is a real
improvement over main, and they are now the whole answer to "how do I get a
file in or out" — drop it on the Terminal, or Back up container. The drop gate
(`lib/dropTarget.ts`, `PaneVisibility`) is untouched.
`resolve_host_path` gets the general hidden-component rule back. Round 3
replaced it with `HOST_CREDENTIAL_DIRS`, which is allow-by-omission for the
rest of `$HOME`: `~/.local/bin` (write there and you own the user's next shell
command), `~/.password-store`, browser profiles and `~/.pki/nssdb` were all
reachable through a planted symlink with a visible name — verified against a
real home directory, and all five refused now. It over-catches `.pnpm` and
`~/.cache`; for two occasional callers that is the cheaper mistake, and the
refusal says which folder it resolved through.
Two defects fixed while in here:
* A symlinked directory listed as empty. `find` defaults to `-P`, which does
not follow a symlink even as the starting point, so `-mindepth 1` discarded
the only match and a real directory rendered as "Empty directory" — a
first-order defect now that browsing *is* the feature. `-H` follows the
starting point and nothing else, so a loop is `ELOOP` rather than a walk
that does not end; verified against a live container for a symlinked
directory, a broken link and a loop. `find`'s errno for the loop case is
now a sentence.
* `finish_download`'s replace path fired on *any* rename failure with a
destination present — a vanished partial, a permission error, a directory
at the destination — and deleted the user's file to complete a move that
could not complete. It is now fenced to Windows (where a rename onto an
existing path genuinely fails) and to a partial that still exists.
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
@@ -197,18 +197,17 @@ pub async fn upload_host_file_to_terminal(
|
||||
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 — applied to the path with its
|
||||
// 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.
|
||||
// itself, so it goes through `file_commands`' host-read policy: absolute,
|
||||
// no traversal, and nothing whose path passes through a hidden directory
|
||||
// (`~/.ssh`, `~/.aws`, `~/.local/bin`) or a system location — applied to
|
||||
// the path with its symlinks already resolved, so a visible directory that
|
||||
// *leads* to one of those is refused too. What comes back is that resolved
|
||||
// path, and it is what gets opened. This is now one of only two commands
|
||||
// that touch a host path at all; the other is `download_container_backup`.
|
||||
// The name is taken from the path the user actually dropped, *before*
|
||||
// resolution. Deriving it from the resolved path renames the file behind
|
||||
// the user's back: dropping `~/Downloads/latest.log`, where `latest.log` is
|
||||
// a symlink, would land it in the container as `2026-08-23.log`. The Files
|
||||
// pane's upload had the same bug and fixes it the same way — one helper, so
|
||||
// the two drop targets cannot drift.
|
||||
// a symlink, would land it in the container as `2026-08-23.log`.
|
||||
let base = crate::commands::file_commands::host_upload_name(&host_path)?;
|
||||
let host_path = crate::commands::file_commands::resolve_host_read_path(&host_path).await?;
|
||||
|
||||
@@ -229,7 +228,7 @@ pub async fn upload_host_file_to_terminal(
|
||||
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.",
|
||||
"File too large to drop into the terminal ({:.0} MB; limit {} MB). Mount it into the project instead.",
|
||||
meta.len() as f64 / (1024.0 * 1024.0),
|
||||
MAX_DROP_BYTES / (1024 * 1024)
|
||||
));
|
||||
@@ -301,19 +300,18 @@ pub async fn stop_audio_bridge(
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
/// Both drop targets must name a dropped file the way the *user* named it.
|
||||
/// A dropped file must be named the way the *user* named it.
|
||||
///
|
||||
/// The bug this pins: `upload_host_file_to_terminal` derived the tar entry
|
||||
/// name from the path *after* symlink resolution, so dropping
|
||||
/// `~/Downloads/latest.log` — where `latest.log` is a symlink to
|
||||
/// `2026-08-23.log` — silently landed the file in the container under the
|
||||
/// target's name. Nothing errored; the user just got a name they never
|
||||
/// typed. The Files pane had the identical bug.
|
||||
/// typed.
|
||||
///
|
||||
/// What actually keeps the two from drifting is that they now call one
|
||||
/// helper, so this asserts that helper's contract from the terminal side:
|
||||
/// the answer comes from the spelling, and a path that does not name a file
|
||||
/// is refused rather than silently substituted (it used to fall back to
|
||||
/// This asserts the shared helper's contract from the terminal side: the
|
||||
/// answer comes from the spelling, and a path that does not name a file is
|
||||
/// refused rather than silently substituted (it used to fall back to
|
||||
/// `"dropped-file"`).
|
||||
#[test]
|
||||
fn a_dropped_file_keeps_the_name_the_user_dropped() {
|
||||
|
||||
@@ -351,8 +351,8 @@ pub async fn upload_host_file_to_container(
|
||||
// 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.
|
||||
// packed into anything. This is the terminal's drop target, and it is
|
||||
// the only path by which host bytes enter a container.
|
||||
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(
|
||||
@@ -601,44 +601,6 @@ pub async fn exec_oneshot_as(
|
||||
exec_oneshot_inner(container_id, user, cmd, env, MAX_ONESHOT_OUTPUT).await
|
||||
}
|
||||
|
||||
/// [`exec_oneshot_as`] with a wall-clock ceiling on the whole call.
|
||||
///
|
||||
/// H8. Nothing in this module bounds how long a container command may take,
|
||||
/// which is right for the callers that need it — a base-image migration replays
|
||||
/// `apt-get` and takes minutes — and wrong for a short command that can be made
|
||||
/// to block forever by a *file* the caller does not control. The upload
|
||||
/// reservation is the one that bit: a shell redirect onto a FIFO blocks in
|
||||
/// `open(2)` until a reader appears, so a single `mkfifo` in a project
|
||||
/// directory left the Files pane on "Uploading…" for the rest of the session
|
||||
/// with the rest of the batch abandoned.
|
||||
///
|
||||
/// So the ceiling is opt-in per call site rather than global. Note what it can
|
||||
/// and cannot do: dropping the future closes our end of the stream, but Docker
|
||||
/// has no "kill an exec" API, so a process that is genuinely wedged stays
|
||||
/// wedged in the container's process table. That is why the primitive matters
|
||||
/// more than the timeout — this turns "the app never comes back" into "that
|
||||
/// upload failed", and it is the caller's job not to run something that blocks.
|
||||
pub async fn exec_oneshot_as_within(
|
||||
container_id: &str,
|
||||
user: &str,
|
||||
cmd: Vec<String>,
|
||||
env: Vec<String>,
|
||||
limit: std::time::Duration,
|
||||
) -> Result<(String, i64), String> {
|
||||
match tokio::time::timeout(
|
||||
limit,
|
||||
exec_oneshot_inner(container_id, user, cmd, env, MAX_ONESHOT_OUTPUT),
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(result) => result,
|
||||
Err(_) => Err(format!(
|
||||
"The container did not answer within {}s — the command may still be running inside it.",
|
||||
limit.as_secs()
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
/// What a one-shot exec printed, with the two streams still tellable apart.
|
||||
///
|
||||
/// `combined` is stdout and stderr interleaved in arrival order — the shape
|
||||
|
||||
@@ -497,9 +497,7 @@ pub fn run() {
|
||||
commands::terminal_commands::stop_audio_bridge,
|
||||
// Files
|
||||
commands::file_commands::list_container_files,
|
||||
commands::file_commands::download_container_file,
|
||||
commands::file_commands::download_container_backup,
|
||||
commands::file_commands::upload_file_to_container,
|
||||
commands::file_commands::read_container_file,
|
||||
commands::file_commands::rename_container_path,
|
||||
commands::file_commands::create_container_directory,
|
||||
|
||||
Reference in New Issue
Block a user