diff --git a/docs/04_filesystem_interface.md b/docs/04_filesystem_interface.md index 1e40b11c..464d043d 100644 --- a/docs/04_filesystem_interface.md +++ b/docs/04_filesystem_interface.md @@ -282,7 +282,7 @@ so handlers from Node code port over directly. | `EPERM` | Operation is forbidden, e.g. deleting the workspace root. | | `EIO` | Backing storage failed unexpectedly. | | `EACCES` | *Reserved for future mount layer (see [06. Mount Interface](./06_mount_interface.md)).* No code path in `workspace-fs` currently throws it. | -| `EROFS` | *Reserved for future mount layer (see [06. Mount Interface](./06_mount_interface.md)).* No code path in `workspace-fs` currently throws it. | +| `EROFS` | The write was refused. Either the path is under a read-only mount root (see [06. Mount Interface](./06_mount_interface.md)), or the filesystem handle has no write access because the command holding it is running read-only (see [20. Write access and approval](./20_approval.md)). | ### Example: handle "file missing" and bubble everything else diff --git a/docs/05_runtime_interface.md b/docs/05_runtime_interface.md index 64a57485..344510d2 100644 --- a/docs/05_runtime_interface.md +++ b/docs/05_runtime_interface.md @@ -33,6 +33,7 @@ interface WorkspaceRuntimeExecOptions { timeoutMs?: number; env?: Record; stdin?: Uint8Array | string; + writable?: boolean; } interface WorkspaceRuntimeExecHandle extends ReadableStream { @@ -88,6 +89,10 @@ await workspace.runtime.exec( Omitting `backend` selects the first configured backend. Backend selection is routing, not authorization; public gateways must validate it against server-side policy. +## Write access + +`writable` defaults to true. Pass `false` for a command expected only to read, and a write it attempts fails rather than lands. What that costs the command depends on the backend: `worker-shell` and `worker-javascript` share the host store and refuse the write where it happens, while a Container writes to its own copy and has the change refused on the way back, reported in `skipped` with reason `no-write-access`. A configured gate can also withdraw write access from a command that asked for it. See [20. Write access and approval](./20_approval.md). + ## Command synchronization Command backends continue to use the existing synchronization bracket: diff --git a/docs/09_tool_interface.md b/docs/09_tool_interface.md index eeec7ca2..24364f8a 100644 --- a/docs/09_tool_interface.md +++ b/docs/09_tool_interface.md @@ -204,6 +204,7 @@ createExecTool({ backends, defaultBackend, maxBytes?, + writable?, }); ``` @@ -212,6 +213,7 @@ createExecTool({ | `backends` | required | Map of backend id to a model-facing description. | | `defaultBackend` | required | Backend used when the model omits `backend`. Must be a key in `backends`. | | `maxBytes` | 64 KiB | UTF-8 byte cap for each of stdout and stderr. | +| `writable` | every command may write | `({ command, cwd, backend }) => boolean`. Decides whether a command may modify the workspace. | Schema: @@ -223,22 +225,25 @@ Schema: } ``` -Calls `workspace.runtime.exec(command, { cwd, encoding: "utf8", backend })`, waits for `result()`, and returns: +Calls `workspace.runtime.exec(command, { cwd, encoding: "utf8", backend, writable })`, waits for `result()`, and returns: ```ts { command: string; cwd: string | null; backend: string; + writable: boolean; exitCode: number; stdout: string; stderr: string; } ``` +`writable` is resolved by the host, not by the model, and is deliberately absent from the schema. The case it defends against is the command mislabelled as read-only, so a label the model supplies would agree with the mistake. It is reported back on the result so the model can tell a refused write from a broken command instead of retrying the same thing. A gate refusal arrives as an `error` field rather than a thrown error, so the agent loop survives it. See [20. Write access and approval](./20_approval.md). + `exec` is opt-in. `createAITools()` includes it only when the caller passes `shell` options and `readonly` is not true. The backend descriptions are included in the tool description so the model can choose the cheapest backend that can run the command. -Wire this tool up carefully: it executes arbitrary shell commands inside the configured backend. Use `readonly: true` for inspection-only agents, or omit `shell` when command execution is not part of the agent's job. +Wire this tool up carefully: it executes arbitrary shell commands inside the configured backend. Use `readonly: true` for inspection-only agents, or omit `shell` when command execution is not part of the agent's job. A `writable` resolver narrows what a command can do but is not a substitute for either: it stops a command classified read-only from writing, and does nothing about one classified writable. ## `publish` diff --git a/docs/20_approval.md b/docs/20_approval.md new file mode 100644 index 00000000..b7e9fab4 --- /dev/null +++ b/docs/20_approval.md @@ -0,0 +1,218 @@ +# 20. Write access and approval + +Two related things: a per-command write capability, and a pair of hooks +for deciding and recording what a workspace is asked to do. + +## The problem + +An agent decides a command is read-only and runs it. Sometimes it is +wrong — an alias, a shell function, a `&&` it did not parse, a script +that writes a lockfile on the way to printing a version. The workspace +is modified, nothing reports it, and the mistake is found later by +whatever breaks next. + +Classifying commands correctly is not solvable. What is solvable is +making the classification safe to get wrong: a command believed to be +read-only runs without write access, so if the belief was wrong the +write fails instead of landing. + +## Per-command write access + +`writable` on an exec call. Defaults to true. + +```ts +const handle = await workspace.runtime.exec("git log --oneline", { + writable: false, +}); +``` + +The capability is per command, not per write. `rm -rf` issues one call +per entry; denying partway through leaves a half-deleted tree. A +command is the smallest unit that can be refused and leave the +workspace in a state the caller can reason about. + +It travels with the command rather than being read from configuration, +because commands overlap. A read-only command running beside a writable +one must not be able to disarm it, and must not be able to borrow its +access. + +### What "no write access" enforces, per backend + +Not the same thing everywhere, and the difference matters when +choosing a backend for work you intend to constrain. + +| Backend | Enforcement | Effect of a write | +|---|---|---| +| `worker-shell` | Preventive | Fails inside the command with `EROFS`. Nothing is written. | +| `worker-javascript` | Preventive | The capability handed to the module is narrowed, and the guest shim throws the refusal inside the module. | +| `container-shell` | After the fact | Lands in the container's own copy, then is refused on the way back and reported in `skipped`. | + +`worker-shell` shares the host store, so a command that runs there +holds a filesystem handle built without the capability, and the first +write fails where it happens. The command sees an ordinary filesystem +error and reports it like any other. + +For `worker-javascript` the per-execution flag is intersected with the +backend's own `access` option rather than replacing it. A backend +registered `access: "read"` stays read-only however the call was made, +and an execution asking to be read-only gets that on a read-write +backend. Neither side widens the other, which is the same rule the gate +follows. + +A container has its own copy of the files. By the time the host hears +about a change the container has already written it, so there is +nothing left to prevent — only to refuse. The changes are dropped on +arrival and listed in `skipped` with reason `no-write-access`: + +```ts +const result = await handle.result(); +for (const entry of result.skipped) { + if (entry.reason === "no-write-access") { + // The command wrote this. It was not applied. + } +} +``` + +Refusing is discarding, not deferring. A refused change is not +redelivered to the next pull that does have write access, or the +refusal would only be a delay. The consequence is that the container's +copy and the workspace disagree from that point on: the container still +holds the file it wrote. Treat a refused write there as a reason to +discard the container rather than keep using it. + +## The gate + +Consulted before an action, and able to refuse it. + +```ts +new Workspace({ + storage: ctx.storage, + gate: { + async check(action) { + if (action.kind !== "shell.exec") return { allow: true }; + if (isDestructive(action.command)) { + return { allow: false, reason: "destructive commands need review" }; + } + return { allow: true }; + }, + }, +}); +``` + +A refusal throws `ActionDeniedError`, which carries the action and the +reason. Nothing runs. + +A gate may also allow an action with write access withdrawn, which is +the answer for a command a policy will run but not trust: + +```ts +return { allow: true, writable: false }; +``` + +Narrowing only. A gate cannot grant access an action did not ask for, +so a read-only exec stays read-only regardless of what the gate +returns. + +`check` may be async, and the action does not start until it settles — +long enough to consult a policy service or wait for a human. Whatever +it waits on holds up the caller. + +A gate that throws propagates. A gate that could not reach a decision +is not a gate that said no, and code that cannot tell those apart will +eventually treat an outage as permission. + +### What is gated + +`shell.exec`, once per command, and the mutating methods on +`Workspace.fs` — `writeFile`, `mkdir`, `rm`, `chmod`, `symlink` — once +per call. Reads are not gated. + +The filesystem half is there because `Workspace.fs` writes to the store +without crossing the wire. A gate covering only `shell.exec` would have +an obvious way around it: deny the command, write the file directly. + +Filesystem calls are gated individually because each call is the whole +action, so refusing one leaves nothing half-finished. That is the +difference from a command, and the reason a command is gated once. + +A gate should return `{ allow: true }` for action kinds it does not +recognise, so kinds added later stay permitted rather than being +refused by a gate that was never asked about them. + +### Asking a human + +Ask before the command starts. Once it is running there is nowhere to +suspend it that does not risk a partial result, and a write-by-write +prompt would ask hundreds of times for one `rm -rf`. + +## The audit hook + +Notified after an action has been decided, and after it has run. + +```ts +new Workspace({ + storage: ctx.storage, + audit: { + record(action, outcome) { + log({ kind: action.kind, status: outcome.status }); + }, + }, +}); +``` + +`outcome.status` is `allowed`, `denied`, or `failed`. Refused actions +are reported too, and are usually the more interesting half. + +It cannot deny anything, and errors it throws are swallowed. By the +time it runs the action has already happened; failing the caller over a +failed log entry would make an audit hook into a gate. + +For `shell.exec` it fires on the spawn, not on the exit. `exec` returns +a detached handle the caller may never drain, so there is no later +moment guaranteed to arrive, and picking one would mean a command that +is dropped is never recorded at all. What the command went on to do is +on the observer's span and on the result. + +## Why this is not the observer + +The observer in +[11. Lifecycle](./11_lifecycle.md) must return its callback's result +unchanged — observability that changes behaviour is a bug. A gate +exists to change behaviour. They are separate seams with the same shape +and opposite licences, rather than one seam with a weakened contract. + +## Tool layer + +`createExecTool` takes a `writable` resolver. It is not part of the +tool's input schema, so the model cannot set it: + +```ts +createExecTool({ + workspace, + backends, + defaultBackend: "worker-shell", + writable: ({ command }) => !readOnlyCommand(command), +}); +``` + +The model must not classify its own command. The case being defended +against is the command mislabelled as read-only, and asking the model +that mislabelled it to declare the label produces a flag that agrees +with the mistake. The host decides from something it already trusts, +and the model finds out by the write failing. + +The effective access is reported on the tool result, so a model can +tell a refused write from a broken command instead of retrying the same +thing. A gate refusal comes back as a tool result too, not a thrown +error, so the agent loop survives it. + +## Related + +- [04. Filesystem interface](./04_filesystem_interface.md) — `EROFS` + and the filesystem surface. +- [05. Runtime interface](./05_runtime_interface.md) — exec options and + the synchronization bracket. +- [06. Mount interface](./06_mount_interface.md) — read-only mounts, + which are a fixed property of a path rather than a per-command + decision. Both apply, and neither is a way around the other. +- [09. Tool interface](./09_tool_interface.md) — the agent-facing tools. diff --git a/docs/README.md b/docs/README.md index ffdf2c5b..776117e0 100644 --- a/docs/README.md +++ b/docs/README.md @@ -243,6 +243,7 @@ above, then dive into the area you're working on. | [17. Isolate JavaScript runtime](./17_isolate_javascript.md) | ECMAScript modules, durable imports, configured libraries, durable `node:fs/promises`, trusted `ws:git` / `ws:artifacts`, and managed lifecycle. | | [18. Runtime migration](./18_runtime_migration.md) | Breaking preview-API mappings from public shell and script-execution surfaces to `workspace.runtime`. | | [19. Performance](./19_performance.md) | Filesystem benchmarks: `fs-bench` numbers, an `npm install` comparison, and how to reproduce them. | +| [20. Write access and approval](./20_approval.md) | Per-command write access, the gate consulted before an action, and the audit hook notified after it. | ## High-level API diff --git a/packages/computer/README.md b/packages/computer/README.md index 1075d5f7..278b074c 100644 --- a/packages/computer/README.md +++ b/packages/computer/README.md @@ -470,6 +470,44 @@ default is a zero-cost no-op, so there's no overhead unless you opt in. An adapter for the Cloudflare runtime lives at `@cloudflare/computer/observe/cloudflare`. +### Write access and approval + +Pass `writable: false` to an exec call for a command you expect to only +read. A write it attempts then fails instead of landing, which is what +makes a wrong guess about a command safe: + +```ts +await workspace.runtime.exec("git log --oneline", { writable: false }); +``` + +`worker-shell` and `worker-javascript` share the host store, so the +write fails inside the command. A container has its own copy and writes +there first, so the change is refused on the way back and reported in +`result.skipped` with reason `no-write-access`. + +Pass a `gate` to be consulted before each command and each mutating +`workspace.fs` call, with the option to refuse it or to withdraw its +write access, and an `audit` hook to be told what was decided: + +```ts +new Workspace({ + storage: ctx.storage, + gate: { + check(action) { + if (action.kind === "shell.exec" && isDestructive(action.command)) { + return { allow: false, reason: "needs review" }; + } + return { allow: true }; + }, + }, + audit: { record: (action, outcome) => log(action, outcome) }, +}); +``` + +Both default to no-ops. They are separate from `observer` because an +observer must not change what happens and a gate exists to. See +[20. Write access and approval](../../docs/20_approval.md). + ## Examples - [`examples/worker-shell`](../../examples/worker-shell) — the diff --git a/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts b/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts index 686070be..037929f8 100644 --- a/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts +++ b/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts @@ -516,6 +516,90 @@ describe("WorkerJavaScriptBackend", () => { expect(events.at(-1)).toMatchObject({ name: "exit", value: 0 }); }); + it("refuses a host write when the execution has no write access", async () => { + // The module backend shares the host store, so the refusal + // happens where the write is attempted rather than afterwards. + const db = new Database(new SQLiteTestStorage()); + initializeSchema(db, () => 0); + const fs = new WorkspaceFilesystem(db); + await fs.mkdir("/workspace", { recursive: true }); + const refusal: string[] = []; + const backend = new WorkerJavaScriptBackend({ loader: writingLoader(refusal) }); + const handle = await backend.connect({ + db, + fs, + git: undefined as never, + artifacts: undefined as never, + }); + + const execution = await handle.exec({ + id: "read-only-write", + source: "export default 1", + writable: false, + }); + for await (const _event of execution.events) { + // drain + } + + await expect(fs.stat("/workspace/output.txt")).rejects.toThrow(); + // The bridge answers a refused capability call with an error + // payload; the generated guest shim is what turns that into a + // thrown error inside the module. This asserts the payload, since + // the fake module here stands in for the shim. + expect(refusal).toHaveLength(1); + expect(JSON.parse(refusal[0]).error.message).toMatch(/write access is not available/); + }); + + it("allows the same host write when the execution has write access", async () => { + // The control: without it the test above would pass against a + // module that never managed to write in the first place. + const db = new Database(new SQLiteTestStorage()); + initializeSchema(db, () => 0); + const fs = new WorkspaceFilesystem(db); + await fs.mkdir("/workspace", { recursive: true }); + const backend = new WorkerJavaScriptBackend({ loader: writingLoader() }); + const handle = await backend.connect({ + db, + fs, + git: undefined as never, + artifacts: undefined as never, + }); + + const execution = await handle.exec({ id: "writable-write", source: "export default 1" }); + for await (const _event of execution.events) { + // drain + } + + expect(await fs.readFile("/workspace/output.txt", "utf8")).toBe("done"); + }); + + it("keeps a read-only backend read-only when an execution asks to write", async () => { + // Intersection, not replacement. A backend registered read-only + // cannot be talked into write access by the call. + const db = new Database(new SQLiteTestStorage()); + initializeSchema(db, () => 0); + const fs = new WorkspaceFilesystem(db); + await fs.mkdir("/workspace", { recursive: true }); + const backend = new WorkerJavaScriptBackend({ loader: writingLoader(), access: "read" }); + const handle = await backend.connect({ + db, + fs, + git: undefined as never, + artifacts: undefined as never, + }); + + const execution = await handle.exec({ + id: "read-backend", + source: "export default 1", + writable: true, + }); + for await (const _event of execution.events) { + // drain + } + + await expect(fs.stat("/workspace/output.txt")).rejects.toThrow(); + }); + it("streams stdout before user code returns", async () => { const db = new Database(new SQLiteTestStorage()); initializeSchema(db, () => 0); @@ -992,3 +1076,33 @@ describe("WorkerJavaScriptBackend", () => { expect(load).not.toHaveBeenCalled(); }); }); + +// A loader whose module asks the host to write one file. Used by the +// write-access tests: whether the file exists afterwards is the whole +// assertion. `payloads` collects the raw bridge responses so a test can +// check the refusal the guest shim would have thrown on. +function writingLoader(payloads: string[] = []) { + return { + load() { + return { + getEntrypoint() { + return { + async evaluate( + _input: unknown, + host: { + call(name: string, args: string): Promise; + assertResult(value: unknown): Promise; + attachOutput(readable: ReadableStream): Promise; + }, + ) { + payloads.push( + await host.call("fs.writeFile", JSON.stringify(["/workspace/output.txt", "done"])), + ); + return evaluateResult(host, 1); + }, + }; + }, + }; + }, + }; +} diff --git a/packages/computer/src/backends/worker-javascript/worker-javascript.ts b/packages/computer/src/backends/worker-javascript/worker-javascript.ts index d0bdf43a..4e753b9b 100644 --- a/packages/computer/src/backends/worker-javascript/worker-javascript.ts +++ b/packages/computer/src/backends/worker-javascript/worker-javascript.ts @@ -333,10 +333,16 @@ class JavaScriptBackendHandle implements WorkspaceModuleBackendHandle { this.#pendingIds.add(id); let admittedRecord: ExecutionRecord | undefined; try { + // Intersection, not replacement: a backend configured read-only + // stays read-only however this execution was called, and an + // execution asking to be read-only gets that even on a + // read-write backend. Neither side can widen the other. + const access: WorkspaceRuntimeAccess = + input.writable === false ? "read" : this.#options.access; const capability = new WorkspaceRuntimeCapability( this.#host.fs, this.#options.root, - this.#options.access, + access, this.#options.maxCapabilityBytes, this.#options.maxDirectoryEntries, ); diff --git a/packages/computer/src/backends/worker-shell/entrypoint.test.ts b/packages/computer/src/backends/worker-shell/entrypoint.test.ts index c1fcd049..faca3625 100644 --- a/packages/computer/src/backends/worker-shell/entrypoint.test.ts +++ b/packages/computer/src/backends/worker-shell/entrypoint.test.ts @@ -81,7 +81,7 @@ interface FakeWorkspace { interface FakeEnv { HOST: { - getWorkspace(): Promise; + getWorkspace(options?: { writable?: boolean }): Promise; }; } @@ -272,6 +272,163 @@ describe("ShellWorker", () => { expect(await response.text()).toMatch(/Workers RPC/); }); + // --------------------------------------------------------------- + // write access + // --------------------------------------------------------------- + // + // Real just-bash against a real Workspace, because the claim being + // tested is about what a real command does to a real store. The env + // here calls workspace.stub(options), which is what production does + // over RPC, so the read-only path under test is the shipped one and + // not a flag this test set by hand. + describe("write access", () => { + let workspace: Workspace; + const stubs: WorkspaceStub[] = []; + + beforeEach(async () => { + workspace = new Workspace({ + storage: new SQLiteTestStorage() as never, + backends: [noopBackend()], + git: createGitClient(), + }); + await workspace.ready(); + await workspace.fs.mkdir("/workspace", { recursive: true }); + }); + + afterEach(async () => { + for (const stub of stubs.splice(0)) stub[Symbol.dispose](); + await workspace.close(); + }); + + function envForWorkspace(): FakeEnv { + return { + HOST: { + async getWorkspace(options?: { writable?: boolean }) { + const stub = workspace.stub(options); + stubs.push(stub); + return stub as unknown as FakeWorkspace; + }, + }, + }; + } + + async function run(command: string, writable?: boolean) { + const worker = new ShellWorker(undefined as never, envForWorkspace() as never); + const envelope = await worker.exec({ + command, + cwd: "/workspace", + id: `run-${Math.random().toString(36).slice(2)}`, + writable, + }); + const events = (await drain(envelope.events)) as { name: string; value: string | number }[]; + return { + stdout: events + .filter((e) => e.name === "stdout") + .map((e) => String(e.value)) + .join(""), + stderr: events + .filter((e) => e.name === "stderr") + .map((e) => String(e.value)) + .join(""), + exitCode: events.find((e) => e.name === "exit")?.value, + }; + } + + it("does not delete the workspace when a read-only command tries to", async () => { + // The case the whole feature exists for. `find -delete` is one + // command issuing a delete per entry, so a per-write refusal + // would stop partway and leave a half-deleted tree. Refusing + // the capability up front means the first delete fails and + // every file is still there. + await workspace.fs.writeFile("/workspace/keep.txt", "keep"); + await workspace.fs.mkdir("/workspace/dir", { recursive: true }); + await workspace.fs.writeFile("/workspace/dir/nested.txt", "nested"); + + const result = await run("find /workspace -mindepth 1 -delete", false); + + expect(result.exitCode).not.toBe(0); + expect(await workspace.fs.readFile("/workspace/keep.txt", "utf8")).toBe("keep"); + expect(await workspace.fs.readFile("/workspace/dir/nested.txt", "utf8")).toBe("nested"); + }); + + it("deletes the workspace when the same command is allowed to", async () => { + // The control. Without it the test above would pass just as + // well against a command that never worked. + await workspace.fs.writeFile("/workspace/keep.txt", "keep"); + + const result = await run("find /workspace -mindepth 1 -delete", true); + + expect(result.exitCode).toBe(0); + await expect(workspace.fs.readFile("/workspace/keep.txt", "utf8")).rejects.toThrow(); + }); + + it("fails a redirect into a new file and creates nothing", async () => { + const result = await run("echo written > /workspace/new.txt", false); + + expect(result.exitCode).not.toBe(0); + await expect(workspace.fs.stat("/workspace/new.txt")).rejects.toThrow(); + }); + + it("leaves an existing file untouched when a read-only command overwrites it", async () => { + await workspace.fs.writeFile("/workspace/existing.txt", "original"); + + const result = await run("echo replaced > /workspace/existing.txt", false); + + expect(result.exitCode).not.toBe(0); + expect(await workspace.fs.readFile("/workspace/existing.txt", "utf8")).toBe("original"); + }); + + it("still reads while refusing to write", async () => { + // Read-only has to mean read-only, not broken. A command that + // only reads must behave exactly as it would with access. + await workspace.fs.writeFile("/workspace/readable.txt", "contents\n"); + + const result = await run("cat /workspace/readable.txt", false); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("contents"); + }); + + it("reports the refusal to the command rather than failing silently", async () => { + // The point of failing the write inside the command: the + // command sees an error it can act on, and a caller reading + // stderr can tell what happened. + await workspace.fs.writeFile("/workspace/existing.txt", "original"); + + const result = await run("echo replaced > /workspace/existing.txt", false); + + // Pinned to the reason, not just to non-empty output: a + // non-zero exit and some stderr would also be produced by the + // stub being broken, which would make the tests above pass for + // the wrong reason. + expect(result.stderr).toMatch(/read-only|EROFS/i); + }); + + it("defaults to writable when the caller does not say", async () => { + const result = await run("echo written > /workspace/default.txt"); + + expect(result.exitCode).toBe(0); + expect(await workspace.fs.readFile("/workspace/default.txt", "utf8")).toBe("written\n"); + }); + + it("keeps concurrent commands' access independent", async () => { + // Two commands overlap in production, and the capability is + // per-handle precisely so a read-only one cannot disarm a + // writable one running beside it. + await workspace.fs.writeFile("/workspace/shared.txt", "original"); + + const [readOnly, writable] = await Promise.all([ + run("echo denied > /workspace/blocked.txt", false), + run("echo allowed > /workspace/allowed.txt", true), + ]); + + expect(readOnly.exitCode).not.toBe(0); + expect(writable.exitCode).toBe(0); + await expect(workspace.fs.stat("/workspace/blocked.txt")).rejects.toThrow(); + expect(await workspace.fs.readFile("/workspace/allowed.txt", "utf8")).toBe("allowed\n"); + }); + }); + // --------------------------------------------------------------- // `git` custom command wiring // --------------------------------------------------------------- diff --git a/packages/computer/src/backends/worker-shell/entrypoint.ts b/packages/computer/src/backends/worker-shell/entrypoint.ts index 690d6a91..a7e073b2 100644 --- a/packages/computer/src/backends/worker-shell/entrypoint.ts +++ b/packages/computer/src/backends/worker-shell/entrypoint.ts @@ -32,6 +32,14 @@ export interface ExecInput { timeoutMs?: number; env?: Record; stdin?: Uint8Array; + // Whether this command may modify the workspace. Defaults to true. + // + // Enforced by asking the host for a workspace stub without write + // access, so the shell never holds a writable capability for the + // duration of the command. A write then fails inside the command + // with EROFS, which the command sees and reports like any other + // filesystem error, and nothing is applied to undo afterwards. + writable?: boolean; } export interface ShellWorkerOptions { @@ -59,7 +67,7 @@ export interface ShellWorkerEnv { // Subset of the WorkspaceServiceProxy Fetcher the shell uses. // Declared structurally so tests can swap in a fake. export interface ShellHostFetcher { - getWorkspace(): Promise; + getWorkspace(options?: { writable?: boolean }): Promise; } // The shape getWorkspace() returns. The shell touches .fs (for @@ -154,9 +162,15 @@ export class ShellWorker< // Reach the host workspace on each call. Concurrent execs get separate // stubs; only their cancellation controllers share this entrypoint. + // The write access is requested here rather than checked later. + // Asking for a read-only stub means every path into the workspace + // for this command — the shell's own builtins, the git command, + // anything a subclass adds — is refused by the handle itself. A + // check at one call site would only cover that call site. + const writable = input.writable ?? true; let ws: HostWorkspaceStub; try { - ws = await this.env.HOST.getWorkspace(); + ws = await this.env.HOST.getWorkspace({ writable }); } catch (error) { if (timer !== undefined) clearTimeout(timer); if (this.#executions.get(id) === controller) this.#executions.delete(id); diff --git a/packages/computer/src/backends/worker-shell/worker-shell.ts b/packages/computer/src/backends/worker-shell/worker-shell.ts index b78c9abc..5464e8e0 100644 --- a/packages/computer/src/backends/worker-shell/worker-shell.ts +++ b/packages/computer/src/backends/worker-shell/worker-shell.ts @@ -40,6 +40,7 @@ export interface WorkerShellFetcher { timeoutMs?: number; env?: Record; stdin?: Uint8Array; + writable?: boolean; }): Promise<{ id: string; events: ReadableStream; @@ -177,6 +178,7 @@ export class WorkerShellBackend implements WorkspaceBackend { timeoutMs: input.timeoutMs, env: input.env, stdin: input.stdin, + writable: input.writable, }); return { id: envelope.id, events: decodeFramedEvents(envelope.events) }; }, diff --git a/packages/computer/src/gate.test.ts b/packages/computer/src/gate.test.ts new file mode 100644 index 00000000..8d8db34d --- /dev/null +++ b/packages/computer/src/gate.test.ts @@ -0,0 +1,214 @@ +import { describe, expect, it, vi } from "vitest"; + +import { + ActionDeniedError, + type AuditOutcome, + noopAudit, + openGate, + type WorkspaceAction, + type WorkspaceAudit, + type WorkspaceGate, + withGate, +} from "./gate.js"; + +function execAction(writable = true): WorkspaceAction { + return { kind: "shell.exec", command: "ls", writable }; +} + +function recordingAudit(): WorkspaceAudit & { entries: [WorkspaceAction, AuditOutcome][] } { + const entries: [WorkspaceAction, AuditOutcome][] = []; + return { + entries, + record(action, outcome) { + entries.push([action, outcome]); + }, + }; +} + +function gateReturning(decision: Awaited>): WorkspaceGate { + return { check: () => decision }; +} + +describe("withGate", () => { + it("runs the action and reports it when the gate allows", async () => { + const audit = recordingAudit(); + const result = await withGate(openGate, audit, execAction(), async () => "ran"); + + expect(result).toBe("ran"); + expect(audit.entries).toEqual([[execAction(), { status: "allowed", writable: true }]]); + }); + + it("does not run the action when the gate denies", async () => { + const audit = recordingAudit(); + const run = vi.fn(async () => "ran"); + const gate = gateReturning({ allow: false, reason: "not on the allowlist" }); + + await expect(withGate(gate, audit, execAction(), run)).rejects.toThrow(ActionDeniedError); + expect(run).not.toHaveBeenCalled(); + expect(audit.entries).toEqual([ + [execAction(), { status: "denied", reason: "not on the allowlist" }], + ]); + }); + + it("carries the action and reason on the denial", async () => { + const gate = gateReturning({ allow: false, reason: "needs approval" }); + const action = execAction(); + + const error = await withGate(gate, noopAudit, action, async () => "ran").catch((e) => e); + + expect(error).toBeInstanceOf(ActionDeniedError); + expect(error.action).toEqual(action); + expect(error.reason).toBe("needs approval"); + expect(error.message).toBe("shell.exec denied: needs approval"); + }); + + it("narrows write access when the gate withdraws it", async () => { + // The useful middle answer: run the command, but not with write + // access. The callback must see the narrowed value. + const audit = recordingAudit(); + const gate = gateReturning({ allow: true, writable: false }); + const seen: boolean[] = []; + + await withGate(gate, audit, execAction(true), async (writable) => { + seen.push(writable); + }); + + expect(seen).toEqual([false]); + expect(audit.entries[0][1]).toEqual({ status: "allowed", writable: false }); + }); + + it("cannot widen write access the action did not ask for", async () => { + // A gate is a restriction, not a grant. Allowing it to hand out + // access the caller never requested would make a read-only exec + // depend on the gate's good behaviour. + const audit = recordingAudit(); + const gate = gateReturning({ allow: true, writable: true }); + const seen: boolean[] = []; + + await withGate(gate, audit, execAction(false), async (writable) => { + seen.push(writable); + }); + + expect(seen).toEqual([false]); + expect(audit.entries[0][1]).toEqual({ status: "allowed", writable: false }); + }); + + it("waits for an async gate before running the action", async () => { + const order: string[] = []; + const gate: WorkspaceGate = { + async check() { + order.push("gate-start"); + await new Promise((resolve) => setTimeout(resolve, 5)); + order.push("gate-end"); + return { allow: true }; + }, + }; + + await withGate(gate, noopAudit, execAction(), async () => { + order.push("action"); + }); + + expect(order).toEqual(["gate-start", "gate-end", "action"]); + }); + + it("propagates a gate that throws instead of treating it as a denial", async () => { + // A gate that could not reach a decision is not a gate that said + // no. Collapsing the two would let an outage read as permission + // in one direction or as a policy refusal in the other. + const failure = new Error("policy service unreachable"); + const gate: WorkspaceGate = { + check() { + throw failure; + }, + }; + const run = vi.fn(async () => "ran"); + + await expect(withGate(gate, noopAudit, execAction(), run)).rejects.toBe(failure); + expect(run).not.toHaveBeenCalled(); + }); + + it("reports a failed action and rethrows the original error", async () => { + const audit = recordingAudit(); + const failure = new Error("command blew up"); + + await expect( + withGate(openGate, audit, execAction(), async () => { + throw failure; + }), + ).rejects.toBe(failure); + expect(audit.entries).toEqual([[execAction(), { status: "failed", error: failure }]]); + }); + + it("does not let a throwing audit hook change a successful outcome", async () => { + const audit: WorkspaceAudit = { + record() { + throw new Error("log sink down"); + }, + }; + + await expect(withGate(openGate, audit, execAction(), async () => "ran")).resolves.toBe("ran"); + }); + + it("does not let a rejecting audit hook change a successful outcome", async () => { + const audit: WorkspaceAudit = { + record() { + return Promise.reject(new Error("log sink down")); + }, + }; + + await expect(withGate(openGate, audit, execAction(), async () => "ran")).resolves.toBe("ran"); + }); + + it("still denies when the audit hook throws on a denial", async () => { + const audit: WorkspaceAudit = { + record() { + throw new Error("log sink down"); + }, + }; + const gate = gateReturning({ allow: false }); + + await expect(withGate(gate, audit, execAction(), async () => "ran")).rejects.toThrow( + ActionDeniedError, + ); + }); + + it("treats filesystem actions as write actions regardless of decision shape", async () => { + const audit = recordingAudit(); + const action: WorkspaceAction = { kind: "fs.rm", path: "/workspace/a.txt" }; + + await withGate(openGate, audit, action, async (writable) => { + expect(writable).toBe(true); + }); + + expect(audit.entries).toEqual([[action, { status: "allowed", writable: true }]]); + }); + + it("gates each filesystem call separately", async () => { + // Per-call gating is safe for fs actions because each call is the + // whole action. This is the contrast with shell.exec, which is + // gated once for the command. + const seen: string[] = []; + const gate: WorkspaceGate = { + check(action) { + if (action.kind !== "shell.exec") seen.push(`${action.kind}:${action.path}`); + return { allow: true }; + }, + }; + + await withGate(gate, noopAudit, { kind: "fs.write", path: "/a", size: 1 }, async () => {}); + await withGate(gate, noopAudit, { kind: "fs.rm", path: "/b" }, async () => {}); + + expect(seen).toEqual(["fs.write:/a", "fs.rm:/b"]); + }); + + it("passes the open gate and noop audit through without allocating a decision per call", async () => { + // The default path should be cheap: the point of the no-op + // defaults is that a caller who does not opt in pays nothing + // beyond a comparison. + const first = await openGate.check(execAction()); + const second = await openGate.check(execAction()); + + expect(first).toBe(second); + expect(noopAudit.record(execAction(), { status: "allowed", writable: true })).toBeUndefined(); + }); +}); diff --git a/packages/computer/src/gate.ts b/packages/computer/src/gate.ts new file mode 100644 index 00000000..0c6c036b --- /dev/null +++ b/packages/computer/src/gate.ts @@ -0,0 +1,214 @@ +/** + * Approval seams: a gate that runs before an action and an audit hook + * that runs after it. + * + * These exist because the observer in `./observe.ts` deliberately + * cannot do this job. Its contract says an implementation "must run + * `run` synchronously and return its result unchanged so the wrapping + * is invisible to the caller" — observability that changes behaviour + * is a bug. A gate is the opposite: its whole purpose is to refuse. + * Rather than weaken the observer contract, this is a second seam with + * the same shape and the opposite licence. + * + * The split into two hooks matches what each one is for. The gate + * decides, so it runs before the action and sees only the request. The + * audit records, so it runs after and sees the outcome. An audit hook + * cannot deny anything; if it throws, the action has already happened + * and the throw is swallowed, exactly as `withSpan` treats a failing + * `finalize`. + * + * Both default to no-ops, so a caller who does not opt in pays one + * comparison per action. + * + * ## Why a command is gated once, not per write + * + * The gate fires once for a whole `shell.exec`, and the decision + * covers every write that command makes. It is tempting to instead + * check each write as it happens, but that produces worse outcomes + * than refusing the command outright: `rm -rf` issues one call per + * entry, so denying halfway leaves a half-deleted tree. A command is + * the smallest unit that can be refused and leave the workspace in a + * state the caller can reason about. + * + * The same reasoning rules out asking a human mid-command. By the time + * a write arrives the command is running, and there is nowhere to + * suspend it that does not risk a partial result. A gate that wants + * human approval must ask before the command starts, which is where + * this one runs. + * + * Filesystem actions are gated per call, because there each call is + * the whole action and refusing one leaves nothing half-done. + */ + +/** + * What is about to happen, in enough detail for a gate to decide. + * + * Discriminated on `kind`. A gate that only cares about one kind + * should switch on it and return `allowed` for the rest, so new kinds + * added later stay permitted rather than silently breaking callers who + * were never asked about them. + */ +export type WorkspaceAction = + | { + kind: "shell.exec"; + // The command line as it will be handed to the runner. + command: string; + cwd?: string; + // Whether the caller is asking for write access. A gate can + // narrow this to false in its decision; it cannot widen it. + writable: boolean; + // Which registered backend will run this, once resolved. + backend?: string; + } + | { + kind: "fs.write" | "fs.mkdir" | "fs.rm" | "fs.chmod" | "fs.symlink"; + // Absolute path inside the workspace. + path: string; + // Byte length for a write, when known. Absent for the others. + size?: number; + }; + +/** The action kinds, for callers that want to switch exhaustively. */ +export type WorkspaceActionKind = WorkspaceAction["kind"]; + +/** + * A gate's answer. + * + * `allow: true` may carry `writable: false` to permit the action with + * write access withdrawn — the useful middle answer for a command a + * policy is willing to run but not to trust with the workspace. A gate + * can only narrow write access this way. Passing `writable: true` for + * an action that did not ask for it does not grant it. + * + * `allow: false` refuses. `reason` is surfaced to the caller and + * should be phrased for whoever asked for the action, since that is + * who will read it. + */ +export type GateDecision = { allow: true; writable?: boolean } | { allow: false; reason?: string }; + +/** + * Consulted before an action runs. + * + * May be async: a gate is allowed to consult a policy service or wait + * for a human, and the action does not start until it settles. Keep in + * mind that whatever it waits on holds up the caller. + * + * A gate that throws denies the action. The error propagates to the + * caller unchanged rather than being converted into a refusal, because + * a gate that failed to reach a decision is not the same as a gate + * that decided no, and a caller who cannot tell those apart will + * eventually treat an outage as permission. + */ +export interface WorkspaceGate { + check(action: WorkspaceAction): Promise | GateDecision; +} + +/** The outcome of a gated action, as reported to an audit hook. */ +export type AuditOutcome = + | { status: "allowed"; writable: boolean } + | { status: "denied"; reason?: string } + // The action ran and threw. `error` is whatever it threw. + | { status: "failed"; error: unknown }; + +/** + * Notified after an action has been decided and, if allowed, run. + * + * Fires for denied actions too — a record of what was refused is + * usually the more interesting half. Errors thrown here are + * swallowed: the action has already happened, and failing the caller + * over a failed log entry would turn an audit hook into a gate. + */ +export interface WorkspaceAudit { + record(action: WorkspaceAction, outcome: AuditOutcome): void | Promise; +} + +/** Gate that permits everything, used when the caller passes none. */ +export const openGate: WorkspaceGate = { + check() { + return ALLOWED; + }, +}; + +const ALLOWED: GateDecision = { allow: true }; + +/** Audit hook that records nothing, used when the caller passes none. */ +export const noopAudit: WorkspaceAudit = { + record() { + // intentionally empty + }, +}; + +/** + * Thrown when a gate refuses an action. + * + * Carries the action so a caller catching it can report what was + * refused without having to remember what it asked for. + */ +export class ActionDeniedError extends Error { + readonly action: WorkspaceAction; + readonly reason?: string; + + constructor(action: WorkspaceAction, reason?: string) { + super(reason ? `${action.kind} denied: ${reason}` : `${action.kind} denied`); + this.name = "ActionDeniedError"; + this.action = action; + this.reason = reason; + } +} + +/** + * Internal helper: consult `gate`, run `action` if allowed, and report + * the outcome to `audit`. Mirrors `withSpan` in `./observe.ts` so the + * two seams read the same way at a call site that uses both. + * + * `run` receives the effective write access, which is the requested + * access narrowed by whatever the gate decided. Call sites should use + * that argument rather than the flag they passed in, since ignoring it + * is how a narrowed decision gets quietly dropped. + * + * Throws `ActionDeniedError` when the gate refuses. Nothing runs in + * that case. + */ +export async function withGate( + gate: WorkspaceGate, + audit: WorkspaceAudit, + action: WorkspaceAction, + run: (writable: boolean) => Promise, +): Promise { + const requested = action.kind === "shell.exec" ? action.writable : true; + const decision = await gate.check(action); + + if (!decision.allow) { + await reportSafely(audit, action, { status: "denied", reason: decision.reason }); + throw new ActionDeniedError(action, decision.reason); + } + + // The gate can narrow write access but not widen it, so a decision + // asking for more than the action requested is capped rather than + // honoured. + const writable = requested && (decision.writable ?? true); + + try { + const value = await run(writable); + await reportSafely(audit, action, { status: "allowed", writable }); + return value; + } catch (error) { + await reportSafely(audit, action, { status: "failed", error }); + throw error; + } +} + +// An audit hook that throws must not change the outcome of work that +// has already happened. Rejections are swallowed here for the same +// reason withSpan swallows a failing finalize. +async function reportSafely( + audit: WorkspaceAudit, + action: WorkspaceAction, + outcome: AuditOutcome, +): Promise { + try { + await audit.record(action, outcome); + } catch { + // not actionable: the action's outcome is already decided + } +} diff --git a/packages/computer/src/gated-fs.ts b/packages/computer/src/gated-fs.ts new file mode 100644 index 00000000..ba2a92f9 --- /dev/null +++ b/packages/computer/src/gated-fs.ts @@ -0,0 +1,100 @@ +/** + * A `WorkspaceFilesystem` whose mutating methods go through a gate. + * + * `Workspace.fs` hands callers a filesystem handle that writes to the + * local store directly, without crossing the wire. That makes it the + * other way into the workspace besides `shell.exec`, and a gate that + * only covered exec would be trivial to walk around — deny the + * command, write the file yourself. Gating both closes that. + * + * A subclass rather than a wrapper object, so the handle stays a + * `WorkspaceFilesystem` for every caller that already has one and for + * the mount and think surfaces that take one by type. Reads are not + * overridden and cost nothing. + * + * Each mutation is gated on its own. That is safe here in a way it is + * not for a shell command: one `writeFile` is the entire action, so + * refusing it leaves nothing half-finished. See the note in `./gate.ts` + * for why a command is gated once instead. + */ + +import type { Database } from "@cloudflare/dofs"; +import { + type MkdirOptions, + type RmOptions, + WorkspaceFilesystem, + type WorkspaceFilesystemOptions, + type WriteFileContent, + type WriteFileOptions, +} from "@cloudflare/dofs"; + +import { type WorkspaceAudit, type WorkspaceGate, withGate } from "./gate.js"; + +export interface GatedFilesystemOptions extends WorkspaceFilesystemOptions { + gate: WorkspaceGate; + audit: WorkspaceAudit; +} + +export class GatedWorkspaceFilesystem extends WorkspaceFilesystem { + readonly #gate: WorkspaceGate; + readonly #audit: WorkspaceAudit; + + constructor(db: Database, options: GatedFilesystemOptions) { + super(db, options); + this.#gate = options.gate; + this.#audit = options.audit; + } + + override async writeFile( + path: string, + content: WriteFileContent, + options: WriteFileOptions = {}, + ): Promise { + // Only reported when it is already known. Measuring a stream to + // fill in the field would consume it. + const size = byteLength(content); + return withGate(this.#gate, this.#audit, { kind: "fs.write", path, size }, () => + super.writeFile(path, content, options), + ); + } + + override async mkdir(path: string, options: MkdirOptions = {}): Promise { + return withGate(this.#gate, this.#audit, { kind: "fs.mkdir", path }, () => + super.mkdir(path, options), + ); + } + + override async rm(path: string, options: RmOptions = {}): Promise { + // One gate call for the whole tree when recursive. The gate sees + // the path the caller named, which is the decision it can + // actually make; asking per descendant would raise the same + // half-deleted-tree problem that shell commands have. + return withGate(this.#gate, this.#audit, { kind: "fs.rm", path }, () => + super.rm(path, options), + ); + } + + override async chmod(path: string, mode: number): Promise { + return withGate(this.#gate, this.#audit, { kind: "fs.chmod", path }, () => + super.chmod(path, mode), + ); + } + + override async symlink(target: string, path: string): Promise { + // Gated on the link's own path, not the target. Creating a link + // writes at `path`; the target may not exist and is allowed to + // dangle, so it is not the thing being modified. + return withGate(this.#gate, this.#audit, { kind: "fs.symlink", path }, () => + super.symlink(target, path), + ); + } +} + +// A stream has no length until it is read, and reading it here would +// consume the bytes the write needs. Those calls reach the gate with +// `size` absent rather than with a wrong number. +function byteLength(content: WriteFileContent): number | undefined { + if (typeof content === "string") return new TextEncoder().encode(content).byteLength; + if (content instanceof Uint8Array) return content.byteLength; + return undefined; +} diff --git a/packages/computer/src/index.ts b/packages/computer/src/index.ts index 2d521050..74c4070a 100644 --- a/packages/computer/src/index.ts +++ b/packages/computer/src/index.ts @@ -34,6 +34,18 @@ export { type WorkspaceRuntimeClient, } from "./client.js"; export { decodeExecEvents, encodeExecEvents } from "./exec-wire.js"; +export { + ActionDeniedError, + type AuditOutcome, + type GateDecision, + noopAudit, + openGate, + type WorkspaceAction, + type WorkspaceActionKind, + type WorkspaceAudit, + type WorkspaceGate, +} from "./gate.js"; +export { type GatedFilesystemOptions, GatedWorkspaceFilesystem } from "./gated-fs.js"; export { R2Bucket, type R2BucketBinding, type R2BucketOptions } from "./mounts/providers/r2.js"; export type { EagerMount, diff --git a/packages/computer/src/proxy.ts b/packages/computer/src/proxy.ts index 861d9231..495e49a7 100644 --- a/packages/computer/src/proxy.ts +++ b/packages/computer/src/proxy.ts @@ -163,9 +163,11 @@ export class WorkspaceServiceProxy extends WorkerEntrypoint { - const stub = this.#hostStub<{ __getWorkspaceStub(): Promise }>(); - return stub.__getWorkspaceStub(); + async getWorkspace(options?: { writable?: boolean }): Promise { + const stub = this.#hostStub<{ + __getWorkspaceStub(options?: { writable?: boolean }): Promise; + }>(); + return stub.__getWorkspaceStub(options); } // Optional Artifacts CLI hook used by the worker-backend shell. diff --git a/packages/computer/src/runtime/runtime.ts b/packages/computer/src/runtime/runtime.ts index b426a214..cae03914 100644 --- a/packages/computer/src/runtime/runtime.ts +++ b/packages/computer/src/runtime/runtime.ts @@ -67,6 +67,7 @@ export class WorkspaceRuntime { timeoutMs: options.timeoutMs, env: options.env, stdin: options.stdin, + writable: options.writable, }); return wrapCommandHandle(handle, backend); } @@ -80,6 +81,7 @@ export class WorkspaceRuntime { env: options.env, stdin: options.stdin, timeoutMs: options.timeoutMs, + writable: options.writable, }); return wrapModuleHandle(runtime, backend, envelope.id, envelope.events, options.encoding); } diff --git a/packages/computer/src/runtime/types.ts b/packages/computer/src/runtime/types.ts index 52d505ce..153e0daf 100644 --- a/packages/computer/src/runtime/types.ts +++ b/packages/computer/src/runtime/types.ts @@ -111,6 +111,10 @@ export interface WorkspaceRuntimeExecOptions env?: Record; stdin?: Uint8Array | string; timeoutMs?: number; + // Whether this execution may modify the workspace. Defaults to + // true. Narrows the backend's own access rather than widening it: + // a backend registered read-only stays read-only regardless. + writable?: boolean; } export interface WorkspaceRuntimeGetOptions { @@ -145,6 +149,10 @@ export interface ModuleExecutionInput { env?: Record; stdin?: Uint8Array | string; timeoutMs?: number; + // Whether this execution may modify the workspace. Defaults to + // true. Intersected with the backend's configured access, so a + // backend registered read-only stays read-only. + writable?: boolean; } export interface ModuleExecutionEnvelope { diff --git a/packages/computer/src/shell.test.ts b/packages/computer/src/shell.test.ts index 99f937b5..f18c50c6 100644 --- a/packages/computer/src/shell.test.ts +++ b/packages/computer/src/shell.test.ts @@ -8,6 +8,13 @@ import type { ExecEvent, ShellRPC, SyncRPC, WorkspaceRPC } from "@cloudflare/computer-rpc"; import { describe, expect, it } from "vitest"; +import { + ActionDeniedError, + type AuditOutcome, + type WorkspaceAction, + type WorkspaceAudit, + type WorkspaceGate, +} from "./gate.js"; import type { KillSignal } from "./shell.js"; import { type Sync, WorkspaceShell } from "./shell.js"; @@ -46,6 +53,7 @@ interface ExecCall { id: string | undefined; cwd: string | undefined; timeoutMs: number | undefined; + writable: boolean | undefined; } interface GetExecCall { @@ -133,6 +141,7 @@ function fakeRpc(options: FakeRpcOptions = {}): FakeRpc { id: input.id, cwd: input.cwd, timeoutMs: input.timeoutMs, + writable: input.writable, }); if (options.throwOnExec !== undefined) throw options.throwOnExec; const id = input.id ?? mintedId; @@ -900,3 +909,219 @@ function liveRpc(): { }, }; } + +// --------------------------------------------------------------------------- +// exec() — write access and the gate +// --------------------------------------------------------------------------- + +describe("WorkspaceShell.exec — write access", () => { + function recordingSync(): Sync & { pulls: (boolean | undefined)[] } { + const pulls: (boolean | undefined)[] = []; + return { + pulls, + async push() { + return 0; + }, + async pull(options) { + pulls.push(options?.writable); + return applied(0); + }, + }; + } + + it("sends writable: true when the caller says nothing", async () => { + const fake = fakeRpc(); + const shell = new WorkspaceShell(fake.rpc.shell, makeSync()); + await (await shell.exec("ls")).result(); + expect(fake.calls.exec[0].writable).toBe(true); + }); + + it("forwards writable: false to the runner", async () => { + const fake = fakeRpc(); + const shell = new WorkspaceShell(fake.rpc.shell, makeSync()); + await (await shell.exec("ls", { writable: false })).result(); + expect(fake.calls.exec[0].writable).toBe(false); + }); + + it("carries the command's write access into its own post-command pull", async () => { + // Without this the bracket after the command would apply exactly + // the changes the command was not allowed to make. + const fake = fakeRpc(); + const sync = recordingSync(); + const shell = new WorkspaceShell(fake.rpc.shell, sync); + + await (await shell.exec("ls", { writable: false })).result(); + + expect(sync.pulls).toEqual([false]); + }); + + it("pulls with write access for an ordinary command", async () => { + const fake = fakeRpc(); + const sync = recordingSync(); + const shell = new WorkspaceShell(fake.rpc.shell, sync); + + await (await shell.exec("ls")).result(); + + expect(sync.pulls).toEqual([true]); + }); + + it("consults the gate once per command, before the push", async () => { + const order: string[] = []; + const fake = fakeRpc(); + const sync: Sync = { + async push() { + order.push("push"); + return 0; + }, + async pull() { + order.push("pull"); + return applied(0); + }, + }; + const gate: WorkspaceGate = { + check(action) { + order.push(`gate:${action.kind}`); + return { allow: true }; + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync, undefined, { gate }); + + await (await shell.exec("ls")).result(); + + expect(order).toEqual(["gate:shell.exec", "push", "pull"]); + }); + + it("does not spawn or push when the gate denies", async () => { + const fake = fakeRpc(); + const sync = recordingSync(); + const gate: WorkspaceGate = { + check: () => ({ allow: false, reason: "not allowed" }), + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync, undefined, { gate }); + + await expect(shell.exec("rm -rf /")).rejects.toThrow(ActionDeniedError); + expect(fake.calls.exec).toEqual([]); + expect(sync.pulls).toEqual([]); + }); + + it("runs a command the gate allowed but withdrew write access from", async () => { + // The middle answer: the command runs, and neither the runner nor + // the pull that follows it gets write access. + const fake = fakeRpc(); + const sync = recordingSync(); + const gate: WorkspaceGate = { + check: () => ({ allow: true, writable: false }), + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync, undefined, { gate }); + + await (await shell.exec("git log")).result(); + + expect(fake.calls.exec[0].writable).toBe(false); + expect(sync.pulls).toEqual([false]); + }); + + it("shows the gate the command and its requested write access", async () => { + const seen: WorkspaceAction[] = []; + const fake = fakeRpc(); + const gate: WorkspaceGate = { + check(action) { + seen.push(action); + return { allow: true }; + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { gate }); + + await (await shell.exec("cat f", { cwd: "/workspace/sub", writable: false })).result(); + + expect(seen).toEqual([ + { + kind: "shell.exec", + command: "cat f", + cwd: "/workspace/sub", + writable: false, + backend: undefined, + }, + ]); + }); + + it("reports the spawn to the audit hook with the effective write access", async () => { + const entries: AuditOutcome[] = []; + const fake = fakeRpc(); + const audit: WorkspaceAudit = { + record(_action, outcome) { + entries.push(outcome); + }, + }; + const gate: WorkspaceGate = { check: () => ({ allow: true, writable: false }) }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { gate, audit }); + + await (await shell.exec("ls")).result(); + + expect(entries).toEqual([{ status: "allowed", writable: false }]); + }); + + it("reports a refused command to the audit hook", async () => { + const entries: [string, AuditOutcome][] = []; + const fake = fakeRpc(); + const audit: WorkspaceAudit = { + record(action, outcome) { + entries.push([action.kind, outcome]); + }, + }; + const gate: WorkspaceGate = { check: () => ({ allow: false, reason: "no" }) }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { gate, audit }); + + await shell.exec("rm -rf /").catch(() => {}); + + expect(entries).toEqual([["shell.exec", { status: "denied", reason: "no" }]]); + }); + + it("reports a failed spawn to the audit hook", async () => { + const failure = new Error("spawn failed"); + const entries: AuditOutcome[] = []; + const fake = fakeRpc({ throwOnExec: failure }); + const audit: WorkspaceAudit = { + record(_action, outcome) { + entries.push(outcome); + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { audit }); + + await expect(shell.exec("ls")).rejects.toBe(failure); + expect(entries).toEqual([{ status: "failed", error: failure }]); + }); + + it("surfaces entries the read-only pull refused on the result", async () => { + // The reporting path a caller actually reads: a command that ran + // without write access, against a backend that wrote to its own + // copy anyway, comes back with those entries in skipped. + const fake = fakeRpc(); + const sync: Sync = { + async push() { + return 0; + }, + async pull(options) { + if (options?.writable === false) { + return { + applied: 0, + skipped: [{ path: "/workspace/out.txt", rev: 4, reason: "no-write-access" as const }], + }; + } + return applied(1); + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync); + + const result = await (await shell.exec("touch out.txt", { writable: false })).result(); + + expect(result.pulled).toBe(0); + expect(result.skipped).toEqual([ + { path: "/workspace/out.txt", rev: 4, reason: "no-write-access" }, + ]); + expect(result.sync).toEqual({ + status: "complete", + applied: 0, + skipped: [{ path: "/workspace/out.txt", rev: 4, reason: "no-write-access" }], + }); + }); +}); diff --git a/packages/computer/src/shell.ts b/packages/computer/src/shell.ts index a199281c..b1e7acf2 100644 --- a/packages/computer/src/shell.ts +++ b/packages/computer/src/shell.ts @@ -29,6 +29,7 @@ import type { ExecEvent, ShellRPC } from "@cloudflare/computer-rpc"; import type { ApplyResult, SkippedEntry } from "@cloudflare/dofs"; +import { noopAudit, openGate, type WorkspaceAudit, type WorkspaceGate, withGate } from "./gate.js"; import { noopObserver, safeErrorMessage, type WorkspaceObserver, withSpan } from "./observe.js"; import { assertNotTemplate } from "./sh.js"; @@ -54,10 +55,13 @@ export interface ExecResult { // VFS sync stats from the docs/05 bracket. // pushed — entries shipped by the pre-exec pushOnce. // pulled — entries the post-drain pullOnce applied locally. - // skipped — entries the post-drain pullOnce did NOT apply - // because they targeted a read-only mount root. - // Empty when no read-only mounts are registered or - // the container stayed clear of them. + // skipped — entries the post-drain pullOnce did not apply, + // either because they targeted a read-only mount + // root or because the command ran without write + // access and the pull refused everything it was + // offered. Always empty for a backend that shares + // the workspace store: there is no pull to refuse, + // and a write fails inside the command instead. // pushed is observed before the stream is returned. The remaining // fields describe the post-command pull when result() is used. pushed: number; @@ -116,6 +120,23 @@ export interface ExecOptions { // one passed to the Workspace constructor); pass the id of // another configured backend to route this call there. backend?: string; + // Whether this command may modify the workspace. Defaults to true. + // Pass false for a command expected only to read: writes it + // attempts then fail rather than land silently. + // + // The point is the misclassified command. A caller that decides + // `git log` is read-only and is wrong about it — an alias, a shell + // function, a `;` it did not parse — gets a failure it can see + // instead of a mutation it did not intend. Which failure depends on + // the backend: one that works directly against the workspace store + // fails the write inside the command with EROFS, and one with its + // own copy of the files has the change refused on the way back, + // reported in `skipped`. + // + // A configured gate can withdraw write access even when this is + // true, so a command may end up read-only without the caller + // asking. It can never gain access this way. + writable?: boolean; } export interface GetExecOptions { @@ -138,19 +159,36 @@ export interface GetExecOptions { // skipped read-only entries on ExecResult. export interface Sync { push(): Promise; - pull(): Promise; + // The post-command pull carries the command's write access, so a + // command that ran read-only cannot have its changes applied by the + // bracket that follows it. + pull(options?: { writable?: boolean }): Promise; onPullPending?(error: unknown): Promise; } +export interface WorkspaceShellHooks { + gate?: WorkspaceGate; + audit?: WorkspaceAudit; +} + export class WorkspaceShell { readonly #shell: ShellRPC; readonly #sync: Sync; readonly #observer: WorkspaceObserver; + readonly #gate: WorkspaceGate; + readonly #audit: WorkspaceAudit; - constructor(shell: ShellRPC, sync: Sync, observer: WorkspaceObserver = noopObserver) { + constructor( + shell: ShellRPC, + sync: Sync, + observer: WorkspaceObserver = noopObserver, + hooks: WorkspaceShellHooks = {}, + ) { this.#shell = shell; this.#sync = sync; this.#observer = observer; + this.#gate = hooks.gate ?? openGate; + this.#audit = hooks.audit ?? noopAudit; } exec(command: string): Promise>; @@ -161,6 +199,34 @@ export class WorkspaceShell { options: ExecOptions = {}, ): Promise> { assertNotTemplate(command); + // The gate runs before the push, so a denied command does not + // move data. It is consulted once for the whole command; see + // ./gate.ts for why a write-by-write gate would be worse. + // + // The audit hook fires here too, on the spawn, rather than after + // the command exits. exec() returns a detached handle that the + // caller may never drain, so there is no later point that is + // guaranteed to arrive. What the command then did is on the + // observer's span and on ExecResult. + return withGate( + this.#gate, + this.#audit, + { + kind: "shell.exec", + command, + cwd: options.cwd, + writable: options.writable ?? true, + backend: options.backend, + }, + (writable) => this.#spawn(command, options, writable), + ); + } + + async #spawn( + command: string, + options: ExecOptions, + writable: boolean, + ): Promise> { // Pre-exec push: ship anything the host wrote since the last // push so the spawned command sees it. Failures non-fatal per // docs/05 — the command still runs; pushed reports 0. @@ -177,6 +243,7 @@ export class WorkspaceShell { "workspace.runtime.cwd": options.cwd, "workspace.runtime.timeout_ms": options.timeoutMs, "workspace.runtime.id": options.id, + "workspace.runtime.writable": writable, }, () => this.#shell.exec({ @@ -189,6 +256,7 @@ export class WorkspaceShell { typeof options.stdin === "string" ? new TextEncoder().encode(options.stdin) : options.stdin, + writable, }), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.runtime.id", outcome.value.id); @@ -200,7 +268,15 @@ export class WorkspaceShell { // exec call — because we hand the inner stream off to the // caller and can't `using` the envelope ourselves. const events = disposeOnDone(envelope.events, () => maybeDispose(envelope)); - return wrapHandle(this.#shell, this.#sync, envelope.id, events, options.encoding, pushed); + return wrapHandle( + this.#shell, + this.#sync, + envelope.id, + events, + options.encoding, + pushed, + writable, + ); } get(id: string): Promise>; @@ -249,8 +325,9 @@ function wrapHandle( wireEvents: ReadableStream, encoding: E | undefined, pushed: number, + writable = true, ): ExecHandle { - const postPull = withPostPull(pipeEvents(wireEvents, encoding), sync); + const postPull = withPostPull(pipeEvents(wireEvents, encoding), sync, writable); const stream = postPull.stream; const handle = stream as ExecHandle; let resultPromise: Promise> | undefined; @@ -378,6 +455,7 @@ interface PostPullOutcome { function withPostPull( source: ReadableStream>, sync: Sync, + writable: boolean, ): { stream: ReadableStream>; outcome: Promise } { const reader = source.getReader(); let resolveOutcome!: (outcome: PostPullOutcome) => void; @@ -394,7 +472,7 @@ function withPostPull( return; } reader.releaseLock(); - const pulled = await runPostPull(sync); + const pulled = await runPostPull(sync, writable); resolveOutcome(pulled); controller.close(); } catch (error) { @@ -427,9 +505,12 @@ function withPostPull( return { stream, outcome }; } -async function runPostPull(sync: Sync): Promise { +async function runPostPull(sync: Sync, writable: boolean): Promise { try { - const result = await sync.pull(); + // The command's write access travels with its own post-command + // pull. Without this the bracket would apply exactly the changes + // the command was not allowed to make. + const result = await sync.pull({ writable }); return { applied: result.applied, skipped: result.skipped, diff --git a/packages/computer/src/stub.ts b/packages/computer/src/stub.ts index 844d8452..fe6d0b71 100644 --- a/packages/computer/src/stub.ts +++ b/packages/computer/src/stub.ts @@ -47,6 +47,7 @@ import type { ReadFileOptions, RmOptions, WorkspaceDirentResult, + WorkspaceFilesystem, WorkspaceFoundEntry, WorkspaceGrepMatch, WorkspaceStatResult, @@ -89,10 +90,18 @@ import type { Workspace } from "./workspace.js"; // the same reason. export class WorkspaceFilesystemStub extends RpcTarget { readonly #ws: Workspace; - - constructor(ws: Workspace) { + // The handle this stub's mutations go through. A stub made without + // write access gets a read-only handle, so a caller holding it + // cannot write no matter which method it reaches for. The handle + // carries the capability rather than this class checking a flag per + // method, which is what keeps a new method from being added without + // the check. + readonly #fs: WorkspaceFilesystem; + + constructor(ws: Workspace, writable = true) { super(); this.#ws = ws; + this.#fs = ws.fsWithAccess(writable); trackStub(this); } @@ -110,7 +119,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { optionsOrEncoding?: "utf8" | ReadFileOptions, ): Promise> { return withSpan(this.#ws.observer, "workspace.fs.readFile", { "workspace.fs.path": path }, () => - this.#ws.fs.readFile(path, optionsOrEncoding as ReadFileOptions), + this.#fs.readFile(path, optionsOrEncoding as ReadFileOptions), ); } @@ -125,7 +134,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { stat(path: string): Promise { return withSpan(this.#ws.observer, "workspace.fs.stat", { "workspace.fs.path": path }, () => - this.#ws.fs.stat(path), + this.#fs.stat(path), ); } @@ -136,7 +145,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { { "workspace.fs.path": path }, async () => { try { - return await this.#ws.fs.stat(path); + return await this.#fs.stat(path); } catch (error) { if ((error as { code?: string }).code === "ENOENT") return null; throw error; @@ -147,7 +156,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { lstat(path: string): Promise { return withSpan(this.#ws.observer, "workspace.fs.lstat", { "workspace.fs.path": path }, () => - this.#ws.fs.lstat(path), + this.#fs.lstat(path), ); } @@ -158,7 +167,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { { "workspace.fs.path": path }, async () => { try { - return await this.#ws.fs.lstat(path); + return await this.#fs.lstat(path); } catch (error) { if ((error as { code?: string }).code === "ENOENT") return null; throw error; @@ -169,7 +178,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { readlink(path: string): Promise { return withSpan(this.#ws.observer, "workspace.fs.readlink", { "workspace.fs.path": path }, () => - this.#ws.fs.readlink(path), + this.#fs.readlink(path), ); } @@ -178,7 +187,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.readdir", { "workspace.fs.path": path }, - () => this.#ws.fs.readdir(path, options), + () => this.#fs.readdir(path, options), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.entries", outcome.value.length); }, @@ -190,7 +199,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.find", { "workspace.fs.path": directory, "workspace.fs.pattern": pattern }, - () => this.#ws.fs.find(directory, pattern), + () => this.#fs.find(directory, pattern), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.matches", outcome.value.length); }, @@ -202,7 +211,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.ls", { "workspace.fs.path": prefix }, - () => this.#ws.fs.ls(prefix), + () => this.#fs.ls(prefix), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.entries", outcome.value.length); }, @@ -214,7 +223,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.grep", { "workspace.fs.path": path, "workspace.fs.pattern": pattern }, - () => this.#ws.fs.grep(pattern, path, options), + () => this.#fs.grep(pattern, path, options), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.matches", outcome.value.length); }, @@ -232,7 +241,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.writeFile", { "workspace.fs.path": path }, - () => this.#ws.fs.writeFile(path, content, options), + () => this.#fs.writeFile(path, content, options), ); } @@ -241,7 +250,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.mkdir", { "workspace.fs.path": path, "workspace.fs.recursive": options.recursive }, - () => this.#ws.fs.mkdir(path, options), + () => this.#fs.mkdir(path, options), ); } @@ -254,7 +263,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { "workspace.fs.recursive": options.recursive, "workspace.fs.force": options.force, }, - () => this.#ws.fs.rm(path, options), + () => this.#fs.rm(path, options), ); } @@ -263,7 +272,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.chmod", { "workspace.fs.path": path, "workspace.fs.mode": mode }, - () => this.#ws.fs.chmod(path, mode), + () => this.#fs.chmod(path, mode), ); } @@ -272,7 +281,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.symlink", { "workspace.fs.path": path, "workspace.fs.target": target }, - () => this.#ws.fs.symlink(target, path), + () => this.#fs.symlink(target, path), ); } } @@ -373,10 +382,15 @@ export class WorkspaceRuntimeExecHandleStub< export class WorkspaceRuntimeStub extends RpcTarget { readonly #ws: Workspace; + // Ceiling on the write access of commands spawned through this + // stub. A read-only stub narrows every exec it is asked for; it + // cannot be talked into granting more by passing writable: true. + readonly #writable: boolean; - constructor(ws: Workspace) { + constructor(ws: Workspace, writable = true) { super(); this.#ws = ws; + this.#writable = writable; trackStub(this); } @@ -403,7 +417,10 @@ export class WorkspaceRuntimeStub extends RpcTarget { source: string, options: WorkspaceRuntimeExecOptions<"utf8" | undefined>, ) => Promise> - )(source, options); + )(source, { + ...options, + writable: this.#writable && (options.writable ?? true), + }); if (options.id !== undefined && handle.id !== options.id) { throw new Error(`backend ran exec as ${handle.id}, not the requested id ${options.id}`); } @@ -580,6 +597,12 @@ export class WorkspaceGitStub extends RpcTarget { // between wsd and the DO. WorkspaceStub here is a different thing // (the Workers-RPC value carried between the DO and a Worker), so // the name doesn't clash. +export interface WorkspaceStubOptions { + // Whether the holder of this stub may modify the workspace. + // Defaults to true. + writable?: boolean; +} + export class WorkspaceStub extends RpcTarget { // Getters rather than instance properties so Workers RPC // exposes them through the stub proxy. Plain readonly fields @@ -592,10 +615,14 @@ export class WorkspaceStub extends RpcTarget { readonly #artifacts: WorkspaceArtifactsStub; readonly #useThink: boolean; - constructor(ws: Workspace) { + // `writable: false` hands out a stub that cannot write. The whole + // stub, not just its fs: a read-only stub whose runtime could still + // spawn a writing command would not be read-only. + constructor(ws: Workspace, options: WorkspaceStubOptions = {}) { super(); - this.#fs = new WorkspaceFilesystemStub(ws); - this.#runtime = new WorkspaceRuntimeStub(ws); + const writable = options.writable ?? true; + this.#fs = new WorkspaceFilesystemStub(ws, writable); + this.#runtime = new WorkspaceRuntimeStub(ws, writable); this.#git = new WorkspaceGitStub(ws); this.#assets = ws.assets === undefined ? undefined : new WorkspaceAssetsStub(ws); this.#artifacts = new WorkspaceArtifactsStub(ws.artifacts); diff --git a/packages/computer/src/tools/ai.test.ts b/packages/computer/src/tools/ai.test.ts index 9ea35cdb..5511c658 100644 --- a/packages/computer/src/tools/ai.test.ts +++ b/packages/computer/src/tools/ai.test.ts @@ -323,6 +323,7 @@ describe("createAITools exec tool", () => { command: "npm test", cwd: "/workspace", backend: "container", + writable: true, exitCode: 2, stdout: "abc\n\n[truncated, 3 more bytes]", stderr: "uvw\n\n[truncated, 3 more bytes]", @@ -437,6 +438,7 @@ describe("createAITools exec tool", () => { command: "npm test", cwd: null, backend: "shell", + writable: true, error: "backend unavailable", }); }); @@ -465,6 +467,7 @@ describe("createAITools exec tool", () => { command: "npm test", cwd: null, backend: "shell", + writable: true, error: "transport closed", }); }); diff --git a/packages/computer/src/tools/exec.test.ts b/packages/computer/src/tools/exec.test.ts new file mode 100644 index 00000000..75deb8fa --- /dev/null +++ b/packages/computer/src/tools/exec.test.ts @@ -0,0 +1,237 @@ +import { describe, expect, it } from "vitest"; +import { createExecTool, type ExecToolInput, type ExecWorkspaceLike } from "./exec.js"; + +interface ExecCall { + command: string; + cwd: string | undefined; + backend: string | undefined; + writable: boolean | undefined; +} + +interface FakeResult { + exitCode: number; + stdout: string; + stderr: string; + skipped?: Array<{ path: string; op: "write" | "delete"; reason: string }>; +} + +function fakeWorkspace( + result: FakeResult | Error = { + exitCode: 0, + stdout: "", + stderr: "", + }, +): ExecWorkspaceLike & { calls: ExecCall[] } { + const calls: ExecCall[] = []; + return { + calls, + runtime: { + async exec(command, options) { + calls.push({ + command, + cwd: options.cwd, + backend: options.backend, + writable: options.writable, + }); + if (result instanceof Error) throw result; + return { result: async () => result }; + }, + }, + }; +} + +function toolFor( + workspace: ExecWorkspaceLike, + writable?: (input: ExecToolInput) => boolean, +): ReturnType { + return createExecTool({ + workspace, + backends: { shell: { description: "a shell" }, container: { description: "a container" } }, + defaultBackend: "shell", + writable, + }); +} + +// The `ai` package types execute as optional and pass a second +// argument the tool ignores. +async function run( + tool: ReturnType, + input: { command: string; cwd?: string; backend?: string }, +): Promise> { + const execute = tool.execute as (i: typeof input, o: unknown) => Promise>; + return execute(input, {}); +} + +describe("createExecTool — write access", () => { + it("runs commands with write access when no resolver is configured", async () => { + const ws = fakeWorkspace(); + await run(toolFor(ws), { command: "npm test" }); + expect(ws.calls[0].writable).toBe(true); + }); + + it("asks the resolver and forwards its answer", async () => { + const ws = fakeWorkspace(); + const tool = toolFor(ws, ({ command }) => !command.startsWith("git log")); + + await run(tool, { command: "git log --oneline" }); + await run(tool, { command: "npm install" }); + + expect(ws.calls.map((c) => c.writable)).toEqual([false, true]); + }); + + it("shows the resolver the resolved backend, not the model's selector", async () => { + // The resolver's decision often depends on where the command + // runs, since that determines whether a refused write is + // prevented or only reported afterwards. + const seen: ExecToolInput[] = []; + const ws = fakeWorkspace(); + const tool = toolFor(ws, (input) => { + seen.push(input); + return true; + }); + + await run(tool, { command: "ls" }); + await run(tool, { command: "ls", backend: "container", cwd: "/workspace/sub" }); + + expect(seen).toEqual([ + { command: "ls", cwd: undefined, backend: "shell" }, + { command: "ls", cwd: "/workspace/sub", backend: "container" }, + ]); + }); + + it("does not accept write access as model input", async () => { + // The model must not classify its own command. If it could, the + // command mislabelled as read-only — the case this exists to + // catch — would arrive labelled read-only and be trusted. + const ws = fakeWorkspace(); + const tool = toolFor(ws, () => false); + + await run(tool, { command: "rm -rf /", writable: true } as never); + + expect(ws.calls[0].writable).toBe(false); + }); + + it("keeps write access out of the input schema entirely", async () => { + const tool = toolFor(fakeWorkspace()); + const schema = tool.inputSchema as { shape?: Record }; + expect(Object.keys(schema.shape ?? {})).toEqual(["command", "cwd", "backend"]); + }); + + it("reports the write access it ran with on a success", async () => { + // So the model can tell a refused write from a broken command; + // otherwise its next move is to retry the same thing. + const ws = fakeWorkspace({ exitCode: 1, stdout: "", stderr: "read-only access\n" }); + const output = await run( + toolFor(ws, () => false), + { command: "touch f" }, + ); + + expect(output.writable).toBe(false); + expect(output.exitCode).toBe(1); + expect(output.stderr).toContain("read-only access"); + }); + + it("returns a refused command as a tool result rather than throwing", async () => { + // A gate denial arrives as a thrown error. Returning it keeps the + // agent loop alive and lets the model read why it was refused. + const ws = fakeWorkspace(new Error("shell.exec denied: not on the allowlist")); + const output = await run(toolFor(ws), { command: "rm -rf /" }); + + expect(output.error).toBe("shell.exec denied: not on the allowlist"); + expect(output.writable).toBe(true); + }); +}); + +describe("createExecTool — refused writes", () => { + // A backend that keeps its own copy of the files does not fail the + // command when it lacks write access. The command writes, exits 0, + // and the changes are refused on the way back. Without this field + // the model reads that as a clean success and reports work it did + // not do. + it("tells the model when the sync refused the command's writes", async () => { + const ws = fakeWorkspace({ + exitCode: 0, + stdout: "", + stderr: "", + skipped: [ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + { path: "/workspace/b.txt", op: "write", reason: "no-write-access" }, + ], + }); + + const result = await run( + toolFor(ws, () => false), + { command: "printf hi > /workspace/a.txt" }, + ); + + expect(result.discardedWrites).toEqual({ + count: 2, + reason: "no-write-access", + paths: ["/workspace/a.txt", "/workspace/b.txt"], + }); + }); + + it("names the read-only mount case separately, since the fix differs", async () => { + const ws = fakeWorkspace({ + exitCode: 0, + stdout: "", + stderr: "", + skipped: [{ path: "/workspace/ro/a.txt", op: "write", reason: "read-only" }], + }); + + const result = await run(toolFor(ws), { command: "printf hi > /workspace/ro/a.txt" }); + + expect(result.discardedWrites).toMatchObject({ count: 1, reason: "read-only" }); + }); + + it("reports a mixed batch as mixed rather than picking one", async () => { + const ws = fakeWorkspace({ + exitCode: 0, + stdout: "", + stderr: "", + skipped: [ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + { path: "/workspace/ro/b.txt", op: "write", reason: "read-only" }, + ], + }); + + const result = await run(toolFor(ws), { command: "sh ./write-both.sh" }); + + expect(result.discardedWrites).toMatchObject({ count: 2, reason: "mixed" }); + }); + + // The field is absent rather than empty on the common path: every + // key here is context the model pays for on every single call. + it("says nothing when the command's writes all landed", async () => { + const ws = fakeWorkspace({ exitCode: 0, stdout: "ok", stderr: "", skipped: [] }); + const result = await run(toolFor(ws), { command: "printf hi > /workspace/a.txt" }); + expect(result).not.toHaveProperty("discardedWrites"); + }); + + it("says nothing when the backend reports no sync stats at all", async () => { + const ws = fakeWorkspace({ exitCode: 0, stdout: "ok", stderr: "" }); + const result = await run(toolFor(ws), { command: "ls" }); + expect(result).not.toHaveProperty("discardedWrites"); + }); + + // One refused write per file means a recursive change can produce + // thousands. The count stays honest; the list is a sample. + it("caps the path list but keeps the true count", async () => { + const skipped = Array.from({ length: 25 }, (_, i) => ({ + path: `/workspace/f${i}.txt`, + op: "write" as const, + reason: "no-write-access" as const, + })); + const ws = fakeWorkspace({ exitCode: 0, stdout: "", stderr: "", skipped }); + + const result = await run( + toolFor(ws, () => false), + { command: "sh ./many.sh" }, + ); + + const discarded = result.discardedWrites as { count: number; paths: string[] }; + expect(discarded.count).toBe(25); + expect(discarded.paths).toHaveLength(10); + expect(discarded.paths[0]).toBe("/workspace/f0.txt"); + }); +}); diff --git a/packages/computer/src/tools/exec.ts b/packages/computer/src/tools/exec.ts index efbb93c1..f83d987f 100644 --- a/packages/computer/src/tools/exec.ts +++ b/packages/computer/src/tools/exec.ts @@ -5,26 +5,89 @@ export interface ExecWorkspaceLike { runtime: { exec( command: string, - options: { cwd?: string; encoding: "utf8"; backend?: string }, + options: { cwd?: string; encoding: "utf8"; backend?: string; writable?: boolean }, ): Promise<{ result(): Promise<{ exitCode: number; stdout: string; stderr: string; + // Changes the post-command pull refused. Optional because a + // backend that shares the workspace store never produces any: + // its writes fail where they happen, so there is nothing left + // to refuse on the way back. + skipped?: ReadonlyArray<{ path: string; op: "write" | "delete"; reason: string }>; }>; }>; }; } +// Refused writes reported back to the model. +// +// A backend that keeps its own copy of the files cannot be stopped +// from writing when it has no write access. It writes, exits zero, +// and the changes are dropped when they are pulled back. Nothing in +// the exit code or the output says so, so a model that is not told +// reports work it did not do. +// +// `count` is every refused entry; `paths` is a capped sample, because +// one entry per file means a recursive change can produce thousands +// and the model is paying for each one. +export interface DiscardedWrites { + count: number; + // "no-write-access" — the command ran without the capability. + // "read-only" — it reached into a read-only mount root. + // Different fixes, so they are not collapsed into one word. + reason: "no-write-access" | "read-only" | "mixed"; + paths: string[]; +} + +const MAX_REPORTED_PATHS = 10; + +function summarizeSkipped( + skipped: ReadonlyArray<{ path: string; reason: string }> | undefined, +): DiscardedWrites | undefined { + if (skipped === undefined || skipped.length === 0) return undefined; + const reasons = new Set(skipped.map((entry) => entry.reason)); + const reason = + reasons.size === 1 ? (skipped[0].reason as DiscardedWrites["reason"]) : ("mixed" as const); + return { + count: skipped.length, + reason, + paths: skipped.slice(0, MAX_REPORTED_PATHS).map((entry) => entry.path), + }; +} + export interface ExecBackendDescription { description: string; } +export interface ExecToolInput { + command: string; + cwd?: string; + backend: string; +} + export interface ExecToolOptions { workspace: ExecWorkspaceLike; backends: Record; defaultBackend: string; maxBytes?: number; + + // Decides whether a command may modify the workspace. Omit to let + // every command write, which is the behaviour without this option. + // + // Deliberately not part of the input schema, so the model cannot + // set it. A model that classifies its own command is the failure + // this is meant to catch: the whole point is the command mislabelled + // as read-only, and asking the same model that mislabelled it to + // declare the label would make the flag agree with the mistake. The + // host decides — from an allowlist, a plan step, a human, whatever + // it already trusts — and the model finds out by the write failing. + // + // The classification is still allowed to be wrong. It fails safe in + // that direction: a read command marked read-only runs fine, and a + // write command marked read-only fails visibly instead of writing. + writable?: (input: ExecToolInput) => boolean; } const DEFAULT_MAX_BYTES = 64 * 1024; @@ -77,26 +140,43 @@ export function createExecTool( }), execute: async ({ command, cwd, backend }) => { const selectedBackend = backend ?? options.defaultBackend; + const writable = options.writable?.({ command, cwd, backend: selectedBackend }) ?? true; try { const handle = await options.workspace.runtime.exec(command, { cwd, encoding: "utf8", backend: selectedBackend, + writable, }); const result = await handle.result(); + const discardedWrites = summarizeSkipped(result.skipped); return { command, cwd: cwd ?? null, backend: selectedBackend, + // Reported so the model can tell a refused write from a + // broken command. Without it a read-only run looks like an + // arbitrary failure and the model's next move is to retry + // the same command. + writable, exitCode: result.exitCode, stdout: truncate(result.stdout, maxBytes), stderr: truncate(result.stderr, maxBytes), + // Spread so the key is absent rather than null on the + // common path. Every key here is context the model reads on + // every call, and "nothing was refused" is the usual case. + ...(discardedWrites !== undefined ? { discardedWrites } : {}), }; } catch (err) { + // A gate refusing the command arrives here. It is returned as + // a tool result rather than thrown, like every other failure, + // so the model reads the refusal and can respond to it instead + // of the agent loop tearing down. return { command, cwd: cwd ?? null, backend: selectedBackend, + writable, error: err instanceof Error ? err.message : String(err), }; } diff --git a/packages/computer/src/tools/index.ts b/packages/computer/src/tools/index.ts index 45dc297c..22e9dff8 100644 --- a/packages/computer/src/tools/index.ts +++ b/packages/computer/src/tools/index.ts @@ -1,5 +1,11 @@ export { type CreateAIToolsOptions, createAITools } from "./ai.js"; -export { createExecTool, type ExecBackendDescription, type ExecToolOptions } from "./exec.js"; +export { + createExecTool, + type ExecBackendDescription, + type ExecToolInput, + type ExecToolOptions, + type ExecWorkspaceLike, +} from "./exec.js"; export { createEditTool, type EditToolOptions } from "./fs/edit.js"; export { createListTool, type ListToolOptions } from "./fs/list.js"; export { createReadTool, type ReadToolOptions } from "./fs/read.js"; diff --git a/packages/computer/src/with-workspace.ts b/packages/computer/src/with-workspace.ts index d3278f7e..9a3ff9b8 100644 --- a/packages/computer/src/with-workspace.ts +++ b/packages/computer/src/with-workspace.ts @@ -36,7 +36,9 @@ export const WORKSPACE = Symbol("workspace"); // The prototype method `getWorkspace(stub)` calls over RPC. Exported // so the client and its tests can name the shape. export interface WorkspaceStubHost { - __getWorkspaceStub(): Promise; + __getWorkspaceStub( + options?: import("./stub.js").WorkspaceStubOptions, + ): Promise; } // A host that carries the symbol-stashed Workspace. Used by the @@ -68,11 +70,19 @@ export function withWorkspace( (this as unknown as WorkspaceLocalHost)[WORKSPACE] = new Workspace(options(self)); } - __getWorkspaceStub(): Promise { + // `options.writable: false` returns a stub that cannot write. + // This is how a command running without write access gets a + // handle that refuses writes rather than a handle it is merely + // asked not to use: the worker-shell backend passes the flag + // here, and the shell inside the Dynamic Worker never holds a + // writable capability at all. + __getWorkspaceStub( + options?: import("./stub.js").WorkspaceStubOptions, + ): Promise { const ws = (this as unknown as WorkspaceLocalHost)[WORKSPACE]; return (async () => { await ws.ready(); - return ws.stub(); + return ws.stub(options); })(); } } diff --git a/packages/computer/src/workspace.test.ts b/packages/computer/src/workspace.test.ts index 467d7fee..72540ed4 100644 --- a/packages/computer/src/workspace.test.ts +++ b/packages/computer/src/workspace.test.ts @@ -1,7 +1,14 @@ +import { WorkspaceFilesystem } from "@cloudflare/dofs"; import { SQLiteTestStorage } from "@cloudflare/dofs/testing"; import { describe, expect, it, vi } from "vitest"; import type { BackendHandle, WorkspaceBackend } from "./backend.js"; +import { + ActionDeniedError, + type WorkspaceAction, + type WorkspaceAudit, + type WorkspaceGate, +} from "./gate.js"; import { createGitClient } from "./git/index.js"; import type { WorkspaceModuleBackend } from "./runtime/types.js"; import { WorkspaceTransportError } from "./transport-failure.js"; @@ -1509,3 +1516,123 @@ describe("Workspace Think compatibility", () => { expect(ws).not.toHaveProperty("stat"); }); }); + +describe("Workspace gate — filesystem facade", () => { + function gateDenying(kinds: string[]): { + gate: WorkspaceGate; + seen: WorkspaceAction[]; + } { + const seen: WorkspaceAction[] = []; + return { + seen, + gate: { + check(action) { + seen.push(action); + return kinds.includes(action.kind) + ? { allow: false, reason: "denied by policy" } + : { allow: true }; + }, + }, + }; + } + + it("permits everything when no gate is configured", async () => { + const ws = new Workspace({ storage: makeStorage() }); + await ws.fs.writeFile("/a.txt", "hello"); + expect(await ws.fs.readFile("/a.txt", "utf8")).toBe("hello"); + }); + + it("refuses a write the gate denies, and does not write it", async () => { + // Workspace.fs writes to the local store without crossing the + // wire, so a gate that only covered shell.exec could be walked + // around by writing the file directly. + const { gate, seen } = gateDenying(["fs.write"]); + const ws = new Workspace({ storage: makeStorage(), gate }); + + await expect(ws.fs.writeFile("/blocked.txt", "nope")).rejects.toThrow(ActionDeniedError); + await expect(ws.fs.readFile("/blocked.txt", "utf8")).rejects.toThrow(); + expect(seen).toEqual([{ kind: "fs.write", path: "/blocked.txt", size: 4 }]); + }); + + it("leaves reads ungated", async () => { + const { gate, seen } = gateDenying(["fs.write", "fs.mkdir", "fs.rm"]); + const ws = new Workspace({ storage: makeStorage() }); + await ws.fs.writeFile("/readable.txt", "content"); + + const gated = new Workspace({ storage: makeStorage(), gate }); + await expect(gated.fs.readFile("/missing.txt", "utf8")).rejects.toThrow(); + // The read reached the store and failed there, not at the gate. + expect(seen).toEqual([]); + }); + + it("gates mkdir, rm, chmod and symlink", async () => { + const { gate, seen } = gateDenying([]); + const ws = new Workspace({ storage: makeStorage(), gate }); + + await ws.fs.mkdir("/dir"); + await ws.fs.writeFile("/dir/f.txt", "x"); + await ws.fs.chmod("/dir/f.txt", 0o600); + await ws.fs.symlink("/dir/f.txt", "/link"); + await ws.fs.rm("/dir", { recursive: true }); + + expect(seen.map((a) => a.kind)).toEqual([ + "fs.mkdir", + "fs.write", + "fs.chmod", + "fs.symlink", + "fs.rm", + ]); + }); + + it("gates a recursive rm once, on the path the caller named", async () => { + // Not once per descendant: a gate cannot usefully refuse half a + // tree, and asking it to would reintroduce the partial-delete + // problem the design avoids. + const { gate, seen } = gateDenying([]); + const ws = new Workspace({ storage: makeStorage(), gate }); + await ws.fs.mkdir("/tree/nested", { recursive: true }); + await ws.fs.writeFile("/tree/nested/a.txt", "a"); + await ws.fs.writeFile("/tree/nested/b.txt", "b"); + seen.length = 0; + + await ws.fs.rm("/tree", { recursive: true }); + + expect(seen).toEqual([{ kind: "fs.rm", path: "/tree" }]); + }); + + it("reports each mutation to the audit hook, including refusals", async () => { + const entries: [string, string][] = []; + const audit: WorkspaceAudit = { + record(action, outcome) { + entries.push([action.kind, outcome.status]); + }, + }; + const { gate } = gateDenying(["fs.rm"]); + const ws = new Workspace({ storage: makeStorage(), gate, audit }); + + await ws.fs.writeFile("/a.txt", "a"); + await ws.fs.rm("/a.txt").catch(() => {}); + + expect(entries).toEqual([ + ["fs.write", "allowed"], + ["fs.rm", "denied"], + ]); + }); + + it("stays a WorkspaceFilesystem so mount and think surfaces are unaffected", async () => { + const ws = new Workspace({ storage: makeStorage(), gate: gateDenying([]).gate }); + expect(ws.fs).toBeInstanceOf(WorkspaceFilesystem); + }); + + it("reports no size for a streamed write rather than a wrong one", async () => { + // Measuring the stream would consume the bytes the write needs. + const { gate, seen } = gateDenying([]); + const ws = new Workspace({ storage: makeStorage(), gate }); + const stream = new Blob(["streamed"]).stream(); + + await ws.fs.writeFile("/streamed.txt", stream); + + expect(seen).toEqual([{ kind: "fs.write", path: "/streamed.txt", size: undefined }]); + expect(await ws.fs.readFile("/streamed.txt", "utf8")).toBe("streamed"); + }); +}); diff --git a/packages/computer/src/workspace.ts b/packages/computer/src/workspace.ts index e75ada0f..3e2f3043 100644 --- a/packages/computer/src/workspace.ts +++ b/packages/computer/src/workspace.ts @@ -27,6 +27,8 @@ import { } from "./artifacts/index.js"; import type { AssetsClient } from "./assets/index.js"; import type { BackendHandle, WorkspaceBackend } from "./backend.js"; +import { noopAudit, openGate, type WorkspaceAudit, type WorkspaceGate } from "./gate.js"; +import { GatedWorkspaceFilesystem } from "./gated-fs.js"; import type { GitClient, GitClientFactory, GitIdentity } from "./git/index.js"; import { MountIndex } from "./mounts/index.js"; import { buildMountRegistry, type MountValue } from "./mounts/registry.js"; @@ -40,7 +42,7 @@ import { type WorkspaceRegisteredBackend, } from "./runtime/types.js"; import { WorkspaceShell } from "./shell.js"; -import { WorkspaceStub } from "./stub.js"; +import { WorkspaceStub, type WorkspaceStubOptions } from "./stub.js"; import { isWorkspaceTransportFailure } from "./transport-failure.js"; export interface SyncRetryIntent { @@ -61,6 +63,13 @@ export interface SyncRetryScheduler { clear(backend: string): Promise; } +export interface WorkspacePullOptions { + // Whether this pull may write to the local store. Defaults to true. + // A pull without write access applies nothing and reports every + // entry it was offered as skipped. + writable?: boolean; +} + export interface SyncRetryOptions { initialDelayMs?: number; maxDelayMs?: number; @@ -118,6 +127,21 @@ export interface WorkspaceOptions { // adapter subpaths for the Cloudflare runtime and OpenTelemetry. observer?: WorkspaceObserver; + // Consulted before each gated action — one call per shell.exec and + // one per mutating Workspace.fs call — and able to refuse it or to + // withdraw its write access. The default permits everything, so the + // package behaves exactly as before for callers who do not opt in. + // + // This is a separate seam from `observer` on purpose: an observer + // must not change what happens, and a gate exists to. See + // `./gate.ts`. + gate?: WorkspaceGate; + + // Notified after each gated action with what was decided and how it + // turned out, including the ones that were refused. Cannot deny + // anything; errors it throws are swallowed. + audit?: WorkspaceAudit; + // Optional durable retry boundary for failed post-command pulls. // The host persists one intent per backend and wakes the Durable // Object at intent.notBefore to call retryPendingSync(backend). @@ -218,6 +242,9 @@ export class Workspace { readonly #defaultBackendId: string | undefined; readonly #defaultCommandBackendId: string | undefined; readonly #observer: WorkspaceObserver; + readonly #gate: WorkspaceGate; + readonly #audit: WorkspaceAudit; + #readOnlyFs: WorkspaceFilesystem | undefined; readonly #now: () => number; readonly #waitUntil: ((promise: Promise) => void) | undefined; readonly #retryScheduler: SyncRetryScheduler | undefined; @@ -299,7 +326,17 @@ export class Workspace { : createDisabledArtifactsClient(); this.#db = new Database(options.storage); initializeSchema(this.#db, this.#now); - this.#fs = new WorkspaceFilesystem(this.#db, { now: this.#now }); + this.#gate = options.gate ?? openGate; + this.#audit = options.audit ?? noopAudit; + // The gated subclass is still a WorkspaceFilesystem, so the mount + // index, the think surface, and every caller holding `fs` are + // unaffected. With the default open gate the override adds one + // comparison per mutation. + this.#fs = new GatedWorkspaceFilesystem(this.#db, { + now: this.#now, + gate: this.#gate, + audit: this.#audit, + }); const registered = (options.backends ?? []).slice(); if (registered.some((backend) => isModuleBackend(backend) && backend.requiresWaitUntil)) { if (!options.waitUntil) { @@ -376,6 +413,17 @@ export class Workspace { return this.#observer; } + // Gate and audit hook, exposed for the same reason as the observer: + // the shell facade gates its own entry point and needs the pair the + // constructor was given rather than a second set of defaults. + get gate(): WorkspaceGate { + return this.#gate; + } + + get audit(): WorkspaceAudit { + return this.#audit; + } + // Filesystem facade — the documented Workspace.fs surface from // docs/04. Available immediately; doesn't need ready() because // reads and writes hit the local store, not the wire. @@ -386,6 +434,12 @@ export class Workspace { // wrapper. The same check fires on the apply path used by // pullOnce, so container-side writes under a read-only mount are // also rejected (and surfaced via Workspace.pull's skipped[]). + // + // The handle is a GatedWorkspaceFilesystem, so mutations also pass + // the configured gate. Mount enforcement and the gate answer + // different questions — a mount root is a fixed property of the + // workspace, a gate is a decision per action — and both apply. + // Neither can be bypassed by going through the other. get fs(): WorkspaceFilesystem { return this.#fs; } @@ -528,8 +582,31 @@ export class Workspace { // across the Workers-RPC boundary (e.g. returned from a DO RPC // method). The stub is a lazy RpcTarget — it doesn't own any // resources itself; it just delegates back to this workspace. - stub(): WorkspaceStub { - return new WorkspaceStub(this); + stub(options?: WorkspaceStubOptions): WorkspaceStub { + return new WorkspaceStub(this, options); + } + + // Filesystem handle for a given write access. The writable case is + // the workspace's own gated handle; the read-only case is a second + // handle over the same store built without the capability. + // + // Two handles over one database is the point: commands overlap, and + // a read-only one must not be able to disarm a writable one running + // beside it. The capability lives on the handle for exactly that + // reason — it cannot be a property of the database without one + // command's access leaking into another's. It also cannot be a + // second Database, because dofs assumes exactly one Database wraps + // each SqlStorage and its resolve cache depends on that. + // + // The read-only handle is built once and shared. It holds no + // per-command state, and every caller wants the same thing from it. + fsWithAccess(writable: boolean): WorkspaceFilesystem { + if (writable) return this.#fs; + this.#readOnlyFs ??= new WorkspaceFilesystem(this.#db, { + now: this.#now, + writable: false, + }); + return this.#readOnlyFs; } // Sync the local store with a configured backend. @@ -549,7 +626,8 @@ export class Workspace { // pull() returns the dofs ApplyResult { applied, skipped } — // `applied` is the number of entries written into the local // store, `skipped` surfaces remote-side writes the apply path - // rejected because they targeted a read-only mount root. + // rejected, either because they targeted a read-only mount root + // or because the pull ran without write access. // // Both methods emit a `workspace.sync.push` / `workspace.sync.pull` // span on the configured observer, tagged with the resolved @@ -580,8 +658,15 @@ export class Workspace { ); } - pull(id?: string): Promise { - return this.#serialize(id, (resolvedId) => this.#pullResolved(resolvedId)); + // `writable: false` pulls without write access: nothing is + // applied and every entry comes back in `skipped`. This is how a + // command that ran without write access has its changes refused + // when the backend keeps its own copy of the files and wrote to + // that copy before we heard about it. + pull(id?: string, options?: WorkspacePullOptions): Promise { + return this.#serialize(id, (resolvedId) => + this.#pullResolved(resolvedId, options?.writable ?? true), + ); } /** @@ -612,7 +697,7 @@ export class Workspace { }; } try { - const result = await this.#pullResolved(resolvedId); + const result = await this.#pullResolved(resolvedId, true); await scheduler.clear(resolvedId); return { status: "complete", @@ -637,11 +722,11 @@ export class Workspace { }); } - #pullResolved(resolvedId: string | undefined): Promise { + #pullResolved(resolvedId: string | undefined, writable: boolean): Promise { return withSpan( this.#observer, "workspace.sync.pull", - { "workspace.sync.backend": resolvedId }, + { "workspace.sync.backend": resolvedId, "workspace.sync.writable": writable }, async () => { if (resolvedId === undefined || this.#moduleBackendsById.has(resolvedId)) { return { applied: 0, skipped: [] }; @@ -649,7 +734,7 @@ export class Workspace { const handle = await this.#handleFor(resolvedId); if (handle.sync === "none") return { applied: 0, skipped: [] }; return this.#runWithInvalidation(resolvedId, handle, () => - pullOnce(this.#db, handle.rpc.sync, resolvedId), + pullOnce(this.#db, handle.rpc.sync, resolvedId, { writable }), ); }, (span, outcome) => { @@ -927,10 +1012,11 @@ export class Workspace { handle.rpc.shell, { push: () => this.push(id), - pull: () => this.pull(id), + pull: (options) => this.pull(id, options), onPullPending: () => this.#schedulePendingSync(id), }, this.#observer, + { gate: this.#gate, audit: this.#audit }, ); this.#shells.set(id, shell); return { shell, handle }; @@ -1026,12 +1112,19 @@ class WorkspaceShellRouter { // swapped in a newer handle that we must not clobber. const { shell, handle: dispatchHandle } = await this.#shellFor(id); const { backend: _backend, ...rest } = options; + // The caller's selector is replaced with the id it resolved to, + // rather than dropped. The per-backend shell does not need it to + // route — it is already the right shell — but the gate is handed + // this and a gate deciding whether to trust a command with write + // access wants to know which backend will run it, since that is + // what determines whether a refused write is prevented or merely + // reported. + const forwarded = { ...rest, backend: id }; let execHandle: unknown; try { - execHandle = await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( - command, - rest, - ); + execHandle = await ( + shell.exec as unknown as (c: string, o: typeof forwarded) => Promise + )(command, forwarded); } catch (error) { this.#onError(id, dispatchHandle, error); throw error; diff --git a/packages/computerd/src/fuse/driver.test.ts b/packages/computerd/src/fuse/driver.test.ts index 97f377b3..06fa9e1b 100644 --- a/packages/computerd/src/fuse/driver.test.ts +++ b/packages/computerd/src/fuse/driver.test.ts @@ -1,7 +1,11 @@ +import { invalidateReadOnlyMountCache } from "@cloudflare/dofs"; import { expect, test } from "vitest"; import { createNodeVirtualFileSystem, makeFUSEOps } from "./index.js"; +const EROFS = -30; +const EACCES = -13; + const callback = (fn: (cb: (errno: number, result: unknown) => void) => void) => new Promise<{ errno: number; result: unknown }>((resolve) => fn((errno, result) => resolve({ errno, result })), @@ -1044,3 +1048,33 @@ test("FUSE ftruncate does not depend on fuse-native binding this", async () => { expect(await status((cb) => ops.flush("/ftruncate-this.txt", fh, cb))).toBe(0); expect(Buffer.from(vfs.readFileSync("/ftruncate-this.txt")).toString("utf8")).toBe("abc"); }); + +// A write into a read-only mount root is refused by dofs with EROFS. +// The guest has to see that code: EROFS says "this tree does not take +// writes", which is a fact about the workspace the caller can act on, +// while EIO says the daemon or the disk is broken, which invites a +// retry that will fail the same way forever. +test("FUSE reports a refused write into a read-only mount as EROFS", async () => { + const { vfs, db } = await createNodeVirtualFileSystem(); + vfs.mkdirSync("/ro", { recursive: true }); + db.run("INSERT INTO _vfs_mounts (root, kind, mode) VALUES (?, ?, ?)", "/ro", "r2", "read-only"); + invalidateReadOnlyMountCache(db); + const ops = makeFUSEOps(vfs); + + expect(await status((cb) => ops.mkdir("/ro/nested", 0o755, cb))).toBe(EROFS); + expect(await status((cb) => ops.unlink("/ro/gone.txt", cb))).toBe(EROFS); +}); + +// Same reasoning for a path the caller is not permitted to touch: +// EACCES is actionable, EIO is not. +test("FUSE preserves EACCES rather than flattening it to EIO", async () => { + const { vfs } = await createNodeVirtualFileSystem(); + const denied = Object.assign(Object.create(Object.getPrototypeOf(vfs)), vfs, { + mkdirSync() { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }); + }, + }); + const ops = makeFUSEOps(denied); + + expect(await status((cb) => ops.mkdir("/denied", 0o755, cb))).toBe(EACCES); +}); diff --git a/packages/computerd/src/fuse/driver.ts b/packages/computerd/src/fuse/driver.ts index afefd202..89a89a73 100644 --- a/packages/computerd/src/fuse/driver.ts +++ b/packages/computerd/src/fuse/driver.ts @@ -13,6 +13,8 @@ const ERRNO = { EISDIR: -21, EINVAL: -22, EPERM: -1, + EACCES: -13, + EROFS: -30, EFBIG: -27, ENOTEMPTY: -39, ENODATA: -61, @@ -1078,6 +1080,13 @@ function statNode(stat: { }; } +// Falling back to EIO is right for an error this layer does not +// recognise, and wrong for a refusal. A refusal is a fact about the +// workspace that the caller can act on — the tree does not take +// writes, or this caller may not make them — while EIO reads as a +// broken daemon and invites a retry that fails identically. dofs +// raises EROFS for both a read-only mount root and a handle without +// the write capability, so those have to survive the translation. function toErrno(error: unknown): number { const code = typeof error === "object" && error !== null && "code" in error ? String(error.code) : undefined; @@ -1088,5 +1097,7 @@ function toErrno(error: unknown): number { if (code === "ENOTEMPTY") return ERRNO.ENOTEMPTY; if (code === "EINVAL") return ERRNO.EINVAL; if (code === "EPERM") return ERRNO.EPERM; + if (code === "EACCES") return ERRNO.EACCES; + if (code === "EROFS") return ERRNO.EROFS; return ERRNO.EIO; } diff --git a/packages/dofs/README.md b/packages/dofs/README.md index 8eacbf4a..a8a3e13e 100644 --- a/packages/dofs/README.md +++ b/packages/dofs/README.md @@ -59,3 +59,12 @@ export class WorkspaceDO extends DurableObject { Repeated reads of dedup'd chunks (a file of zeroes, a re-used package payload) skip SQLite after the first fetch. - Sync protocol building blocks implemented and exported; the typed RPC surface on top of them lives in `@cloudflare/computer-rpc`. +- Two independent write guards, both reporting `EROFS`. A registered + read-only mount root is enforced next to the stored tree, so writes + arriving through the sync apply path are caught as well as direct + ones. A write capability is enforced on the handle: build a + `WorkspaceFilesystem` or `SQLiteWorkspaceProvider` with + `writable: false` and every mutating method on it refuses. The + capability is fixed for the life of the handle, which is what lets a + caller run one whole command without write access instead of + stopping it part-way through. diff --git a/packages/dofs/src/fs/filesystem.ts b/packages/dofs/src/fs/filesystem.ts index 82ea588c..ae505776 100644 --- a/packages/dofs/src/fs/filesystem.ts +++ b/packages/dofs/src/fs/filesystem.ts @@ -19,6 +19,7 @@ import { find, type WorkspaceFoundEntry } from "./find.js"; import { type GrepOptions, grep, type WorkspaceGrepMatch } from "./grep.js"; import { ls } from "./ls.js"; import { type MkdirOptions, mkdir } from "./mkdir.js"; +import { assertWritable } from "./mount-guard.js"; import { type ReaddirOptions, readdir, type WorkspaceDirentResult } from "./readdir.js"; import { type ReadFileOptions, readFile } from "./readFile.js"; import { readlink } from "./readlink.js"; @@ -31,15 +32,28 @@ export interface WorkspaceFilesystemOptions { // Clock used for mtime / last_seen. Defaults to Date.now. // Override for deterministic tests. now?: () => number; + + // Write access for this handle. Defaults to true. Pass false to + // get a handle whose mutating methods all reject with EROFS, + // which is how a single command runs without write access. + // + // The capability belongs to the handle, not to the database, so + // two handles over one database can disagree. That matters + // because commands overlap: a read-only command must not be able + // to disarm a writable one running beside it, and a writable one + // must not lend its access to a read-only one. + writable?: boolean; } export class WorkspaceFilesystem { readonly db: Database; readonly now: () => number; + readonly writable: boolean; constructor(db: Database, options: WorkspaceFilesystemOptions = {}) { this.db = db; this.now = options.now ?? Date.now; + this.writable = options.writable ?? true; } // --- Reads ------------------------------------------------------- @@ -96,19 +110,26 @@ export class WorkspaceFilesystem { // --- Mutations --------------------------------------------------- - writeFile( + // Declared async so a refusal arrives as a rejected promise. The + // guard throws, and a plain `return writeFile(...)` would let that + // throw escape synchronously out of a method whose signature + // promises otherwise, which a caller using `.catch()` would miss. + async writeFile( path: string, content: WriteFileContent, options: WriteFileOptions = {}, ): Promise { + assertWritable(this, path); return writeFile(this.db, path, content, options, this.now); } async mkdir(path: string, options: MkdirOptions = {}): Promise { + assertWritable(this, path); mkdir(this.db, path, options, this.now); } async rm(path: string, options: RmOptions = {}): Promise { + assertWritable(this, path); rm(this.db, path, options); } @@ -116,6 +137,7 @@ export class WorkspaceFilesystem { // POSIX chmod — the change lands on the target, not the link. // The supplied mode is masked to twelve bits. async chmod(path: string, mode: number): Promise { + assertWritable(this, path); chmod(this.db, path, mode, this.now); } @@ -123,6 +145,7 @@ export class WorkspaceFilesystem { // target is stored verbatim; it can be relative or absolute and // is allowed to dangle. async symlink(target: string, path: string): Promise { + assertWritable(this, path); symlink(this.db, target, path, this.now); } } diff --git a/packages/dofs/src/fs/mount-guard.ts b/packages/dofs/src/fs/mount-guard.ts index 6a2aadd3..ad32882d 100644 --- a/packages/dofs/src/fs/mount-guard.ts +++ b/packages/dofs/src/fs/mount-guard.ts @@ -1,11 +1,30 @@ -// Read-only mount guard. +// Write guard. // -// Every dofs mutating entry point (writeFile, mkdir, rm, and the -// apply path in sync/apply.ts) consults this module to reject -// writes that fall under a registered read-only mount root. The -// guard lives at the data layer so container-side writes that -// arrive via pullOnce -> applyChanges are caught too — the -// workspace-side surface wrapper alone cannot see them. +// Two separate things can refuse a write, and both report EROFS. +// +// The first is a read-only mount root. Every dofs mutating entry +// point (writeFile, mkdir, rm, and the apply path in sync/apply.ts) +// consults this module to reject writes that fall under a +// registered read-only mount root. That check lives at the data +// layer so container-side writes arriving via pullOnce -> +// applyChanges are caught too, which a surface wrapper alone +// cannot see. +// +// The second is a write capability, and it belongs to whoever holds +// the filesystem handle rather than to the data. A caller that +// builds a WorkspaceFilesystem or SQLiteWorkspaceProvider with +// `writable: false` gets a handle that refuses every mutation for +// as long as it exists. The point is to run one command without +// write access: a command is not atomic, so deciding per write +// would let the first forty writes land before the forty-first is +// refused. A capability fixed for the handle's lifetime cannot +// produce that state. +// +// Each check sits at the layer that owns its state. The mount mode +// is a property of the stored tree, so it is enforced next to the +// tree. The capability is a property of the handle, so it is +// enforced on the handle's own methods, where the flag is +// immutable and cannot be flipped part-way through a write. // // The set of read-only roots is small (one row per registered // mount per workspace, typically <10) and changes only at indexer @@ -17,6 +36,34 @@ import { createWorkspaceError } from "../errors.js"; import type { Database } from "../storage.js"; +/** + * A handle's write access. `WorkspaceFilesystem` and + * `SQLiteWorkspaceProvider` both satisfy this, so they can pass + * themselves to `assertWritable`. + * + * The field is deliberately immutable on both. A mutable flag would + * reopen the window the capability exists to close: `writeFile` + * checks once and then awaits its source stream many times before + * committing, so a flag that could change mid-call would let a write + * that was refused on entry still land. + */ +export interface WriteCapability { + readonly writable: boolean; +} + +/** + * Throws EROFS when the handle has no write access. Call this before + * any mutation on a surface that carries a capability. + * + * The message is distinct from the mount-root message below so a + * caller reading `error.message` can tell a command that was denied + * write access from a command that reached into a read-only mount. + */ +export function assertWritable(capability: WriteCapability, path: string): void { + if (capability.writable) return; + throw createWorkspaceError("EROFS", "read-only access: cannot modify", path); +} + // undefined sentinel = "not loaded yet"; an empty array means // "loaded, no read-only mounts registered". The two are not the // same: the empty case must skip the SQL lookup on every check. diff --git a/packages/dofs/src/fs/write-capability.test.ts b/packages/dofs/src/fs/write-capability.test.ts new file mode 100644 index 00000000..306f16da --- /dev/null +++ b/packages/dofs/src/fs/write-capability.test.ts @@ -0,0 +1,226 @@ +import { describe, expect, it } from "vitest"; + +import { SQLiteWorkspaceProvider } from "../provider.js"; +import { WorkspaceFilesystem } from "./filesystem.js"; +import { withDB } from "./with-db.js"; + +const clock = () => 1000; + +describe("WorkspaceFilesystem write capability", () => { + it("defaults to writable so existing callers are unaffected", async () => { + await withDB(async (db) => { + const fs = new WorkspaceFilesystem(db, { now: clock }); + + await fs.mkdir("/workspace", { recursive: true }); + await fs.writeFile("/workspace/hello.txt", "hi"); + + expect(await fs.readFile("/workspace/hello.txt", "utf8")).toBe("hi"); + }); + }); + + it("rejects writeFile with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.writeFile("/workspace/hello.txt", "hi")).rejects.toMatchObject({ + code: "EROFS", + }); + + await expect(writable.readFile("/workspace/hello.txt", "utf8")).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + }); + + it("rejects mkdir with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + await expect(readOnly.mkdir("/workspace", { recursive: true })).rejects.toMatchObject({ + code: "EROFS", + }); + }); + }); + + it("rejects rm with EROFS on a read-only handle and leaves the file in place", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + await writable.writeFile("/workspace/keep.txt", "still here"); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.rm("/workspace/keep.txt", {})).rejects.toMatchObject({ code: "EROFS" }); + + expect(await writable.readFile("/workspace/keep.txt", "utf8")).toBe("still here"); + }); + }); + + it("rejects chmod with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + await writable.writeFile("/workspace/script.sh", "echo hi"); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.chmod("/workspace/script.sh", 0o755)).rejects.toMatchObject({ + code: "EROFS", + }); + }); + }); + + it("rejects symlink with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.symlink("/workspace", "/link")).rejects.toMatchObject({ + code: "EROFS", + }); + }); + }); + + it("serves every read from a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace/sub", { recursive: true }); + await writable.writeFile("/workspace/a.txt", "alpha"); + await writable.writeFile("/workspace/sub/b.txt", "beta"); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + expect(await readOnly.readFile("/workspace/a.txt", "utf8")).toBe("alpha"); + expect((await readOnly.stat("/workspace/a.txt")).size).toBe(5); + expect((await readOnly.readdir("/workspace")).map((e) => e.name).sort()).toEqual([ + "a.txt", + "sub", + ]); + expect(await readOnly.ls("/workspace")).toContain("/workspace/a.txt"); + expect(await readOnly.find("/workspace", "*.txt")).toEqual([ + { path: "/workspace/a.txt", type: "file" }, + ]); + expect(await readOnly.grep("beta", "/workspace")).toHaveLength(1); + }); + }); + + // Two commands can be in flight against one workspace at the same + // time, and they do not have to agree about write access. The + // capability belongs to the handle rather than to the database, so + // a read-only command cannot disarm a concurrent writable one, and + // a writable command cannot lend its access to a read-only one. + it("leaves a writable handle over the same database unaffected", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + await writable.mkdir("/workspace", { recursive: true }); + await expect(readOnly.writeFile("/workspace/denied.txt", "no")).rejects.toMatchObject({ + code: "EROFS", + }); + + await writable.writeFile("/workspace/allowed.txt", "yes"); + expect(await readOnly.readFile("/workspace/allowed.txt", "utf8")).toBe("yes"); + }); + }); + + it("names the path it refused", async () => { + await withDB(async (db) => { + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + await expect(readOnly.writeFile("/workspace/hello.txt", "hi")).rejects.toMatchObject({ + code: "EROFS", + path: "/workspace/hello.txt", + }); + }); + }); +}); + +describe("SQLiteWorkspaceProvider write capability", () => { + it("advertises the capability through the readonly flag", async () => { + await withDB(async (db) => { + expect(new SQLiteWorkspaceProvider(db, { now: clock }).readonly).toBe(false); + expect(new SQLiteWorkspaceProvider(db, { now: clock, writable: false }).readonly).toBe(true); + }); + }); + + it("rejects every mutating call with EROFS on a read-only provider", async () => { + await withDB(async (db) => { + const writable = new SQLiteWorkspaceProvider(db, { now: clock }); + writable.mkdirSync("/workspace", { recursive: true }); + writable.writeFileSync("/workspace/a.txt", "alpha"); + writable.mkdirSync("/workspace/dir", { recursive: true }); + + const readOnly = new SQLiteWorkspaceProvider(db, { now: clock, writable: false }); + const refused = (run: () => unknown) => + expect(run).toThrowError(expect.objectContaining({ code: "EROFS" })); + + refused(() => readOnly.writeFileSync("/workspace/b.txt", "beta")); + refused(() => readOnly.writeFileRangesSync("/workspace/a.txt", "x", [{ start: 0, end: 1 }])); + refused(() => readOnly.mkdirSync("/workspace/other", { recursive: true })); + refused(() => readOnly.rmdirSync("/workspace/dir")); + refused(() => readOnly.unlinkSync("/workspace/a.txt")); + refused(() => readOnly.renameSync("/workspace/a.txt", "/workspace/moved.txt")); + refused(() => readOnly.linkSync("/workspace/a.txt", "/workspace/hard.txt")); + refused(() => readOnly.symlinkSync("/workspace", "/link")); + refused(() => readOnly.truncateSync("/workspace/a.txt", 0)); + refused(() => readOnly.createFileSync("/workspace/c.txt")); + refused(() => readOnly.writeRangeSync("/workspace/a.txt", new Uint8Array([1]), 0)); + refused(() => readOnly.truncateFileSync("/workspace/a.txt", 0)); + refused(() => readOnly.openSync("/workspace/new.txt", "w")); + + // These four reached the database without consulting the guard + // before the capability existed. A read-only command drives all + // of them through a FUSE mount, so they are the ones worth + // naming. + refused(() => readOnly.chmodSync("/workspace/a.txt", 0o755)); + refused(() => readOnly.openWriteBufferSync("/workspace/a.txt")); + refused(() => readOnly.openWriteBufferForCreateSync("/workspace/d.txt")); + refused(() => readOnly.releaseWriteBufferSync("/workspace/a.txt")); + + // Nothing landed. + expect(writable.readFileSync("/workspace/a.txt", "utf8")).toBe("alpha"); + expect(writable.existsSync("/workspace/b.txt")).toBe(false); + expect(writable.existsSync("/workspace/dir")).toBe(true); + }); + }); + + // Opening for write on a read-only filesystem fails at the open, + // not later on the first write, so a read-only provider never hands + // out a writable descriptor. The write path is guarded too, which + // covers a descriptor obtained some other way. + it("refuses to open a file for writing", async () => { + await withDB(async (db) => { + const writable = new SQLiteWorkspaceProvider(db, { now: clock }); + writable.mkdirSync("/workspace", { recursive: true }); + writable.writeFileSync("/workspace/a.txt", "alpha"); + + const readOnly = new SQLiteWorkspaceProvider(db, { now: clock, writable: false }); + for (const flags of ["w", "a", "r+", "w+"]) { + expect(() => readOnly.openSync("/workspace/a.txt", flags)).toThrowError( + expect.objectContaining({ code: "EROFS" }), + ); + } + + expect(writable.readFileSync("/workspace/a.txt", "utf8")).toBe("alpha"); + }); + }); + + it("serves reads from a read-only provider", async () => { + await withDB(async (db) => { + const writable = new SQLiteWorkspaceProvider(db, { now: clock }); + writable.mkdirSync("/workspace", { recursive: true }); + writable.writeFileSync("/workspace/a.txt", "alpha"); + + const readOnly = new SQLiteWorkspaceProvider(db, { now: clock, writable: false }); + + expect(readOnly.readFileSync("/workspace/a.txt", "utf8")).toBe("alpha"); + expect(readOnly.statSync("/workspace/a.txt").size).toBe(5); + expect(readOnly.readdirSync("/workspace")).toEqual(["a.txt"]); + expect(readOnly.existsSync("/workspace/a.txt")).toBe(true); + const fd = readOnly.openSync("/workspace/a.txt", "r"); + readOnly.closeSync(fd); + }); + }); +}); diff --git a/packages/dofs/src/index.ts b/packages/dofs/src/index.ts index 76ef05a5..1d4c148b 100644 --- a/packages/dofs/src/index.ts +++ b/packages/dofs/src/index.ts @@ -9,16 +9,26 @@ export type { WorkspaceFoundEntry } from "./fs/find.js"; export type { GrepOptions, WorkspaceGrepMatch } from "./fs/grep.js"; export { link } from "./fs/link.js"; export type { MkdirOptions } from "./fs/mkdir.js"; -// Read-only mount enforcement. The workspace-side indexer writes -// _vfs_mounts; the helpers here let it invalidate the in-Database -// cache after a write, and let dofs callers (and tests) inspect or -// assert against the registered roots without re-implementing the -// overlap check. +// Write enforcement. Two things refuse a write, and both report +// EROFS: a registered read-only mount root, and a handle built +// without write access. +// +// For mounts, the workspace-side indexer writes _vfs_mounts; the +// helpers here let it invalidate the in-Database cache after a write, +// and let dofs callers (and tests) inspect or assert against the +// registered roots without re-implementing the overlap check. +// +// For handles, `assertWritable` is what `WorkspaceFilesystem` and +// `SQLiteWorkspaceProvider` call on every mutation, and +// `WriteCapability` is the shape any other surface can adopt to join +// in. export { assertNotReadOnly, + assertWritable, getReadOnlyMountRoots, invalidateReadOnlyMountCache, readOnlyRootFor, + type WriteCapability, } from "./fs/mount-guard.js"; export type { ReaddirOptions, WorkspaceDirentResult } from "./fs/readdir.js"; export type { ReadFileOptions } from "./fs/readFile.js"; @@ -31,7 +41,13 @@ export type { SQLiteWorkspaceProviderOptions } from "./provider.js"; export { SQLiteWorkspaceProvider } from "./provider.js"; export { initializeSchema, ROOT_INODE, SCHEMA_VERSION } from "./schema/index.js"; export { Database } from "./storage.js"; -export type { ApplyOptions, ApplyResult, SkippedEntry } from "./sync/apply.js"; +export type { + ApplyOptions, + ApplyResult, + SkippedByMount, + SkippedEntry, + SkippedWithoutWriteAccess, +} from "./sync/apply.js"; // Sync protocol building blocks. The wire wiring lives in // @cloudflare/computer-rpc; these are the helpers that wiring binds // to a Database. diff --git a/packages/dofs/src/provider.ts b/packages/dofs/src/provider.ts index 5adcafc8..a44d08da 100644 --- a/packages/dofs/src/provider.ts +++ b/packages/dofs/src/provider.ts @@ -12,6 +12,7 @@ import { getBlobBytes } from "./fs/blobCache.js"; import { link as linkImpl } from "./fs/link.js"; import type { MkdirOptions } from "./fs/mkdir.js"; import { mkdir as mkdirImpl } from "./fs/mkdir.js"; +import { assertWritable } from "./fs/mount-guard.js"; import { readdir as readdirImpl } from "./fs/readdir.js"; import { readRangeSync as readRangeSyncImpl } from "./fs/readFile.js"; import { readlink as readlinkImpl } from "./fs/readlink.js"; @@ -56,6 +57,12 @@ export interface SQLiteWorkspaceProviderOptions { // to match node's fs.watch on most filesystems; tests can lower // it to keep durations short. watchIntervalMs?: number; + // Write access for this provider. Defaults to true. Pass false to + // get a provider whose mutating calls all reject with EROFS, which + // is how a single command runs without write access. The flag also + // shows up as `readonly` for @platformatic/vfs callers that ask + // before they write. + writable?: boolean; } interface VirtualStatsLike { @@ -113,8 +120,13 @@ export class SQLiteWorkspaceProvider { readonly db: Database; readonly now: () => number; + // Write access for this provider, fixed at construction. Every + // mutating method checks it, so a `writable: false` provider + // refuses the whole command rather than part of it. + readonly writable: boolean; + // Capability flags consulted by @platformatic/vfs callers. - readonly readonly = false; + readonly readonly: boolean; readonly supportsSymlinks = true; readonly supportsWatch = true; @@ -130,6 +142,8 @@ export class SQLiteWorkspaceProvider { this.db = db; this.now = options.now ?? Date.now; this.watchIntervalMs = options.watchIntervalMs ?? 100; + this.writable = options.writable ?? true; + this.readonly = !this.writable; } // -- Essential primitives ------------------------------------------ @@ -140,6 +154,10 @@ export class SQLiteWorkspaceProvider { openSync(path: string, flags: string = "r", _mode?: number): number { const { read, write, truncate, append, create, exclusive } = parseFlags(flags); + // A read-only provider refuses to hand out a writable + // descriptor at all, the way opening for write on a read-only + // filesystem fails rather than failing later on the first write. + if (write || truncate || create) assertWritable(this, path); const existing = resolveInode(this.db, path); if (existing === null) { @@ -253,6 +271,7 @@ export class SQLiteWorkspaceProvider { } mkdirSync(path: string, options?: MkdirOptions): string | undefined { + assertWritable(this, path); mkdirImpl(this.db, path, options ?? {}, this.now); return undefined; } @@ -263,6 +282,7 @@ export class SQLiteWorkspaceProvider { } rmdirSync(path: string): void { + assertWritable(this, path); rmImpl(this.db, path, {}); } @@ -272,6 +292,7 @@ export class SQLiteWorkspaceProvider { } unlinkSync(path: string): void { + assertWritable(this, path); // If a buffered create is still pending for this path, commit // it first so rm sees a real inode to unlink (and so the // resulting GC sees the orphaned blob, matching the non-buffered @@ -301,6 +322,7 @@ export class SQLiteWorkspaceProvider { } linkSync(existingPath: string, newPath: string): void { + assertWritable(this, newPath); // Commit a still-pending source before adding the second dirent, // otherwise link has nothing real to point at. Also commit a // still-pending destination: link's existence check looks at @@ -319,6 +341,8 @@ export class SQLiteWorkspaceProvider { } renameSync(oldPath: string, newPath: string): void { + assertWritable(this, oldPath); + assertWritable(this, newPath); // Commit any still-pending creates at either end before the rename // touches dirents: the source needs a real inode to move, and a // pending buffer at the destination would otherwise slip past @@ -411,6 +435,7 @@ export class SQLiteWorkspaceProvider { data: string | Buffer, options?: { encoding?: BufferEncoding; mode?: number } | BufferEncoding, ): void { + assertWritable(this, path); const mode = typeof options === "string" ? undefined : options?.mode; const bytes = typeof data === "string" @@ -425,6 +450,7 @@ export class SQLiteWorkspaceProvider { ranges: WriteFileRange[], options?: { encoding?: BufferEncoding; mode?: number } | BufferEncoding, ): void { + assertWritable(this, path); const mode = typeof options === "string" ? undefined : options?.mode; const bytes = typeof data === "string" @@ -434,6 +460,7 @@ export class SQLiteWorkspaceProvider { } createFileSync(path: string, options?: { mode?: number }): void { + assertWritable(this, path); createFileSyncImpl(this.db, path, { mode: options?.mode }, this.now); } @@ -443,6 +470,7 @@ export class SQLiteWorkspaceProvider { offset: number, options?: { encoding?: BufferEncoding; mode?: number } | BufferEncoding, ): number { + assertWritable(this, path); const mode = typeof options === "string" ? undefined : options?.mode; const bytes = typeof data === "string" @@ -452,22 +480,27 @@ export class SQLiteWorkspaceProvider { } truncateFileSync(path: string, len: number): void { + assertWritable(this, path); truncateFileSyncImpl(this.db, path, len, this.now); } openWriteBufferSync(path: string): void { + assertWritable(this, path); openWriteBufferSyncImpl(this.db, path); } openWriteBufferForCreateSync(path: string, options?: { mode?: number }): void { + assertWritable(this, path); openWriteBufferForCreateSyncImpl(this.db, path, { mode: options?.mode }, this.now); } releaseWriteBufferSync(path: string): void { + assertWritable(this, path); releaseWriteBufferSyncImpl(this.db, path, this.now); } chmodSync(path: string, mode: number): void { + assertWritable(this, path); const { path: canonical } = canonicalizePath(path); const pending = getPendingWriteBufferByPath(this.db, canonical); if (pending !== undefined) { @@ -605,6 +638,11 @@ export class SQLiteWorkspaceProvider { if (!state.writable) { throw createWorkspaceError("EBADF", `fd ${fd} is not writable`); } + // A read-only provider refuses to open for write, so it should + // never hold a writable descriptor to begin with. Check anyway: + // if some later change adds another way to mint one, the write + // still stops here rather than reaching the database. + assertWritable(this, state.path); // Append needs the current EOF, so stat only then. A non-append // write of >0 bytes doesn't need it: writeRangeSyncImpl resolves the // path and raises ENOENT/EISDIR. A zero-length write short-circuits @@ -635,6 +673,7 @@ export class SQLiteWorkspaceProvider { } truncateSync(path: string, len: number): void { + assertWritable(this, path); const node = resolveInode(this.db, path); if (node === null) { throw createWorkspaceError("ENOENT", `no such path: ${path}`, path); @@ -674,6 +713,7 @@ export class SQLiteWorkspaceProvider { } symlinkSync(target: string, path: string, _type?: string): void { + assertWritable(this, path); symlinkImpl(this.db, target, path, this.now); } diff --git a/packages/dofs/src/sync/apply.test.ts b/packages/dofs/src/sync/apply.test.ts index 5ced0d0e..c5bb5662 100644 --- a/packages/dofs/src/sync/apply.test.ts +++ b/packages/dofs/src/sync/apply.test.ts @@ -821,7 +821,12 @@ describe("applyChanges with read-only mount roots", () => { ); expect(result.applied).toBe(0); - expect(result.skipped[0]?.mountRoot).toBe("/workspace/r2"); + expect(result.skipped[0]).toEqual({ + path: "/workspace/r2", + mountRoot: "/workspace/r2", + op: "write", + reason: "read-only", + }); }); }); @@ -1118,3 +1123,140 @@ describe("applyChanges mtime propagation (auto_cache contract)", () => { ); }); }); + +// An apply that runs without write access is how a read-only command +// is enforced for a backend that keeps its own copy of the files. The +// command has already written to that copy by the time the changes +// reach here, so this is where they stop. Unlike the worker-shell +// lane, this is after the fact rather than preventive, and the skipped +// entries are what tells the caller so. +describe("applyChanges without write access", () => { + const fileEntry = (path: string, rev: number): ChangeEntry => ({ + kind: "file", + rev, + path, + mode: 0o644, + mtime: 1, + size: 0, + chunks: [], + }); + + it("applies nothing and reports every entry it was handed", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + + const result = await applyChanges( + db, + [ + fileEntry("/workspace/a.txt", 100), + { kind: "dir", rev: 101, path: "/workspace/sub", mode: 0o755, mtime: 1 }, + { kind: "delete", rev: 102, path: "/workspace/gone.txt" }, + ], + new Map(), + { writable: false }, + ); + + expect(result.applied).toBe(0); + expect(result.skipped).toEqual([ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + { path: "/workspace/sub", op: "write", reason: "no-write-access" }, + { path: "/workspace/gone.txt", op: "delete", reason: "no-write-access" }, + ]); + + expect(resolveInode(db, "/workspace/a.txt")).toBeNull(); + expect(resolveInode(db, "/workspace/sub")).toBeNull(); + }); + }); + + // A zero-chunk entry would truncate the file it lands on, so the + // original contents surviving is proof the apply did not run. + it("leaves a file the entry would have truncated untouched", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + writeFileSync(db, "/workspace/a.txt", new TextEncoder().encode("original"), {}, () => 0); + const revBefore = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'"); + + const result = await applyChanges(db, [fileEntry("/workspace/a.txt", 200)], new Map(), { + writable: false, + }); + + expect(result.applied).toBe(0); + expect(result.skipped).toHaveLength(1); + expect(await readFile(db, "/workspace/a.txt", "utf8")).toBe("original"); + // Nothing was written, so the revision did not move. + expect(db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'")).toBe(revBefore); + }); + }); + + it("does not delete a file a tombstone would have removed", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + writeFileSync(db, "/workspace/keep.txt", new TextEncoder().encode("hi"), {}, () => 0); + + const result = await applyChanges( + db, + [{ kind: "delete", rev: 300, path: "/workspace/keep.txt" }], + new Map(), + { writable: false }, + ); + + expect(result.applied).toBe(0); + expect(resolveInode(db, "/workspace/keep.txt")).not.toBeNull(); + }); + }); + + it("defaults to applying so existing callers are unaffected", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + + const result = await applyChanges(db, [fileEntry("/workspace/a.txt", 100)], new Map()); + + expect(result.applied).toBe(1); + expect(result.skipped).toEqual([]); + }); + }); + + // Both guards would refuse this entry. The missing capability is + // reported because it is the reason the caller asked for: the + // command had no write access, whatever the mount says. + it("reports the missing capability rather than the mount when both would refuse", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace/r2", { recursive: true }, () => 0); + db.run( + "INSERT INTO _vfs_mounts (root, kind, indexed, mode) VALUES (?, ?, 1, 'read-only')", + "/workspace/r2", + "test", + ); + invalidateReadOnlyMountCache(db); + + const result = await applyChanges( + db, + [fileEntry("/workspace/r2/hello.txt", 100)], + new Map(), + { + writable: false, + }, + ); + + expect(result.skipped).toEqual([ + { path: "/workspace/r2/hello.txt", op: "write", reason: "no-write-access" }, + ]); + }); + }); + + it("applies nothing through applyChangesSync either", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + + const result = applyChangesSync(db, [fileEntry("/workspace/a.txt", 100)], new Map(), { + writable: false, + }); + + expect(result.applied).toBe(0); + expect(result.skipped).toEqual([ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + ]); + expect(resolveInode(db, "/workspace/a.txt")).toBeNull(); + }); + }); +}); diff --git a/packages/dofs/src/sync/apply.ts b/packages/dofs/src/sync/apply.ts index 4986c98e..84d628a1 100644 --- a/packages/dofs/src/sync/apply.ts +++ b/packages/dofs/src/sync/apply.ts @@ -12,23 +12,41 @@ import type { Database } from "../storage.js"; import type { ChangeEntry } from "./changes.js"; import { computeManifestHash } from "./manifests.js"; -// One container-side change that landed under a read-only mount and -// was therefore skipped rather than applied. Callers (the workspace -// pull surface, the shell exec bracket) surface these so the user -// learns the mount stayed authoritative. -export interface SkippedEntry { +// One incoming change that was refused rather than applied. Callers +// (the workspace pull surface, the shell exec bracket) surface these +// so the user learns what did not land. +// +// Discriminated on `reason` because the two refusals carry different +// information. A read-only mount names the mount that owns the path, +// which lets a caller group entries by mount. A missing write +// capability has no mount to name: the apply itself had no write +// access, so every entry is refused wherever it pointed. +export type SkippedEntry = SkippedByMount | SkippedWithoutWriteAccess; + +// Common fields. Split out so both members stay in step. +interface SkippedEntryBase { // Absolute VFS path the change targeted. path: string; - // Mount root that owns the path (the one whose mode is - // read-only). Lets callers group skipped entries by mount. - mountRoot: string; // 'write' covers file / dir / symlink create-or-update; 'delete' // covers tombstones. The single field is enough for callers to // decide messaging. op: "write" | "delete"; - // Open shape: future skip reasons can join this union without - // breaking callers that match on 'read-only' today. +} + +// The path falls under a registered read-only mount root. +export interface SkippedByMount extends SkippedEntryBase { reason: "read-only"; + // Mount root that owns the path (the one whose mode is + // read-only). Lets callers group skipped entries by mount. + mountRoot: string; +} + +// The apply ran without write access, so nothing was applied at all. +// This is the after-the-fact half of a read-only command: the sender +// has already written to its own copy of the files, and the changes +// stop here on the way in. +export interface SkippedWithoutWriteAccess extends SkippedEntryBase { + reason: "no-write-access"; } // Return shape of applyChanges / applyChangesSync. Existing callers @@ -68,6 +86,28 @@ export interface ApplyOptions { // which is fine for the container backend the package shipped // with first. backend?: string; + // Write access for this apply. Defaults to true. Pass false and + // nothing is applied: every entry comes back in `skipped` with + // reason 'no-write-access'. + // + // This is how a command that ran without write access is enforced + // for a backend that keeps its own copy of the files. The command + // wrote to that copy already, so refusing here is after the fact + // rather than preventive — the copies diverge, and the skipped + // entries are what tells the caller so. A backend that writes this + // store directly gets the preventive version instead, through a + // filesystem handle built without write access. + writable?: boolean; +} + +// Shape one refused entry for an apply that has no write access. +// Shared by both apply variants so they cannot drift. +function refusedWithoutWriteAccess(entry: ChangeEntry): SkippedWithoutWriteAccess { + return { + path: entry.path, + op: entry.kind === "delete" ? "delete" : "write", + reason: "no-write-access", + }; } const DEFAULT_MAX_BYTES = 64 * 1024 * 1024; @@ -229,6 +269,7 @@ export async function applyChanges( ): Promise { const maxBytes = options.maxBytesPerBatch ?? DEFAULT_MAX_BYTES; const maxPaths = options.maxPathsPerBatch ?? DEFAULT_MAX_PATHS; + const writable = options.writable ?? true; let bytesInBatch = 0; let pathsInBatch = 0; @@ -240,6 +281,13 @@ export async function applyChanges( }; for await (const entry of entries) { + // Write capability. Checked ahead of everything else so a + // read-only apply reports every entry it was handed rather than + // quietly dropping the ones that happened to match local state. + if (writable === false) { + skipped.push(refusedWithoutWriteAccess(entry)); + continue; + } // Idempotent skip: if the entry already matches the local // state, drop it on the floor. The check is what stops a // pull from bumping vfs_meta.rev for entries that are @@ -358,6 +406,7 @@ export function applyChangesSync( ): ApplyResult { const maxBytes = options.maxBytesPerBatch ?? DEFAULT_MAX_BYTES; const maxPaths = options.maxPathsPerBatch ?? DEFAULT_MAX_PATHS; + const writable = options.writable ?? true; let bytesInBatch = 0; let pathsInBatch = 0; @@ -369,6 +418,10 @@ export function applyChangesSync( }; for (const entry of entries) { + if (writable === false) { + skipped.push(refusedWithoutWriteAccess(entry)); + continue; + } if (options.source === "upstream" && entry.kind !== "delete") { if (alreadyApplied(db, entry)) continue; } diff --git a/packages/rpc/src/interface.ts b/packages/rpc/src/interface.ts index 7eb5bca0..197bc729 100644 --- a/packages/rpc/src/interface.ts +++ b/packages/rpc/src/interface.ts @@ -108,6 +108,19 @@ export interface ShellRPC { env?: Record; // Standard input bytes fed to the command. stdin?: Uint8Array; + // Whether this command may modify the workspace. Defaults to + // true. False means the caller expects the command to only read, + // and asks that any write it attempts fail rather than land. + // + // How completely that holds depends on where the files live. A + // runner working directly against the workspace store gets a + // filesystem handle without write access, so a write fails inside + // the command with EROFS and the command sees the error. A runner + // with its own copy of the files cannot be stopped that way: the + // write lands in its copy, and is refused on the way back, which + // leaves the two copies disagreeing. The refusal is reported + // either way, but only the first shape prevents the write. + writable?: boolean; }): Promise<{ id: string; events: ReadableStream; diff --git a/packages/rpc/src/sync-driver.test.ts b/packages/rpc/src/sync-driver.test.ts index 46c6e294..b1a43b86 100644 --- a/packages/rpc/src/sync-driver.test.ts +++ b/packages/rpc/src/sync-driver.test.ts @@ -1274,6 +1274,119 @@ describe("sync driver — reconcileWatermarks", () => { }); }); +describe("sync driver — pullOnce without write access", () => { + it("applies nothing and reports every entry it was handed", async () => { + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/one.txt", "one"); + providerR.writeFileSync("/two.txt", "two"); + + const result = await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + + expect(result.applied).toBe(0); + expect(result.skipped).toHaveLength(2); + for (const entry of result.skipped) { + expect(entry.reason).toBe("no-write-access"); + } + expect(result.skipped.map((s) => s.path).sort()).toEqual(["/one.txt", "/two.txt"]); + expect(fileEntries(local.db)).toEqual([]); + } finally { + remote.close(); + local.close(); + } + }); + + it("stages no object bytes", async () => { + // The byte fetch is skipped rather than fetched-and-discarded. + // stageBlob writes to the local store directly, so fetching + // bytes for entries we are about to refuse would write through + // the very capability the pull is missing — and pay for the + // transfer to do it. + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/payload.txt", "some bytes worth fetching"); + + await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + + expect(local.db.scalar("SELECT COUNT(*) FROM vfs_blobs")).toBe(0); + } finally { + remote.close(); + local.close(); + } + }); + + it("advances the fetch cursor so refused changes are not redelivered", async () => { + // Refusing is discarding, not deferring. If the cursor stayed + // put, the next pull that *does* have write access would apply + // the writes this one refused, which would make the refusal a + // delay rather than a denial. + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/refused.txt", "refused"); + + await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + expect(readFetchCursor(local.db)).toEqual({ rev: currentRev(remote.db), path: null }); + + const second = await pullOnce(local.db, remote.rpc); + expect(second.applied).toBe(0); + expect(second.skipped).toEqual([]); + expect(fileEntries(local.db)).toEqual([]); + } finally { + remote.close(); + local.close(); + } + }); + + it("applies changes made after write access is restored", async () => { + // The refusal is scoped to the one pull that lacked the + // capability. It does not latch, and it does not poison the + // cursor for later work. + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/refused.txt", "refused"); + await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + + providerR.writeFileSync("/allowed.txt", "allowed"); + const second = await pullOnce(local.db, remote.rpc); + + expect(second.applied).toBe(1); + expect(second.skipped).toEqual([]); + expect(fileEntries(local.db)).toEqual(["allowed.txt"]); + const providerL = new SQLiteWorkspaceProvider(local.db, { now: () => 1 }); + expect(providerL.readFileSync("/allowed.txt", "utf8")).toBe("allowed"); + } finally { + remote.close(); + local.close(); + } + }); + + it("pulls normally when the option is omitted or true", async () => { + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/one.txt", "one"); + + const result = await pullOnce(local.db, remote.rpc, undefined, { writable: true }); + + expect(result.applied).toBe(1); + expect(result.skipped).toEqual([]); + expect(fileEntries(local.db)).toEqual(["one.txt"]); + } finally { + remote.close(); + local.close(); + } + }); +}); + async function sha256(bytes: Uint8Array): Promise { const { createHash } = await import("node:crypto"); const hash = createHash("sha256"); diff --git a/packages/rpc/src/sync-driver.ts b/packages/rpc/src/sync-driver.ts index fba350a5..b9c49fbb 100644 --- a/packages/rpc/src/sync-driver.ts +++ b/packages/rpc/src/sync-driver.ts @@ -66,15 +66,29 @@ const PULL_BATCH_SIZE = 256; // (rev, path), so a retry can resume inside a single large rev. The // cursor is read and written per backend so concurrent backends keep // independent resume points. +// +// `writable: false` pulls the entries and refuses all of them. The +// stream is still drained so the caller learns what the remote tried +// to send, reported as skipped entries with reason no-write-access. +// This is the only enforcement point available against a remote that +// keeps its own copy of the files: it has already written to that +// copy by the time we hear about it. export async function pullOnce( db: Database, remote: SyncRPC, backend?: string, + options?: PullOptions, ): Promise { // Delegate to the inner implementation with retried=false. See // pullOnceImpl for the fetchChanges round trip, invariant check, // reset-and-retry path, and batched apply loop. - return pullOnceImpl(db, remote, backend, false); + return pullOnceImpl(db, remote, backend, false, options?.writable ?? true); +} + +export interface PullOptions { + // Whether this pull may write to the local store. Defaults to + // true. A pull without write access applies nothing. + writable?: boolean; } // Inner pullOnce that knows whether it is already a retry. The @@ -87,6 +101,7 @@ async function pullOnceImpl( remote: SyncRPC, backend: string | undefined, retried: boolean, + writable: boolean, ): Promise { const after = readFetchCursor(db, backend); const localPushRev = readWatermark(db, "pushRev", backend); @@ -164,7 +179,7 @@ async function pullOnceImpl( if (fetchDiverged) { writeFetchCursor(db, { rev: 0, path: null }, backend); } - return pullOnceImpl(db, remote, backend, true); + return pullOnceImpl(db, remote, backend, true, writable); } // After the retry path above, this assertion guards a // divergence that survived a reset. Tear down rather than loop. @@ -211,7 +226,12 @@ async function pullOnceImpl( // Probe + fetch missing chunk bytes for just this batch. Bytes // the receiver already holds (or the remote doesn't have) are // skipped, so the per-batch network cost is bounded. - if (wantedHashes.length > 0) { + // + // Skipped entirely without write access. stageBlob writes to + // the local store, so fetching bytes for entries we are about + // to refuse would write through the capability this pull does + // not have — and pay for the transfer to do it. + if (writable && wantedHashes.length > 0) { const haveSubset = await remote.hasObjects(wantedHashes); const remoteHasLocally = new Set(); for (const h of haveSubset) remoteHasLocally.add(hex(h)); @@ -241,6 +261,7 @@ async function pullOnceImpl( const batchResult = await applyChanges(db, batch, new Map(), { source: "upstream", backend, + writable, }); const last = batch[batch.length - 1]; // Cursor advancement intentionally happens after applyChanges() @@ -248,6 +269,11 @@ async function pullOnceImpl( // checkpoint re-fetches the batch; upstream apply is idempotent // because alreadyApplied() drops entries whose live state // already matches. + // + // The cursor advances over refused entries too. Refusing is + // discarding, not deferring: leaving the cursor behind would + // hand the same writes to the next pull that does have write + // access, turning a denial into a delay. writeFetchCursorIfAhead(db, { rev: last.rev, path: last.path }, backend); totalApplied += batchResult.applied; if (batchResult.skipped.length > 0) {