diff --git a/packages/runtime-host/src/server/execution-composition.ts b/packages/runtime-host/src/server/execution-composition.ts index 85fab99da3..a3a5ca12b3 100644 --- a/packages/runtime-host/src/server/execution-composition.ts +++ b/packages/runtime-host/src/server/execution-composition.ts @@ -1631,7 +1631,9 @@ function adaptManagedWorkspaceFilesystemWorker( ): ManagedWorkspaceFilesystemWorker { return { async execute(input) { - const result = await worker.execute(input); + // Read-only operations never participate in CAS; the adapter says so + // explicitly (#3484) instead of relying on an absent optional field. + const result = await worker.execute({ ...input, expectedIdentity: 'unchecked' }); switch (result.kind) { case 'read': case 'read_image': diff --git a/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts b/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts index b5d2b92746..e516c0e9a4 100644 --- a/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts +++ b/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts @@ -17,6 +17,7 @@ import { type FilesystemWorkerClient, type FilesystemWorkerClientErrorReason, type FilesystemWorkerExecuteInput, + type FilesystemWorkerExpectedIdentity, } from '../filesystem-worker/client.js'; import type { FilesystemWorkerResult } from '../filesystem-worker/protocol.js'; import { createLocalWorkspaceExecutor } from '../workspace-executor.js'; @@ -216,7 +217,7 @@ describe('filesystem mutation T0 identity capture (queue-window closure)', () => releaseFirst = resolve; }); let calls = 0; - let queuedIdentity: { dev: string; ino: string } | undefined; + let queuedIdentity: FilesystemWorkerExpectedIdentity | undefined; const gatedWorker: { execute: (input: FilesystemWorkerExecuteInput) => Promise; } = { @@ -255,13 +256,46 @@ describe('filesystem mutation T0 identity capture (queue-window closure)', () => releaseFirst(); await Promise.all([first, second]); - assert.ok(queuedIdentity, 'the queued mutation should have dispatched to the worker'); + assert.ok( + queuedIdentity && typeof queuedIdentity !== 'string', + 'the queued mutation should have dispatched with the captured identity', + ); assert.equal( - queuedIdentity.ino, + (queuedIdentity as { dev: string; ino: string }).ino, String(original.ino), 'identity must be the inode captured at lock acquisition (before the replacement); a T1 capture would sample the replacement', ); }); + + test('an apply_patch mutation forwards its captured identity, not unchecked (#3484 regression)', async () => { + const cwd = await realpath(await mkdtemp(join(tmpdir(), 'maka-t0-applypatch-'))); + cleanup.push(cwd); + const target = join(cwd, 'file.txt'); + await writeFile(target, 'original', 'utf8'); + + const original = await stat(target, { bigint: true }); + let dispatched: FilesystemWorkerExpectedIdentity | undefined; + const gatedWorker: { + execute: (input: FilesystemWorkerExecuteInput) => Promise; + } = { + async execute(input) { + dispatched = input.expectedIdentity; + return { kind: 'apply_patch', ok: true, path: target }; + }, + }; + const fs = executorWith(gatedWorker); + + await fs.applyPatch({ + operation: { type: 'update_file', path: target, diff: '--- a\n+++ b\n' }, + cwd, + }); + + assert.ok( + dispatched && typeof dispatched !== 'string', + 'apply_patch must dispatch with the captured identity, not unchecked', + ); + assert.equal(dispatched.ino, String(original.ino)); + }); }); function sleep(ms: number): Promise { diff --git a/packages/runtime/src/__tests__/filesystem-target-identity.test.ts b/packages/runtime/src/__tests__/filesystem-target-identity.test.ts index 48d59c4c0f..80435e3ec6 100644 --- a/packages/runtime/src/__tests__/filesystem-target-identity.test.ts +++ b/packages/runtime/src/__tests__/filesystem-target-identity.test.ts @@ -29,7 +29,8 @@ async function temporaryDirectory(prefix: string): Promise { function requestFor( operation: FilesystemWorkerRequest['operation'], - expectedTarget: FilesystemWorkerTarget, + expectedTarget: Omit, + identity: FilesystemWorkerTarget['identity'] = 'missing', ): FilesystemWorkerRequest { return { version: FILESYSTEM_WORKER_PROTOCOL_VERSION, @@ -40,7 +41,7 @@ function requestFor( entries: [{ path: expectedTarget.enforcementPath, access: 'write', scope: 'exact' }], }, }, - expectedTarget, + expectedTarget: { ...expectedTarget, identity }, }; } @@ -67,7 +68,8 @@ describe('filesystem worker target identity CAS', () => { const response = await executeFilesystemWorkerRequest( requestFor( { kind: 'write', cwd, path: target, content: 'new' }, - { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + identity, ), ); @@ -90,7 +92,8 @@ describe('filesystem worker target identity CAS', () => { const response = await executeFilesystemWorkerRequest( requestFor( { kind: 'write', cwd, path: target, content: 'updated' }, - { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + identity, ), ); @@ -115,7 +118,8 @@ describe('filesystem worker target identity CAS', () => { const response = await executeFilesystemWorkerRequest( requestFor( { kind: 'apply_patch', cwd, path: target, action: 'delete' }, - { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + identity, ), ); @@ -139,7 +143,8 @@ describe('filesystem worker target identity CAS', () => { const response = await executeFilesystemWorkerRequest( requestFor( { kind: 'edit', cwd, path: target, oldString: 'old', newString: 'new' }, - { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + identity, ), ); @@ -150,6 +155,37 @@ describe('filesystem worker target identity CAS', () => { assert.equal(await readFile(target, 'utf8'), 'replacement\nold\n'); }); + test('rejects an apply_patch update when the target inode changed after authorisation', async () => { + const cwd = await temporaryDirectory('maka-identity-applypatch-update-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'line\noriginal\n', 'utf8'); + await writeFile(replacement, 'line\nreplacement\n', 'utf8'); + + const identity = await captureIdentity(target); + await rename(replacement, target); + + const response = await executeFilesystemWorkerRequest( + requestFor( + { + kind: 'apply_patch', + cwd, + path: target, + action: 'update', + diff: '--- a\n+++ b\n@@ -1,2 +1,2 @@\n line\n-original\n+updated\n', + }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + identity, + ), + ); + + assert.equal(response.ok, false); + assert.equal(response.error?.code, 'path_changed'); + // The replacement content must be untouched (the patch never applied). + const { readFile } = await import('node:fs/promises'); + assert.equal(await readFile(target, 'utf8'), 'line\nreplacement\n'); + }); + test('creates a missing target without requiring an identity', async () => { const cwd = await temporaryDirectory('maka-identity-missing-'); const target = join(cwd, 'brand-new.txt'); @@ -170,6 +206,32 @@ describe('filesystem worker target identity CAS', () => { assert.equal(await readFile(target, 'utf8'), 'created'); }); + test('a write to a missing target reports a creation diff (#3487 P1)', async () => { + const cwd = await temporaryDirectory('maka-identity-missing-write-'); + const target = join(cwd, 'brand-new.txt'); + + // The truthy 'missing' identity string used to make the "approved + // missing" truthiness test fail, collapsing the diff into unknown and + // hiding new-file changes from review. The write must report the creation + // diff (`--- /dev/null`) instead. + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'write', cwd, path: target, content: 'created\n' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'missing' }, + ), + ); + + assert.equal(response.ok, true); + assert.equal(response.result.kind, 'write'); + if (response.result.kind !== 'write') return; + assert.ok( + response.result.diff?.includes('--- /dev/null'), + 'a created file must carry a creation diff with --- /dev/null', + ); + const { readFile } = await import('node:fs/promises'); + assert.equal(await readFile(target, 'utf8'), 'created\n'); + }); + test('reports path_changed when the target is replaced before the write (pre-write CAS)', async () => { const cwd = await temporaryDirectory('maka-identity-prewrite-'); const target = join(cwd, 'file.txt'); @@ -186,7 +248,8 @@ describe('filesystem worker target identity CAS', () => { const response = await executeFilesystemWorkerRequest( requestFor( { kind: 'write', cwd, path: target, content: 'new' }, - { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + identity, ), ); diff --git a/packages/runtime/src/__tests__/filesystem-worker-client.test.ts b/packages/runtime/src/__tests__/filesystem-worker-client.test.ts index 8ea0ed0296..8af583dbaa 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-client.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-client.test.ts @@ -86,8 +86,10 @@ describe('filesystem worker client permission snapshots', () => { assert.equal(expectedTarget?.targetType, 'symlink'); // The symlink entry's own identity (lstat, no follow) is captured at T0 and // forwarded; only its shape is stable, not its value. - assert.equal(typeof expectedTarget?.identity?.dev, 'string'); - assert.equal(typeof expectedTarget?.identity?.ino, 'string'); + assert.equal(typeof expectedTarget?.identity, 'object'); + const forwarded = expectedTarget?.identity as { dev: string; ino: string }; + assert.equal(typeof forwarded.dev, 'string'); + assert.equal(typeof forwarded.ino, 'string'); }); for (const kind of ['bypass', 'external'] as const) { @@ -101,6 +103,7 @@ describe('filesystem worker client permission snapshots', () => { cwd: workspace, executionBoundary: { kind, revision: 1 }, mode: 'execute', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -129,6 +132,7 @@ describe('filesystem worker client permission snapshots', () => { createWorkspaceWritePermissionProfile(), 0, ), + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -154,6 +158,7 @@ describe('filesystem worker client permission snapshots', () => { cwd: workspace, mode: 'execute', permissionProfile: createReadOnlyPermissionProfile(), + expectedIdentity: 'unchecked', }), isPathDenied, ); @@ -183,6 +188,7 @@ describe('filesystem worker client permission snapshots', () => { cwd: workspace, mode: 'execute', permissionProfile: profile, + expectedIdentity: 'unchecked', }), isPathDenied, ); @@ -210,6 +216,7 @@ describe('filesystem worker client permission snapshots', () => { operation: { kind: 'read', path: target }, cwd: workspace, mode: 'explore', + expectedIdentity: 'unchecked', }), isPathDenied, ); @@ -218,6 +225,7 @@ describe('filesystem worker client permission snapshots', () => { cwd: workspace, mode: 'explore', permissionProfile: profile, + expectedIdentity: 'unchecked', }); assert.deepEqual(result, { kind: 'read', content: 'worker-content' }); @@ -243,6 +251,7 @@ describe('filesystem worker client Grep target scope', () => { operation: grepOperation(target), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.deepEqual(result, { kind: 'grep', matches: ['file.ts:1:value'] }); @@ -259,6 +268,7 @@ describe('filesystem worker client Grep target scope', () => { operation: grepOperation(directory), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(requests[0]?.expectedTarget.scope, 'subtree'); @@ -283,6 +293,7 @@ describe('filesystem worker operation-scoped Seatbelt profile', () => { operation: { kind: 'write', path: target, content: 'target' }, cwd: workspace, mode: 'execute', + expectedIdentity: 'unchecked', }); const transform = transforms[0]; @@ -315,6 +326,7 @@ describe('filesystem worker operation-scoped Seatbelt profile', () => { operation: grepOperation(target), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -338,6 +350,7 @@ describe('filesystem worker operation-scoped Seatbelt profile', () => { operation: grepOperation(target), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -365,6 +378,7 @@ describe('filesystem worker Linux path context', () => { operation: { kind: 'glob', path: target, pattern: '*.ts', limit: 20 }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -385,6 +399,7 @@ describe('filesystem worker Linux path context', () => { operation: { kind: 'read', path: target }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); const processInput = processInputs[0]; @@ -407,6 +422,8 @@ describe('filesystem worker Linux path context', () => { operation: { kind: 'write', path: target, content: 'new' }, cwd: workspace, mode: 'ask', + // T0 observed no target (a create), so the T0 marker is 'missing'. + expectedIdentity: 'missing', }); const processInput = processInputs[0]; @@ -621,6 +638,7 @@ describe('filesystem worker client dispatch classification', () => { operation: { kind: 'write', path: '/tmp/maka-dispatch-incomplete.txt', content: 'x' }, cwd: '/tmp', mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -638,6 +656,7 @@ describe('filesystem worker client dispatch classification', () => { operation: { kind: 'write', path: '/tmp/maka-dispatch-spawn.txt', content: 'x' }, cwd: '/tmp', mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -656,6 +675,7 @@ describe('filesystem worker client dispatch classification', () => { operation: { kind: 'write', path: '/tmp/maka-dispatch-noflag.txt', content: 'x' }, cwd: '/tmp', mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -723,7 +743,10 @@ describe('filesystem worker client dispatch classification', () => { }); assert.equal(requests[0]?.expectedTarget.targetType, 'missing'); - assert.equal(requests[0]?.expectedTarget.identity, undefined); + // The stale identity is never sent on a missing target; the wire's + // required identity contract reports 'missing' (nothing to compare), + // which lets the worker proceed as a fresh exclusive create. + assert.equal(requests[0]?.expectedTarget.identity, 'missing'); }); test('rejects a write whose target was created while queued (never invalid_request)', async () => { @@ -757,6 +780,9 @@ describe('filesystem worker client dispatch classification', () => { operation: { kind: 'write', path: target, content: 'new' }, cwd: workspace, mode: 'ask', + // T0 approved the target as missing; it appeared by T1 — the + // "created while queued" race, which must stay path_changed. + expectedIdentity: 'missing', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -767,6 +793,93 @@ describe('filesystem worker client dispatch classification', () => { // The interloper's content was never touched. assert.equal(await readFile(target, 'utf8'), 'external-content'); }); + + test('lets an unchecked caller write an existing target without a CAS identity (#3484)', async () => { + const workspace = await temporaryDirectory('maka-client-unchecked-'); + const target = join(workspace, 'file.txt'); + // The target already exists; the caller has no T0 snapshot to compare + // (e.g. a verification script that owns the path itself). + await writeFile(target, 'existing', 'utf8'); + + const { client, requests } = fakeClient(); + await client.execute({ + operation: { kind: 'write', path: target, content: 'new' }, + cwd: workspace, + mode: 'ask', + expectedIdentity: 'unchecked', + }); + + const expectedTarget = requests[0]?.expectedTarget; + assert.equal(expectedTarget?.targetType, 'file'); + assert.equal(expectedTarget?.identity, 'unchecked'); + }); + + test('marks a T0-missing create as missing on the wire, not unchecked (#3484)', async () => { + const workspace = await temporaryDirectory('maka-client-t0missing-'); + const target = join(workspace, 'new.txt'); + + const { client, requests } = fakeClient(); + await client.execute({ + operation: { kind: 'write', path: target, content: 'new' }, + cwd: workspace, + mode: 'ask', + expectedIdentity: 'missing', + }); + + const expectedTarget = requests[0]?.expectedTarget; + assert.equal(expectedTarget?.targetType, 'missing'); + assert.equal(expectedTarget?.identity, 'missing'); + }); + + test('a read without expectedIdentity sends unchecked automatically (#3487)', async () => { + const workspace = await temporaryDirectory('maka-client-read-auto-'); + const target = join(workspace, 'file.txt'); + await writeFile(target, 'content', 'utf8'); + + const { client, requests } = fakeClient(); + await client.execute({ + operation: { kind: 'read', path: target }, + cwd: workspace, + mode: 'ask', + // No expectedIdentity: reads never participate in CAS, and the client + // must not reject or silently omit the wire field — a plain-JavaScript + // caller that bypasses TypeScript has no way to get this wrong. + }); + + assert.equal(requests[0]?.expectedTarget.identity, 'unchecked'); + }); + + test('a write without expectedIdentity is rejected at runtime (#3487)', async () => { + const workspace = await temporaryDirectory('maka-client-write-required-'); + const target = join(workspace, 'file.txt'); + await writeFile(target, 'existing', 'utf8'); + + const client = new FilesystemWorkerClient({ + sandboxManager: new SandboxManager([new MacosSeatbeltBackend()]), + platform: 'darwin', + getLaunchSpec: async () => ({ + ok: false, + reason: 'worker_bundle_unavailable', + message: 'unused', + }), + runProcess: async () => { + throw new Error('must not dispatch'); + }, + }); + + await assert.rejects( + client.execute({ + operation: { kind: 'write', path: target, content: 'new' }, + cwd: workspace, + mode: 'ask', + }), + (error: unknown) => { + assert.ok(error instanceof FilesystemWorkerClientError); + assert.equal(error.reason, 'invalid_request'); + return true; + }, + ); + }); }); function isPathDenied(error: unknown): boolean { diff --git a/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts b/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts index f5dfbf8bb3..1c90892c24 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts @@ -61,6 +61,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(await readFile(sourceFile, 'utf8'), 'export const healthSignal = true;\n'); @@ -68,6 +69,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { operation: { kind: 'read', path: sourceFile }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.deepEqual(read, { kind: 'read', @@ -101,6 +103,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.deepEqual(glob, { kind: 'glob', files: ['health.ts'] }); @@ -115,6 +118,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(grep.kind, 'grep'); if (grep.kind === 'grep') { @@ -135,6 +139,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(await readFile(target, 'utf8'), 'created'); @@ -150,6 +155,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { operation: { kind: 'write', path: allowedPath, content: 'blocked' }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), isPathDenied, ); @@ -159,6 +165,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { cwd: workspace, mode: 'ask', executionBoundary, + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -177,6 +184,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { cwd: workspace, mode: 'ask', executionBoundary, + expectedIdentity: 'unchecked', }); assert.equal(await readFile(allowedPath, 'utf8'), 'outside-ok'); }); diff --git a/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts b/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts index 1e5d5fd6d0..7b3db0ae8a 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts @@ -46,6 +46,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' operation: { kind: 'write', path: insidePath, content: 'inside-ok' }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(await readFile(insidePath, 'utf8'), 'inside-ok'); @@ -54,6 +55,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' operation: { kind: 'write', path: outsidePath, content: 'blocked' }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => error instanceof FilesystemWorkerClientError && error.reason === 'path_denied', @@ -72,6 +74,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(await readFile(target, 'utf8'), 'created'); @@ -107,6 +110,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' cwd: workspace, mode: 'ask', executionBoundary, + expectedIdentity: 'unchecked', }), (error: unknown) => { assert.ok(error instanceof FilesystemWorkerClientError); @@ -124,6 +128,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' cwd: workspace, mode: 'ask', executionBoundary, + expectedIdentity: 'unchecked', }); assert.equal(await readFile(allowedPath, 'utf8'), 'outside-ok'); }); @@ -138,6 +143,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' operation: grepOperation(sourceFile, 'healthSignal'), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(fileResult.kind, 'grep'); if (fileResult.kind === 'grep') { @@ -149,6 +155,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' operation: grepOperation(sourceDirectory, 'healthSignal'), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(directoryResult.kind, 'grep'); if (directoryResult.kind === 'grep') { @@ -160,6 +167,7 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' operation: grepOperation(sourceDirectory, 'does-not-exist'), cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.deepEqual(emptyResult, { kind: 'grep', matches: [] }); }); diff --git a/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts b/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts index 967779d4fa..063aecea70 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts @@ -103,6 +103,7 @@ describe('Windows filesystem worker smoke', { skip: !enabled }, () => { operation: { kind: 'read', path: target }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(read.kind, 'read'); if (read.kind === 'read') assert.match(read.content, /windows-relay-ok/); @@ -118,6 +119,7 @@ describe('Windows filesystem worker smoke', { skip: !enabled }, () => { operation: { kind: 'write', path: missing, content: 'blocked' }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => error instanceof FilesystemWorkerClientError && @@ -137,6 +139,7 @@ describe('Windows filesystem worker smoke', { skip: !enabled }, () => { operation: { kind: 'glob', path: sourceDirectory, pattern: '**/*.ts' }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }); assert.equal(globResult.kind, 'glob'); if (globResult.kind === 'glob') { @@ -159,6 +162,7 @@ describe('Windows filesystem worker smoke', { skip: !enabled }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => error instanceof FilesystemWorkerClientError && error.reason === 'grep_unavailable', @@ -175,6 +179,7 @@ describe('Windows filesystem worker smoke', { skip: !enabled }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: 'unchecked', }), (error: unknown) => error instanceof FilesystemWorkerClientError && error.reason === 'path_denied', diff --git a/packages/runtime/src/__tests__/filesystem-worker.test.ts b/packages/runtime/src/__tests__/filesystem-worker.test.ts index f4ebb2a44d..c378e22373 100644 --- a/packages/runtime/src/__tests__/filesystem-worker.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker.test.ts @@ -419,6 +419,45 @@ describe('filesystem worker operations', () => { if (!response.ok) assert.equal(response.error.code, 'path_changed'); }); + test('accepts an unchecked write target without an identity (#3484)', async () => { + const root = await temporaryDirectory('maka-worker-unchecked-'); + const target = join(root, 'file.txt'); + await writeFile(target, 'existing', 'utf8'); + + const response = await executeFilesystemWorkerRequest( + await requestFor( + { kind: 'write', cwd: root, path: target, content: 'new' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, + target, + 'unchecked', + ), + ); + + assert.ok(response.ok); + assert.equal(response.result.kind, 'write'); + if (response.result.kind !== 'write') return; + assert.equal(await readFile(target, 'utf8'), 'new'); + }); + + test('rejects a write whose T0-missing target exists at execution time (#3484)', async () => { + const root = await temporaryDirectory('maka-worker-created-'); + const target = join(root, 'file.txt'); + // T0 approved the target as missing; something created it before the + // worker executed. Writing would clobber content the caller never saw. + await writeFile(target, 'external', 'utf8'); + + const response = await executeFilesystemWorkerRequest( + await requestFor( + { kind: 'write', cwd: root, path: target, content: 'new' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'missing' }, + ), + ); + assert.equal(response.ok, false); + if (!response.ok) assert.equal(response.error.code, 'path_changed'); + // The interloper's content was never touched. + assert.equal(await readFile(target, 'utf8'), 'external'); + }); + test('omits the diff when the content is too large to diff cheaply', async () => { const root = await temporaryDirectory('maka-worker-huge-'); const target = join(root, 'huge.ts'); @@ -504,8 +543,9 @@ describe('filesystem worker operations', () => { async function requestFor( operation: FilesystemWorkerOperation, - expectedTarget: FilesystemWorkerTarget, + expectedTarget: Omit, permissionPath = operation.path, + identity?: FilesystemWorkerTarget['identity'], ): Promise { const operationBoundary: FilesystemWorkerRequest['operationBoundary'] = { filesystem: { @@ -520,8 +560,18 @@ async function requestFor( }; // The real caller captures the target identity at T0; mirror that here so // the worker's mandatory-identity check is satisfied for non-missing targets. - let resolvedTarget = expectedTarget; - if (expectedTarget.targetType !== 'missing' && !expectedTarget.identity) { + let resolvedTarget: FilesystemWorkerTarget = { + ...expectedTarget, + // Always replaced below; the placeholder keeps the type total. + identity: 'unchecked', + }; + if (identity !== undefined) { + // The test chose the identity contract explicitly (e.g. 'unchecked' or a + // stale inode); keep the target exactly as given. + resolvedTarget = { ...expectedTarget, identity }; + } else if (expectedTarget.targetType === 'missing') { + resolvedTarget = { ...expectedTarget, identity: 'missing' }; + } else { const follow = expectedTarget.targetType !== 'symlink'; try { const metadata = follow @@ -533,7 +583,8 @@ async function requestFor( }; } catch { // Target may not exist at request construction time (the test sets it up - // differently); leave identity absent and let the worker surface it. + // differently); fall back to 'missing' so the worker's own checks decide. + resolvedTarget = { ...expectedTarget, identity: 'missing' }; } } return { diff --git a/packages/runtime/src/file-stable-write.ts b/packages/runtime/src/file-stable-write.ts index 08afc9060a..6b69ab3437 100644 --- a/packages/runtime/src/file-stable-write.ts +++ b/packages/runtime/src/file-stable-write.ts @@ -58,51 +58,20 @@ function pathChanged(message: string): StableWriteFailure { * so a failed identity check leaves the file untouched. Windows has no * O_NOFOLLOW; the link is detected with an lstat just before the open and * the residual window is closed by the identity comparison on the fd. - * - Approved-missing target (approvedIdentity undefined): `open(path, 'wx')` — - * atomic create-if-absent. EEXIST means something appeared in the gap. + * - Existing target without an identity (an 'unchecked' caller, targetType is + * a concrete type): same open, no identity comparison — there is nothing to + * compare, and the caller declared it does not participate in CAS. + * - Approved-missing target (approvedIdentity undefined, targetType + * 'missing'): `open(path, 'wx')` — atomic create-if-absent. EEXIST means + * something appeared in the gap. */ export async function openStableTarget(input: { path: string; approvedIdentity: FilesystemTargetIdentity | undefined; + targetType?: 'file' | 'directory' | 'symlink' | 'other' | 'missing'; }): Promise { - const noFollow = process.platform === 'win32' ? 0 : constants.O_NOFOLLOW; if (input.approvedIdentity) { - if (process.platform === 'win32') { - const entry = await lstat(input.path).catch(() => null); - if (entry?.isSymbolicLink()) { - throw pathChanged( - 'The approved filesystem target is a symbolic link; refusing to follow it.', - ); - } - } - let handle: FileHandle; - try { - handle = await open(input.path, constants.O_RDWR | noFollow); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code === 'ELOOP' || code === 'ENOTDIR' || code === 'ENOENT') { - throw pathChanged('The approved filesystem target changed before execution.'); - } - if (code === 'EACCES' || code === 'EPERM') { - // A write-only target (e.g. mode 0o222) refuses 'r+' but is still a - // legitimate mutation target — unlink semantics need no read - // permission. Retry write-only (still no truncate: identity is - // validated on the descriptor before writeThroughHandle truncates). - // The pinned read of the previous content will fail and the caller - // reports 'unknown' (no diff), which is the pre-existing behaviour. - try { - handle = await open(input.path, constants.O_WRONLY | noFollow); - } catch (retry) { - const retryCode = (retry as NodeJS.ErrnoException).code; - if (retryCode === 'ELOOP' || retryCode === 'ENOTDIR' || retryCode === 'ENOENT') { - throw pathChanged('The approved filesystem target changed before execution.'); - } - throw retry; - } - } else { - throw error; - } - } + const handle = await openExistingNoTruncate(input.path); // The compare in compare-and-update, performed on the descriptor itself. const metadata = await handle.stat({ bigint: true }); if ( @@ -114,6 +83,12 @@ export async function openStableTarget(input: { } return handle; } + if (input.targetType !== undefined && input.targetType !== 'missing') { + // An existing target with no identity: the caller explicitly opted out of + // CAS (#3484). Open without truncation but perform no comparison — the + // caller's own absence of a T0 snapshot is the contract. + return openExistingNoTruncate(input.path); + } try { return await open(input.path, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL); } catch (error) { @@ -124,6 +99,45 @@ export async function openStableTarget(input: { } } +/** Open an existing target for read-write without truncating it. */ +async function openExistingNoTruncate(path: string): Promise { + const noFollow = process.platform === 'win32' ? 0 : constants.O_NOFOLLOW; + if (process.platform === 'win32') { + const entry = await lstat(path).catch(() => null); + if (entry?.isSymbolicLink()) { + throw pathChanged( + 'The approved filesystem target is a symbolic link; refusing to follow it.', + ); + } + } + try { + return await open(path, constants.O_RDWR | noFollow); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ELOOP' || code === 'ENOTDIR' || code === 'ENOENT') { + throw pathChanged('The approved filesystem target changed before execution.'); + } + if (code === 'EACCES' || code === 'EPERM') { + // A write-only target (e.g. mode 0o222) refuses 'r+' but is still a + // legitimate mutation target — unlink semantics need no read + // permission. Retry write-only (still no truncate: identity is + // validated on the descriptor before writeThroughHandle truncates). + // The pinned read of the previous content will fail and the caller + // reports 'unknown' (no diff), which is the pre-existing behaviour. + try { + return await open(path, constants.O_WRONLY | noFollow); + } catch (retry) { + const retryCode = (retry as NodeJS.ErrnoException).code; + if (retryCode === 'ELOOP' || retryCode === 'ENOTDIR' || retryCode === 'ENOENT') { + throw pathChanged('The approved filesystem target changed before execution.'); + } + throw retry; + } + } + throw error; + } +} + /** * Write `content` through the pinned descriptor. The truncation happens here, * only after the identity was validated on the fd. Write-step failures diff --git a/packages/runtime/src/filesystem-executor.ts b/packages/runtime/src/filesystem-executor.ts index 2773626f7a..2d0bce408d 100644 --- a/packages/runtime/src/filesystem-executor.ts +++ b/packages/runtime/src/filesystem-executor.ts @@ -31,6 +31,7 @@ import type { } from './filesystem-worker/client.js'; import { isSupportedImagePath, type ImageMimeType } from './image-file.js'; import type { FilesystemWorkerResult } from './filesystem-worker/protocol.js'; +import { operationAccess } from './filesystem-worker/protocol.js'; import { resolveCanonicalDirectoryEntryTarget } from './path-containment.js'; import { normalizeSandboxBoundaryPath } from './sandbox-boundary-path.js'; import { SandboxCommandError } from './sandbox/errors.js'; @@ -121,12 +122,11 @@ function pathScopeForBoundary(boundary: ExecutionBoundary | undefined): Workspac return boundary?.kind === 'bypass' ? 'host' : 'workspace'; } -/** Operations that read, modify and write back, and so must hold the target's lock. */ -function mutates(operation: FilesystemOperation): boolean { - return ( - operation.kind === 'write' || operation.kind === 'edit' || operation.kind === 'format_json' - ); -} +/** + * Operations that read, modify and write back, and so must hold the target's lock. + * The single authority on which kinds are writes is `operationAccess` in the + * worker protocol; `mutates` was a second, narrower list that drifted. + */ /** * Capture the target's stable identity at lock acquisition (T0) — *before* @@ -215,7 +215,17 @@ export function createBoundaryFilesystemExecutor( mode: call.permissionMode ?? 'ask', ...(input.permissionProfile ? { permissionProfile: input.permissionProfile } : {}), ...(call.abortSignal ? { abortSignal: call.abortSignal } : {}), - ...(expectedIdentity ? { expectedIdentity } : {}), + // The worker client now requires an explicit T0 marker (#3484): a + // mutation carries its captured identity, or 'missing' when T0 saw no + // target; a read never participates in CAS and says so. `operationAccess` + // is the single authority on which kinds are writes (write | apply_patch + // | edit | format_json) — `mutates` is narrower and would silently drop + // the apply_patch identity onto 'unchecked', disabling the queue-window + // CAS on the main editing channel. + expectedIdentity: + operationAccess(call.operation.kind) === 'write' + ? (expectedIdentity ?? 'missing') + : 'unchecked', }); if (result.kind === 'read_image') { return { @@ -256,7 +266,7 @@ export function createBoundaryFilesystemExecutor( } return { async execute(call) { - if (!mutates(call.operation)) return await run(call); + if (operationAccess(call.operation.kind) !== 'write') return await run(call); // Canonicalisation without any containment check, so a target the policy // goes on to reject still takes the same lock as its other spellings. The // key is derived from the same canonicalisation the backend will resolve diff --git a/packages/runtime/src/filesystem-worker/client.ts b/packages/runtime/src/filesystem-worker/client.ts index 3e3a23a6aa..ad1788b543 100644 --- a/packages/runtime/src/filesystem-worker/client.ts +++ b/packages/runtime/src/filesystem-worker/client.ts @@ -25,6 +25,7 @@ import { import { FILESYSTEM_WORKER_PROTOCOL_VERSION, FilesystemWorkerOperationSchema, + operationAccess, operationUsesDirectoryEntry, parseFilesystemWorkerResponse, type FilesystemWorkerErrorCode, @@ -50,6 +51,26 @@ export interface FilesystemWorkerClientInput { platform?: SandboxPlatform; } +/** + * What the caller observed about the operation target at lock acquisition + * (T0). Required, so every caller must decide explicitly which CAS contract it + * is participating in — an absent field is no longer a silently accepted "no + * CAS" that a queue window can slip through (#3484). + * + * - `{ dev, ino }`: T0 observed an existing target; the worker compare-and- + * swaps this against the on-disk inode at T1. + * - `'missing'`: T0 observed no target (a create). If the target exists by T1, + * something created it while this call waited — writing would clobber + * content this call never saw, so the operation fails with `path_changed`. + * - `'unchecked'`: the caller does not participate in CAS (no T0 snapshot, + * e.g. a verification script or a read). Writes proceed without an identity + * check; use deliberately, never as a default for mutations. + */ +export type FilesystemWorkerExpectedIdentity = + | { readonly dev: string; readonly ino: string } + | 'missing' + | 'unchecked'; + export interface FilesystemWorkerExecuteInput { operation: FilesystemWorkerClientOperation; cwd: string; @@ -59,12 +80,15 @@ export interface FilesystemWorkerExecuteInput { permissionProfile?: PermissionProfile; abortSignal?: AbortSignal; /** - * The target identity captured at lock acquisition (T0), passed in by the - * caller so the client does not re-derive it after acquiring the lock (T1). - * The worker compare-and-swaps this against the on-disk inode. Undefined for - * a missing target (create) or when the host-local backend is in use. + * The caller's T0 observation, see `FilesystemWorkerExpectedIdentity`. + * + * REQUIRED for write operations: the client throws at runtime when a write + * arrives without one, so a JavaScript caller (which TypeScript cannot + * guard) fails loudly instead of silently skipping the queue-window CAS. + * Reads never participate in CAS: the client sends 'unchecked' for them + * automatically, so read callers have no way to get this wrong. */ - expectedIdentity?: { dev: string; ino: string }; + expectedIdentity?: FilesystemWorkerExpectedIdentity; } export type FilesystemWorkerClientErrorReason = @@ -174,22 +198,41 @@ export class FilesystemWorkerClient { if (!parsedOperation.success) throw clientError('invalid_operation', 'validation', requestId); const access = operationAccess(parsedOperation.data.kind); + // Reads never participate in CAS: the client sends 'unchecked' for them + // automatically, so read callers (including plain-JavaScript verifiers + // that bypass TypeScript) have no way to get the identity wrong. + // Writes require an explicit T0 state, enforced at runtime: a caller that + // omits it fails loudly here instead of silently skipping the + // queue-window CAS (#3487, maintainer review). + const writeIdentity = access === 'write' ? input.expectedIdentity : 'unchecked'; + if (access === 'write' && writeIdentity === undefined) { + throw clientError( + 'invalid_request', + 'validation', + requestId, + 'A write operation requires an explicit expectedIdentity: {dev, ino}, "missing", or "unchecked".', + ); + } const entryMode = operationUsesDirectoryEntry(parsedOperation.data); - const target: FilesystemWorkerTarget & { writableAncestor?: string } = await (entryMode - ? normalizeDirectoryEntryTarget({ - path: parsedOperation.data.path, - access, - cwd: canonicalCwd, - }) - : normalizeSandboxBoundaryPath({ - path: parsedOperation.data.path, - access, - scope: operationScope(parsedOperation.data.kind), - cwd: canonicalCwd, - }) - ).catch(() => { - throw clientError('invalid_operation', 'validation', requestId); - }); + // The wire identity contract is derived below from the caller's explicit + // expectedIdentity; the normalised target itself has no identity field, + // so the declared type omits it. + const target: Omit & { writableAncestor?: string } = + await (entryMode + ? normalizeDirectoryEntryTarget({ + path: parsedOperation.data.path, + access, + cwd: canonicalCwd, + }) + : normalizeSandboxBoundaryPath({ + path: parsedOperation.data.path, + access, + scope: operationScope(parsedOperation.data.kind), + cwd: canonicalCwd, + }) + ).catch(() => { + throw clientError('invalid_operation', 'validation', requestId); + }); // The identity was captured by the caller at lock acquisition (T0) and // passed in as expectedIdentity. Do NOT re-derive it here: re-deriving at // this point (after the lock is held) would sample the post-queue inode, @@ -203,14 +246,16 @@ export class FilesystemWorkerClient { // mutation proceed as a fresh exclusive create ("delete then rewrite" // stays a clean apply; a rename-swap is NOT this case — it leaves an // existing inode and is caught by the identity comparison instead). - // - T0 missing (no identity) but T1 existing: the target was created while + // - T0 missing ('missing') but T1 existing: the target was created while // this call waited. Writing would clobber content this call never saw, // so fail with a meaningful path_changed (never invalid_request). + // - 'unchecked': the caller does not participate in CAS. The target may + // be present at T1 without this being a race — the caller simply has no + // T0 snapshot, so nothing can be compared (#3484). + const targetExistsAtT1 = target.targetType !== 'missing'; const identity = - input.expectedIdentity && target.targetType !== 'missing' - ? input.expectedIdentity - : undefined; - if (!identity && target.targetType !== 'missing' && access === 'write') { + targetExistsAtT1 && typeof writeIdentity === 'object' ? writeIdentity : undefined; + if (targetExistsAtT1 && access === 'write' && writeIdentity === 'missing') { throw clientError( 'path_changed', 'validation', @@ -297,7 +342,11 @@ export class FilesystemWorkerClient { access, scope: target.scope, targetType: target.targetType, - ...(identity ? { identity } : {}), + // The execution-time identity contract. A concrete identity is only + // carried when the target still exists at T1; a target that vanished + // while queued (or was never there) is 'missing'; reads always say + // 'unchecked' (the client generates it, callers cannot get it wrong). + identity: typeof writeIdentity === 'object' ? (identity ?? 'missing') : writeIdentity, }, } as const; const requestJson = JSON.stringify(request); @@ -622,12 +671,6 @@ function deriveWorkerProfile( }; } -function operationAccess(kind: FilesystemWorkerOperation['kind']): 'read' | 'write' { - return kind === 'write' || kind === 'apply_patch' || kind === 'edit' || kind === 'format_json' - ? 'write' - : 'read'; -} - function operationScope(kind: FilesystemWorkerOperation['kind']): 'exact' | 'subtree' | 'auto' { if (kind === 'glob') return 'subtree'; return kind === 'grep' ? 'auto' : 'exact'; @@ -641,7 +684,7 @@ async function normalizeDirectoryEntryTarget(input: { path: string; cwd: string; access: 'read' | 'write'; -}): Promise { +}): Promise & { writableAncestor?: string }> { const target = await resolveCanonicalDirectoryEntryTarget(input.cwd, input.path); let targetType: FilesystemWorkerTarget['targetType']; try { diff --git a/packages/runtime/src/filesystem-worker/operations.ts b/packages/runtime/src/filesystem-worker/operations.ts index 5dad9dd871..ca59f41ec8 100644 --- a/packages/runtime/src/filesystem-worker/operations.ts +++ b/packages/runtime/src/filesystem-worker/operations.ts @@ -24,6 +24,7 @@ import { import { isSupportedImagePath, readWorkspaceImage } from '../image-file.js'; import { FILESYSTEM_WORKER_PROTOCOL_VERSION, + operationAccess, operationUsesDirectoryEntry, type FilesystemWorkerErrorCode, type FilesystemWorkerOperation, @@ -67,12 +68,6 @@ export type FilesystemWorkerGrepRunner = ( input: FilesystemWorkerGrepRunInput, ) => Promise; -function operationAccess(kind: FilesystemWorkerOperation['kind']): 'read' | 'write' { - return kind === 'write' || kind === 'apply_patch' || kind === 'edit' || kind === 'format_json' - ? 'write' - : 'read'; -} - export async function executeFilesystemWorkerRequest( request: FilesystemWorkerRequest, dependencies: FilesystemWorkerOperationDependencies = {}, @@ -159,13 +154,18 @@ export async function executeFilesystemOperation( // appeared in the gap is `path_changed`, never truncated. const handle = await openStableTarget({ path, - approvedIdentity: expectedTarget?.identity, + approvedIdentity: + typeof expectedTarget?.identity === 'object' ? expectedTarget.identity : undefined, + targetType: expectedTarget?.targetType, }); try { // Read-before-write (for the diff): only through the pinned descriptor. // An approved-missing target was just created by 'wx', so it is new. + // The wire identity is three-state (#3484): 'missing' is a truthy + // string, so a truthiness test can no longer stand in for "the target + // was approved as missing" — targetType is the authority here. let previous: 'new' | 'unknown' | string; - if (!expectedTarget?.identity) { + if (expectedTarget?.targetType === 'missing') { previous = 'new'; } else { try { @@ -210,7 +210,8 @@ export async function executeFilesystemOperation( else await compareAndDeleteEntry({ path, - approvedIdentity: expectedTarget?.identity, + approvedIdentity: + typeof expectedTarget?.identity === 'object' ? expectedTarget.identity : undefined, }); return { kind: 'apply_patch', ok: true, path }; } @@ -225,7 +226,9 @@ export async function executeFilesystemOperation( // rejected patch propagates before any truncation, so the file is intact. const handle = await openStableTarget({ path, - approvedIdentity: expectedTarget?.identity, + approvedIdentity: + typeof expectedTarget?.identity === 'object' ? expectedTarget.identity : undefined, + targetType: expectedTarget?.targetType, }); try { await readModifyWriteThroughHandle(handle, (existing) => @@ -248,7 +251,9 @@ export async function executeFilesystemOperation( ); const handle = await openStableTarget({ path, - approvedIdentity: expectedTarget?.identity, + approvedIdentity: + typeof expectedTarget?.identity === 'object' ? expectedTarget.identity : undefined, + targetType: expectedTarget?.targetType, }); try { const content = await handle.readFile('utf8'); @@ -294,7 +299,9 @@ export async function executeFilesystemOperation( ); const handle = await openStableTarget({ path, - approvedIdentity: expectedTarget?.identity, + approvedIdentity: + typeof expectedTarget?.identity === 'object' ? expectedTarget.identity : undefined, + targetType: expectedTarget?.targetType, }); try { const original = await handle.readFile('utf8'); @@ -485,30 +492,34 @@ async function assertTargetUnchanged( // while the call waited for the lock has a different inode even when its // canonical path and type still match. // - // A non-missing WRITE target MUST carry an identity — if it does not, the - // CAS is silently skipped and the entire defence collapses. Fail loudly - // rather than degrading to "no check", so a buggy caller that omits the - // identity is caught immediately instead of leaving the window open. Reads - // are exempt: they do not mutate, so there is no queue window to close. - if (access === 'write' && expected.targetType !== 'missing' && !expected.identity) { - throw operationError( - 'invalid_request', - 'A non-missing filesystem target must carry an identity for CAS.', - ); - } - if (expected.identity) { - const metadata = noFollowFinalSymlink - ? await fs.lstat(enforcementPath, { bigint: true }) - : await fs.stat(enforcementPath, { bigint: true }); - if ( - String(metadata.dev) !== expected.identity.dev || - String(metadata.ino) !== expected.identity.ino - ) { + // The wire carries one required three-state identity contract (#3484): + // - { dev, ino }: CAS against the on-disk inode. + // - 'missing': T0 saw no target but T1 does — something created it while + // this call waited. Writing would clobber content the caller never saw. + // - 'unchecked': the caller deliberately does not participate in CAS. + // Reads never mutate and are exempt either way. + if (access === 'write' && expected.targetType !== 'missing') { + if (expected.identity === 'missing') { throw operationError( 'path_changed', - 'The approved filesystem target changed before execution.', + 'The target was created while this call waited for the lock; re-read before writing.', ); } + if (typeof expected.identity === 'object') { + const metadata = noFollowFinalSymlink + ? await fs.lstat(enforcementPath, { bigint: true }) + : await fs.stat(enforcementPath, { bigint: true }); + if ( + String(metadata.dev) !== expected.identity.dev || + String(metadata.ino) !== expected.identity.ino + ) { + throw operationError( + 'path_changed', + 'The approved filesystem target changed before execution.', + ); + } + } + // identity === 'unchecked': nothing to compare, nothing to fail. } } diff --git a/packages/runtime/src/filesystem-worker/protocol.ts b/packages/runtime/src/filesystem-worker/protocol.ts index 00f09015cd..8f40a097c9 100644 --- a/packages/runtime/src/filesystem-worker/protocol.ts +++ b/packages/runtime/src/filesystem-worker/protocol.ts @@ -6,7 +6,16 @@ import { validateSandboxBoundaryExpansion } from '@maka/core/sandbox-boundary'; // inode that was authorised at lock acquisition instead of only the path // string. The identity is carried as strings because bigint cannot cross the // JSON protocol boundary. -export const FILESYSTEM_WORKER_PROTOCOL_VERSION = 6 as const; +export const FILESYSTEM_WORKER_PROTOCOL_VERSION = 7 as const; + +/** The single authority on which operation kinds are writes. Shared by the + * client (permission/identity decisions) and the worker (operation guards) so + * the set cannot drift. */ +export function operationAccess(kind: FilesystemWorkerOperation['kind']): 'read' | 'write' { + return kind === 'write' || kind === 'apply_patch' || kind === 'edit' || kind === 'format_json' + ? 'write' + : 'read'; +} const path = z.string().min(1).max(4096); const cwd = z.string().min(1).max(4096); @@ -53,16 +62,19 @@ export const FilesystemWorkerTargetSchema = z access: z.enum(['read', 'write']), scope: z.enum(['exact', 'subtree']), targetType: z.enum(['file', 'directory', 'symlink', 'other', 'missing']), - // Captured at lock acquisition (T0). Present (and required) for every - // targetType except 'missing'. The contract module (FilesystemTargetDescriptor) - // models this as a discriminated union; the wire schema keeps the field - // optional so it can omit it for missing targets, and the client that - // builds the request enforces the invariant. - identity: FilesystemTargetIdentitySchema.optional(), + // The execution-time identity contract, one required field (no separate + // T0 marker — a single three-state shape mirrors the client input, so an + // illegal combination cannot be expressed on the wire): + // - { dev, ino }: the T0 identity the worker must CAS against at T1. + // - 'missing': T0 saw no target; a target present at execution time was + // created while the call waited and must fail. + // - 'unchecked': the caller does not participate in CAS; the write + // proceeds without an identity comparison. + identity: FilesystemTargetIdentitySchema.or(z.literal('missing')).or(z.literal('unchecked')), }) .strict() .superRefine((target, context) => { - if (target.targetType === 'missing' && target.identity !== undefined) { + if (target.targetType === 'missing' && typeof target.identity === 'object') { context.addIssue({ code: 'custom', message: 'A missing target cannot carry an identity.', diff --git a/scripts/verify-macos-arm64-dmg.mjs b/scripts/verify-macos-arm64-dmg.mjs index c9635e13d8..11cf2ae5bb 100644 --- a/scripts/verify-macos-arm64-dmg.mjs +++ b/scripts/verify-macos-arm64-dmg.mjs @@ -56,6 +56,9 @@ export async function smokePackagedFilesystemWorker( access: 'write', scope: 'exact', targetType: 'missing', + // Protocol v7 requires the three-state identity; a write to a target + // approved as missing carries 'missing' (#3484 / #3487). + identity: 'missing', }, }; diff --git a/scripts/verify-windows-sandbox-e2e.mjs b/scripts/verify-windows-sandbox-e2e.mjs index 73c3840354..c10f01ad3a 100644 --- a/scripts/verify-windows-sandbox-e2e.mjs +++ b/scripts/verify-windows-sandbox-e2e.mjs @@ -110,7 +110,13 @@ export async function verifyWindowsSandboxWorkerE2E(appDirectoryPath) { // any launch. let parentEntryDenied = false; try { - await execute({ kind: 'write', path: join(workspace, 'missing.txt'), content: 'x' }); + // The target is genuinely missing; 'missing' is the truthful T0 state + // so the client lets the request through and the sandbox's own + // fail-closed parent-entry check is what rejects it (#3487). + await execute( + { kind: 'write', path: join(workspace, 'missing.txt'), content: 'x' }, + 'missing', + ); } catch (error) { parentEntryDenied = error instanceof FilesystemWorkerClientError && @@ -161,7 +167,12 @@ export async function verifyWindowsSandboxWorkerE2E(appDirectoryPath) { let denied = false; try { - await execute({ kind: 'write', path: join(outside, 'blocked.txt'), content: 'blocked' }); + // Same contract as above: a truthful T0 state so the client's own + // permission gate (not the identity validation) is what denies. + await execute( + { kind: 'write', path: join(outside, 'blocked.txt'), content: 'blocked' }, + 'missing', + ); } catch (error) { denied = error instanceof FilesystemWorkerClientError && error.reason === 'path_denied'; }