Close what two reviews found in the Files tab transfers
Build App (Preview) / compute-version (pull_request) Successful in 5s
Build Container / build-container (pull_request) Successful in 37s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m40s
Build App (Preview) / build-windows (pull_request) Successful in 4m56s
Build App (Preview) / build-linux (pull_request) Successful in 5m11s
Build App (Preview) / prune-previews (pull_request) Successful in 1s

Two independent reviews of 2c9482a, one for correctness and one against the
threat model. Between them they found two ways to lose a file, one way for a
container to choose a Windows save destination, and a rule that made every
dotfile unsavable. Every finding below was demonstrated against a real
container before being fixed, and the fixes are demonstrated the same way.

## The download was accepting truncated reads and refusing whole ones

The `[ -f ]` bracket around the read was wrong in both directions.

It missed the case that loses data. Truncation *in place* — `> file`, log
rotation, `tar -x`, most build tools — leaves a regular file behind, so `dd`
stopped at the new EOF and exited 0. Measured: a 600 MB source truncated
mid-read delivered 34 MB, which was then renamed over the user's own earlier
copy and toasted as `Saved (34.1 MB)`. Truncate-to-zero did the same and needs
no adversary at all.

And it failed *good* downloads. `dd` already holds the fd, and neither `rm` nor
`mv` can touch an open one — the bytes are complete. But `rm` makes `[ -f ]`
false, so a finished 600 MB transfer of a file a bundler happened to unlink was
deleted, reporting "nothing was saved". The comment claiming the bracket caught
a "mix of two files" was simply wrong; an open fd cannot be a mix.

So the script now measures the file before reading it and puts the answer on
stderr, and Rust checks that at least that many bytes arrived. One rule,
subsuming everything the bracket was for. Verified against a real container:
truncation mid-read and the FIFO race are refused; deletion, rename-over, a
growing file, an empty file and an untouched 600 MB read all pass.

## On Windows the container, not the user, was naming the save destination

`suggested_save_name` split on `/` only, and it feeds the save dialog's
pre-filled name. Backslash is a legal Linux filename character and
`validate_container_path` has no reason to object — `..\..\Users\…` is one
POSIX segment. The Windows common file dialog parses its name box as a path on
Save, so a container-created file called
`..\..\..\Users\vic\AppData\Roaming\Microsoft\Word\STARTUP\x.dotm` put its own
bytes in an auto-loading Office directory on one un-read click. `resolve_host_path`
did not stop it: Word's `STARTUP` and Excel's `XLSTART` are not in the autorun
denylist, which this file's own docs already concede is "losing by
construction" and was never meant to be the boundary here.

The name is now sanitized of every separator, the drive colon and the rest of
what NTFS refuses, so it cannot be a path on any platform this ships to.

## No dotfile could be saved, and the app pre-filled the name that guaranteed it

The write policy judged the leaf for hiddenness, so `/workspace/.env` was
refused *after* the modal and the overwrite prompt — quoting the name the app
itself had suggested. `.gitignore`, `.dockerignore`, `.eslintrc.json`, `.nvmrc`:
all unsavable, while uploading them worked, so a dotfile could go in and never
come out.

`HostPathUse` gains a third mode. `WriteChosenName` drops the leaf check and
keeps every directory rule, and only `download_container_file` uses it — the
dialog is a real boundary for that caller and only that caller.
`download_container_backup` still takes its path over IPC as a string and keeps
the strict rule.

## Smaller, all found by the reviews

  * A download had no ceiling. `dd` resolves through the container's `PATH`,
    which its agent owns with passwordless sudo; a replacement writing forever
    was measured at ~6 GB/s, so one click on a file listed as 2 KB filled the
    host disk with no progress shown and no cancel. Bounded now by what the
    file measured, with slack that is absolute for small files and
    proportional for large ones, so an honest growing log is unaffected.
  * "Framed rather than verbatim" did not stop container text reaching the
    toast headline: `readableRefusal` matches with `includes`, and it has to,
    because the app's own refusals carry those markers mid-sentence. Anchoring
    would break them. The fix is at the injection point — container text is
    clipped to one 200-character line with control characters stripped — plus a
    `max-h-40` on the toast message, which the `detail` block always had and
    this half did not. 8 KB of prose in a `z-[60]` card pushed its own dismiss
    button off-screen.
  * `savingPath` was a scalar while the design deliberately allows concurrent
    saves. Starting a second freed the first's row mid-transfer, and whichever
    finished first cleared both; dismissing the second dialog was enough. It is
    a `Set` now.
  * `setUploading(false)` fired when the command settled, not when the refresh
    finished, so a second click landed mid-relisting.
  * `upload_files_to_container` checked the container directory before the
    picker and then used the unresolved path. A modal has no time limit. It
    re-checks after, which is what `docker::exec`'s doc comment already claimed.
  * `wait_for_exec_exit` flattened a missing exit code to `Some(0)`, which made
    the new `!= Some(0)` check unreachable by construction.
  * `normalize_host_path` did not collapse repeated separators, so the lexical
    system-root rule was silently absent for `C:\\Windows\…`. Not exploitable —
    the resolved pass catches it — but a documented layer that does nothing is
    a trap for the next caller.
  * README still carried the "no host path crosses IPC in either direction"
    claim the previous commit narrowed everywhere else, and HOW-TO-USE
    described the hidden rule without saying it applies to folders only.

480 Rust tests, 602 frontend, no new clippy warnings. Eleven mutations against
the new tests, all killed — two of the first round survived and were rewritten:
one because `split_whitespace` already handled the case I thought I was
testing, one because I had deleted a comment rather than the behaviour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LHL9ty7arp8FHwvE77ne7y
This commit is contained in:
2026-08-25 10:42:26 -07:00
co-authored by Claude Opus 5
parent 2c9482a67d
commit eead748222
8 changed files with 624 additions and 128 deletions
@@ -68,7 +68,7 @@ export default function FilesTab({ project }: Props) {
uploadFiles,
saveToHost,
uploading,
savingPath,
savingPaths,
} = useFileManager(project.id);
const running = project.status === "running";
@@ -540,7 +540,7 @@ export default function FilesTab({ project }: Props) {
// Only this row: a large file can take a while,
// and there is no reason the rest of the pane
// should go dead while it is written.
disabled={savingPath === entry.path}
disabled={savingPaths.has(entry.path)}
onClick={(e) => {
e.stopPropagation();
void saveToHost(entry);
@@ -553,7 +553,7 @@ export default function FilesTab({ project }: Props) {
// save dialog the backend had just opened.
onDoubleClick={(e) => e.stopPropagation()}
>
{savingPath === entry.path ? "Saving…" : "Save to host…"}
{savingPaths.has(entry.path) ? "Saving…" : "Save to host…"}
</Button>
)}
</>
+10 -1
View File
@@ -47,7 +47,16 @@ function ToastCard({ toast, onDismiss }: { toast: Toast; onDismiss: () => void }
{tone.glyph}
</span>
<div className="flex-1 min-w-0">
<div className="text-[var(--text-primary)] break-words">{toast.message}</div>
{/* Clamped. A toast message is normally a sentence, but some of them
quote text a *container* wrote — and this card is `z-[60]`, above
every modal, with its dismiss button at the top. An unclamped
message of a few kilobytes is a card taller than the viewport whose
✕ has been pushed off-screen, i.e. an unclosable overlay. The
`detail` block below has always had `max-h-40 overflow-auto`; this
half did not. */}
<div className="text-[var(--text-primary)] break-words max-h-40 overflow-y-auto">
{toast.message}
</div>
{toast.detail && (
<>
<button
+69 -31
View File
@@ -471,6 +471,38 @@ describe("useFileManager transfer state", () => {
expect(result.current.uploading).toBe(false);
});
it("stays in flight through the refresh, not just the transfer", async () => {
// Clearing the flag the moment the command settled put the button back
// while the re-listing was still running, so a second click landed
// mid-refresh on a grid that was still showing the old contents.
uploadFilesToContainer.mockResolvedValueOnce({
uploaded: ["/workspace/a.txt"],
failures: [],
});
let finishListing: (v: unknown) => void = () => {};
listContainerFiles.mockReturnValueOnce(
new Promise((resolve) => {
finishListing = resolve;
}),
);
const { result } = renderHook(() => useFileManager("p1"));
let uploading: Promise<void>;
act(() => {
uploading = result.current.uploadFiles();
});
await act(async () => {
await Promise.resolve();
await Promise.resolve();
});
// The transfer is done; the listing it triggered is not.
expect(result.current.uploading).toBe(true);
await act(async () => {
finishListing([file("a.txt")]);
await uploading;
});
expect(result.current.uploading).toBe(false);
});
it("clears the upload flag when the transfer fails", async () => {
// The `catch` returns early, so without a `finally` the button is disabled
// for the rest of the session — the failure mode is a pane that can never
@@ -483,41 +515,47 @@ describe("useFileManager transfer state", () => {
expect(result.current.uploading).toBe(false);
});
it("marks only the row being saved, and clears it on failure", async () => {
let release: (v: unknown) => void = () => {};
downloadContainerFile.mockReturnValueOnce(
new Promise((resolve) => {
release = resolve;
}),
);
const { result } = renderHook(() => useFileManager("p1"));
expect(result.current.savingPath).toBeNull();
let saving: Promise<void>;
act(() => {
saving = result.current.saveToHost(file("a.txt"));
});
// The path, not a boolean — the rest of the pane stays usable.
expect(result.current.savingPath).toBe("/workspace/a.txt");
await act(async () => {
release(10);
await saving;
});
expect(result.current.savingPath).toBeNull();
it("tracks each save separately, so one finishing does not free another", async () => {
// The bug this exists for: `savingPath` was a single string. Starting a
// second save overwrote it, so the first row went live again mid-transfer,
// and whichever save settled first cleared the flag for both — dismissing
// the second dialog was enough. A set is what the design needs, because
// "only the row being saved is disabled" is exactly what makes a second
// save startable.
let releaseBig: (v: unknown) => void = () => {};
let releaseSmall: (v: unknown) => void = () => {};
downloadContainerFile
.mockReturnValueOnce(new Promise((r) => { releaseBig = r; }))
.mockReturnValueOnce(new Promise((r) => { releaseSmall = r; }));
const { result } = renderHook(() => useFileManager("p1"));
let big: Promise<void>;
let small: Promise<void>;
act(() => { big = result.current.saveToHost(file("big.bin")); });
expect(result.current.savingPaths.has("/workspace/big.bin")).toBe(true);
act(() => { small = result.current.saveToHost(file("notes.txt")); });
// Both, at once — a scalar could only hold the second.
expect(result.current.savingPaths.has("/workspace/big.bin")).toBe(true);
expect(result.current.savingPaths.has("/workspace/notes.txt")).toBe(true);
// The second one finishing must not re-enable the first, which is still
// streaming. `null` is the dismissal path, which is how this was cheapest
// to trigger in practice.
await act(async () => { releaseSmall(null); await small; });
expect(result.current.savingPaths.has("/workspace/notes.txt")).toBe(false);
expect(result.current.savingPaths.has("/workspace/big.bin")).toBe(true);
await act(async () => { releaseBig(10); await big; });
expect(result.current.savingPaths.size).toBe(0);
});
it("clears a row's saving flag when its save fails", async () => {
downloadContainerFile.mockRejectedValueOnce("Permission denied");
const { result } = renderHook(() => useFileManager("p1"));
await act(async () => {
await result.current.saveToHost(file("b.txt"));
});
expect(result.current.savingPath).toBeNull();
// And on dismissal, which is the path that actually needs the `finally`:
// the dismissal check `return`s from inside the `try`, so a clear placed
// after the block instead is skipped and the row reads "Saving…" for the
// rest of the session with nothing running behind it.
downloadContainerFile.mockResolvedValueOnce(null);
await act(async () => {
await result.current.saveToHost(file("c.txt"));
});
expect(result.current.savingPath).toBeNull();
expect(result.current.savingPaths.size).toBe(0);
});
});
+44 -26
View File
@@ -52,11 +52,19 @@ export function useFileManager(projectId: string) {
* against the same file, and a multi-gigabyte save is indistinguishable from
* a click that did nothing.
*
* `savingPath` rather than a boolean, so only the row being saved is
* disabled the pane stays usable while a big file is written.
* `savingPaths` is a **set**, not one path. Keeping only the row being
* disabled is what makes the pane usable during a big transfer — and that is
* precisely what makes a *second* save startable, so the state has to be able
* to hold two. As a scalar it could not: starting a save on `notes.txt` while
* `big.bin` was still streaming overwrote it, so `big.bin`'s button went live
* again mid-transfer; and whichever save finished first cleared the flag for
* both. Dismissing the second dialog was enough to do it.
*
* Paths are unique within a listing, so a path is a usable key — `FilesTab`
* relies on the same fact for its row keys.
*/
const [uploading, setUploading] = useState(false);
const [savingPath, setSavingPath] = useState<string | null>(null);
const [savingPaths, setSavingPaths] = useState<ReadonlySet<string>>(new Set());
const currentPathRef = useRef(currentPath);
@@ -184,33 +192,37 @@ export function useFileManager(projectId: string) {
*/
const uploadFiles = useCallback(async () => {
const target = currentPathRef.current;
let outcome;
setUploading(true);
try {
outcome = await commands.uploadFilesToContainer(projectId, target);
} catch (e) {
// A failure *before* the picker: no container, not running, or a
// directory this pane may not write to. One toast, not one per file.
report("Could not upload", e);
return;
} finally {
setUploading(false);
}
if (!outcome) return;
for (const failure of outcome.failures) {
useAppState.getState().pushToast({ kind: "error", message: failure });
}
if (outcome.uploaded.length > 0) {
// The directory is named, not implied. `target` is captured at click
// time and the picker is a modal OS dialog — the user has all the time in
// the world to browse somewhere else while it is open, and the files land
// where they started. "Uploaded 2 files." in front of a grid that does not
// contain them is a worse answer than no message at all.
let outcome;
try {
outcome = await commands.uploadFilesToContainer(projectId, target);
} catch (e) {
// A failure *before* the picker: no container, not running, or a
// directory this pane may not write to. One toast, not one per file.
report("Could not upload", e);
return;
}
if (!outcome) return;
for (const failure of outcome.failures) {
useAppState.getState().pushToast({ kind: "error", message: failure });
}
if (outcome.uploaded.length === 0) return;
// The directory is named, not implied. `target` is captured at click time
// and the picker is a modal OS dialog — the user has all the time in the
// world to browse somewhere else while it is open, and the files land
// where they started. "Uploaded 2 files." in front of a grid that does
// not contain them is a worse answer than no message at all.
const count = outcome.uploaded.length;
setCompleted(
`Uploaded ${count === 1 ? "1 file" : `${count} files`} to ${target}.`,
);
if (currentPathRef.current === target) await navigate(target);
} finally {
// Around the *whole* body, refresh included. Clearing it the moment the
// command settled put the button back before the re-listing had run, so
// a second click landed mid-refresh on a grid that was still the old one.
setUploading(false);
}
}, [projectId, navigate, report]);
@@ -223,7 +235,7 @@ export function useFileManager(projectId: string) {
*/
const saveToHost = useCallback(
async (entry: FileEntry) => {
setSavingPath(entry.path);
setSavingPaths((live) => new Set(live).add(entry.path));
try {
const bytes = await commands.downloadContainerFile(projectId, entry.path);
// `0` is a real answer — an empty file saved is a success — so this
@@ -233,7 +245,13 @@ export function useFileManager(projectId: string) {
} catch (e) {
report(`Could not save "${entry.name}"`, e);
} finally {
setSavingPath(null);
// Remove only this one. A save that finishes while another is still
// streaming must not re-enable the other's row.
setSavingPaths((live) => {
const next = new Set(live);
next.delete(entry.path);
return next;
});
}
},
[projectId, report],
@@ -257,6 +275,6 @@ export function useFileManager(projectId: string) {
saveToHost,
/** A host transfer is in flight — see the state declarations above. */
uploading,
savingPath,
savingPaths,
};
}