Give the Files tab back its uploads and downloads
Build App (Preview) / compute-version (pull_request) Successful in 4s
Build Container / build-container (pull_request) Successful in 1m35s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m38s
Build App (Preview) / build-windows (pull_request) Successful in 5m51s
Build App (Preview) / build-linux (pull_request) Successful in 6m50s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
Build App (Preview) / compute-version (pull_request) Successful in 4s
Build Container / build-container (pull_request) Successful in 1m35s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m38s
Build App (Preview) / build-windows (pull_request) Successful in 5m51s
Build App (Preview) / build-linux (pull_request) Successful in 6m50s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
`upload_file_to_container` and `download_container_file` existed on main before
any of this work started. "Ship the Files tab container-side only" removed them
and called it narrowing scope; from a user's side it was a regression they
upgraded into. This restores the feature.
The reason for the removal was real — four consecutive audits found their
criticals in host paths crossing IPC — so the feature comes back only in the
shape that removes the class rather than patching it a fifth time. The dialogs
are opened by **Rust** (`pick_save_path`, `pick_files_to_upload`), not by the
webview. A frontend `open()`/`save()` handing the backend a path string is
exactly what failed, and the backend cannot tell such a string from one a
compromised webview invented. Now the webview can ask for a picker and that is
the whole of its influence: it cannot name a host path as an input. That is the
shape the previous round's own notes named as the honest one if this ever
returned.
None of the machinery the audits condemned returns. No `link(2)` destination
reservation, no placeholder rollback, no collision marker: the OS save dialog
already asks about overwriting and Docker's extractor overwrites on upload the
way `cp` does, so there was nothing left for it to do. Download reuses the
sequence `download_container_backup` has been using unchanged — resolve, stream
into a partial file beside the destination, rename last — so a failed transfer
never touches the file that was already there. Upload reuses the terminal
drop's hardened uploader, with the container's uid/gid resolved once per
selection rather than once per file.
Against a container that is actively hostile rather than merely surprising:
* the read is `dd iflag=nonblock`, not `cat`. `[ -f ]` and the `open` after it
are two syscalls and the container owns the filesystem in between; a loop
swapping the file for a FIFO wins that race, and `cat` then blocks forever
with no writer and no timeout anywhere on the path — the `invoke` never
settles and a partial is left in the user's directory for good. Verified in
a real container that `cat` hangs, that `iflag=nonblock` returns, and that
it is byte-identical on a regular file.
* the read is bracketed by a second `[ -f ]`, because non-blocking turns that
hang into an empty file that would otherwise be renamed over the
destination and reported as a successful save.
* an *undeterminable* exit code is a failure. Backup catches this class with
its `total == 0` check, which download cannot have because an empty file is
a legitimate save; without a replacement, a project restarted mid-download
renames a truncated partial over the user's file and reports the byte count
as if it were whole.
* container stderr is capped. Every other reader of container output in the
tree is capped for this reason; the two streaming commands were the
exception, and stdout was bounded by disk while stderr was bounded by
nothing.
* the script's refusals are framed rather than used verbatim, so a directory
named to look like one of our own sentences cannot become the toast
headline through `readableRefusal`.
* the partial name is capped at NAME_MAX. A bundler's 230-character content
hash is a name that fits its directory and produces a partial name that
does not.
Also: a non-UTF-8 dialog path is refused by name rather than silently mangled
into a different path by U+FFFD substitution; both actions carry in-flight
state, so a second click cannot open a second dialog and a slow save is not
indistinguishable from a dead button; and the upload's completion message names
the directory, since the picker is modal and the user can browse elsewhere
while it is open.
Not restored: drag-and-drop, in either direction. `drag:allow-start-drag` stays
ungranted and `hold/disk-and-dragout` still holds that work.
Two bugs the new tests caught while being written: a double-click on "Save to
host…" opened the file viewer on top of the save dialog, and an N-file upload
made N redundant execs to re-ask `id -u`.
Docs that asserted this feature did not and must not exist are corrected —
CLAUDE.md, README, HOW-TO-USE, TECHNICAL and the capability threat model. The
"no host path crosses IPC" claim is deliberately narrowed to the inbound
direction: paths do still travel outward inside error text, canonical ones
included, and the reviewed record should not overstate.
600 frontend tests, 473 Rust, no new clippy warnings. Every new test was
mutation-checked; four that survived their first mutation were rewritten,
including two whose mutations turned out to be unfaithful and one that was
blind to a dismissal leaving a row stuck on "Saving…".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LHL9ty7arp8FHwvE77ne7y
This commit is contained in:
@@ -6,12 +6,16 @@ import type { FileEntry } from "../lib/types";
|
||||
const listContainerFiles = vi.fn();
|
||||
const renameContainerPath = vi.fn();
|
||||
const createContainerDirectory = vi.fn();
|
||||
const uploadFilesToContainer = vi.fn();
|
||||
const downloadContainerFile = vi.fn();
|
||||
|
||||
vi.mock("../lib/tauri-commands", () => ({
|
||||
listContainerFiles: (p: string, path: string) => listContainerFiles(p, path),
|
||||
renameContainerPath: (p: string, f: string, t: string) => renameContainerPath(p, f, t),
|
||||
createContainerDirectory: (p: string, parent: string, n: string) =>
|
||||
createContainerDirectory(p, parent, n),
|
||||
uploadFilesToContainer: (p: string, dir: string) => uploadFilesToContainer(p, dir),
|
||||
downloadContainerFile: (p: string, path: string) => downloadContainerFile(p, path),
|
||||
readContainerFile: vi.fn(),
|
||||
}));
|
||||
|
||||
@@ -45,6 +49,8 @@ const file = (name: string, extra: Partial<FileEntry> = {}): FileEntry => ({
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
listContainerFiles.mockResolvedValue([file("a.txt")]);
|
||||
uploadFilesToContainer.mockResolvedValue({ uploaded: [], failures: [] });
|
||||
downloadContainerFile.mockResolvedValue(0);
|
||||
});
|
||||
|
||||
describe("useFileManager navigation", () => {
|
||||
@@ -292,3 +298,226 @@ describe("useFileManager surfaces written refusals as prose", () => {
|
||||
expect(lastToast().detail).toBe("no space left on device");
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Both of these actions are *dialog-driven from Rust* — the hook passes a
|
||||
* project and a directory and gets back an answer, and there is deliberately no
|
||||
* host path anywhere in this file. What is worth pinning is the vocabulary of
|
||||
* that answer, because two of its values look like failure and are not: `null`
|
||||
* means the user dismissed the picker, and `0` bytes means an empty file was
|
||||
* saved successfully.
|
||||
*/
|
||||
describe("useFileManager saving to the host", () => {
|
||||
it("treats a zero-byte save as a success", async () => {
|
||||
// The bug this exists for: `if (!bytes) return` reads a genuine
|
||||
// zero-length file — an empty `.gitkeep`, a truncated log — as a
|
||||
// dismissal, so the file lands on the host and the app says nothing at
|
||||
// all. The sentinel is `null`, and only `null`.
|
||||
downloadContainerFile.mockResolvedValueOnce(0);
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.saveToHost(file("empty.txt"));
|
||||
});
|
||||
expect(result.current.completed).toContain("empty.txt");
|
||||
expect(pushToast).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("says nothing at all when the dialog is dismissed", async () => {
|
||||
downloadContainerFile.mockResolvedValueOnce(null);
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.saveToHost(file("a.txt"));
|
||||
});
|
||||
expect(result.current.completed).toBeNull();
|
||||
expect(pushToast).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("names the file in a refusal", async () => {
|
||||
downloadContainerFile.mockRejectedValueOnce("/etc/shadow is not readable");
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.saveToHost(file("secret.txt"));
|
||||
});
|
||||
expect(toastText()).toContain("secret.txt");
|
||||
});
|
||||
});
|
||||
|
||||
describe("useFileManager uploading from the host", () => {
|
||||
it("uploads into the directory on screen and shows the result", async () => {
|
||||
uploadFilesToContainer.mockResolvedValueOnce({
|
||||
uploaded: ["/workspace/app/one.txt", "/workspace/app/two.txt"],
|
||||
failures: [],
|
||||
});
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.navigate("/workspace/app");
|
||||
});
|
||||
listContainerFiles.mockClear();
|
||||
await act(async () => {
|
||||
await result.current.uploadFiles();
|
||||
});
|
||||
expect(uploadFilesToContainer).toHaveBeenCalledWith("p1", "/workspace/app");
|
||||
expect(result.current.completed).toContain("2 files");
|
||||
// The directory is named. `target` is captured at click time and the
|
||||
// picker is a modal dialog, so "Uploaded 2 files." on its own can be shown
|
||||
// in front of a grid those files are not in.
|
||||
expect(result.current.completed).toContain("/workspace/app");
|
||||
// The new files are only on screen if the listing was asked for again.
|
||||
expect(listContainerFiles).toHaveBeenCalledWith("p1", "/workspace/app");
|
||||
});
|
||||
|
||||
it("reports every file that failed, not just a count", async () => {
|
||||
// "3 of 5 uploaded" without naming the two is not a report — the user
|
||||
// cannot tell which ones to retry, or why.
|
||||
uploadFilesToContainer.mockResolvedValueOnce({
|
||||
uploaded: ["/workspace/ok.txt"],
|
||||
failures: [
|
||||
"/home/j/Pictures is a folder — upload its files individually.",
|
||||
"/home/j/vm.img is too large to upload (900 MB; limit 256 MB).",
|
||||
],
|
||||
});
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.uploadFiles();
|
||||
});
|
||||
expect(pushToast).toHaveBeenCalledTimes(2);
|
||||
expect(toastText()).toContain("is a folder");
|
||||
expect(toastText()).toContain("too large");
|
||||
// A partial batch still succeeded partially, and the pane must show it.
|
||||
expect(result.current.completed).toContain("1 file");
|
||||
});
|
||||
|
||||
it("does not refresh when the picker was dismissed", async () => {
|
||||
uploadFilesToContainer.mockResolvedValueOnce(null);
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.navigate("/workspace/app");
|
||||
});
|
||||
listContainerFiles.mockClear();
|
||||
await act(async () => {
|
||||
await result.current.uploadFiles();
|
||||
});
|
||||
expect(listContainerFiles).not.toHaveBeenCalled();
|
||||
expect(pushToast).not.toHaveBeenCalled();
|
||||
expect(result.current.completed).toBeNull();
|
||||
});
|
||||
|
||||
it("reports a refusal that happened before the picker once, not per file", async () => {
|
||||
// No container, not running, or a directory this pane may not write to.
|
||||
// There is no selection yet, so there is nothing to enumerate.
|
||||
uploadFilesToContainer.mockRejectedValueOnce(
|
||||
"Start the project before uploading files — it runs inside the running container.",
|
||||
);
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.uploadFiles();
|
||||
});
|
||||
expect(pushToast).toHaveBeenCalledTimes(1);
|
||||
expect(toastText()).toContain("Start the project");
|
||||
});
|
||||
|
||||
it("does not drag the pane back when the user navigated during the upload", async () => {
|
||||
// The same rule rename and new-folder follow: a slow operation must not
|
||||
// relist a directory the user has already left.
|
||||
let release: (v: unknown) => void = () => {};
|
||||
uploadFilesToContainer.mockReturnValueOnce(
|
||||
new Promise((resolve) => {
|
||||
release = resolve;
|
||||
}),
|
||||
);
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.navigate("/workspace/app");
|
||||
});
|
||||
let uploading: Promise<void>;
|
||||
act(() => {
|
||||
uploading = result.current.uploadFiles();
|
||||
});
|
||||
await act(async () => {
|
||||
await result.current.navigate("/workspace/other");
|
||||
});
|
||||
listContainerFiles.mockClear();
|
||||
await act(async () => {
|
||||
release({ uploaded: ["/workspace/app/one.txt"], failures: [] });
|
||||
await uploading;
|
||||
});
|
||||
expect(listContainerFiles).not.toHaveBeenCalled();
|
||||
expect(result.current.currentPath).toBe("/workspace/other");
|
||||
});
|
||||
});
|
||||
|
||||
describe("useFileManager transfer state", () => {
|
||||
it("marks an upload in flight for as long as it runs", async () => {
|
||||
// Without this the button stays live: a second click opens a second OS
|
||||
// dialog and runs a second concurrent exec, and a slow transfer looks
|
||||
// exactly like a click that did nothing.
|
||||
let release: (v: unknown) => void = () => {};
|
||||
uploadFilesToContainer.mockReturnValueOnce(
|
||||
new Promise((resolve) => {
|
||||
release = resolve;
|
||||
}),
|
||||
);
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
expect(result.current.uploading).toBe(false);
|
||||
let uploading: Promise<void>;
|
||||
act(() => {
|
||||
uploading = result.current.uploadFiles();
|
||||
});
|
||||
expect(result.current.uploading).toBe(true);
|
||||
await act(async () => {
|
||||
release({ uploaded: [], failures: [] });
|
||||
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
|
||||
// upload again, with no error left on screen to explain it.
|
||||
uploadFilesToContainer.mockRejectedValueOnce("Start the project first");
|
||||
const { result } = renderHook(() => useFileManager("p1"));
|
||||
await act(async () => {
|
||||
await result.current.uploadFiles();
|
||||
});
|
||||
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();
|
||||
|
||||
downloadContainerFile.mockRejectedValueOnce("Permission denied");
|
||||
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();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user