From a52d04ad4e1db1ed32caee365779d2635a5a933b Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Sun, 9 Aug 2026 23:10:47 +0200 Subject: [PATCH 1/4] fix: validate worker request fields and wipe a refused upload chunk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine host read `beginWrite`, `snapshot`, `download`, `openContentStream` and `readStream` arguments straight off an untrusted worker message. wasm-bindgen coerces rather than refuses, so a 16-character string reached `NodeId::from_bytes` as sixteen zero bytes and a string or NaN ToInt32'd into a byte count: a malformed request became a valid one against a different node or window. Every field the host reads now passes the codec's checkers, with a finite non-negative check for the counts. The sending side also dropped an upload chunk it refused before any transfer, leaving readable plaintext in a failed-over or closing tab's heap. `EngineClient.pushChunk` now scrubs the chunk its unknown-handle refusal never hands on, and `CorrelatedTransport.request` scrubs what a send would have transferred on every route to a pre-send rejection — a terminal error, a refused readiness gate, or a throwing send. Closes #1154 Closes #1155 --- .../client/src/broadcastTransport.test.ts | 14 ++ packages/client/src/broadcastTransport.ts | 10 +- .../client/src/correlatedTransport.test.ts | 146 ++++++++++++++++++ packages/client/src/correlatedTransport.ts | 40 ++++- packages/client/src/engineClient.test.ts | 6 +- packages/client/src/engineClient.ts | 7 +- packages/client/src/transport.ts | 20 ++- .../client/src/worker/commandCodec.test.ts | 14 +- packages/client/src/worker/commandCodec.ts | 31 +++- packages/client/src/worker/engineHost.test.ts | 116 ++++++++++++++ packages/client/src/worker/engineHost.ts | 39 +++-- 11 files changed, 401 insertions(+), 42 deletions(-) diff --git a/packages/client/src/broadcastTransport.test.ts b/packages/client/src/broadcastTransport.test.ts index e09bf5b7b7..23a386ee69 100644 --- a/packages/client/src/broadcastTransport.test.ts +++ b/packages/client/src/broadcastTransport.test.ts @@ -191,6 +191,20 @@ describe('broadcast transport ↔ leader relay', () => { expect(engine.commits).toEqual([handle]); }); + it('wipes an upload chunk it never gets onto a port', async () => { + const bus = new FakeBus(); + const ports = new FakeCourierNetwork(); + relayOn(bus, new FakeEngineTransport(), ports.courier('leader')); + // No port to move the plaintext out over, so this tab stays its terminal + // owner: a follower that cannot reach the leader keeps no readable chunk. + const follower = followerOn(bus, 'follower-1', unavailableCourier); + const plaintext = Uint8Array.of(4, 3, 2, 1); + + await expect(follower.pushChunk(1n, plaintext.buffer as ArrayBuffer)).rejects.toThrow(); + + expect([...plaintext]).toEqual([0, 0, 0, 0]); + }); + it('keeps upload plaintext and command arguments off the channel', async () => { const bus = new FakeBus(); const ports = new FakeCourierNetwork(); diff --git a/packages/client/src/broadcastTransport.ts b/packages/client/src/broadcastTransport.ts index 53c8633b97..1917301de6 100644 --- a/packages/client/src/broadcastTransport.ts +++ b/packages/client/src/broadcastTransport.ts @@ -211,9 +211,13 @@ export class BroadcastTransport extends CorrelatedTransport { build: (requestId: number) => PortRequest, transfer?: Transferable[] ): Promise { - return this.request(this.ensurePort(), (requestId, port) => { - port.postMessage(build(requestId), transfer); - }); + return this.request( + this.ensurePort(), + (requestId, port) => { + port.postMessage(build(requestId), transfer); + }, + transfer + ); } private ensurePort(): Promise { diff --git a/packages/client/src/correlatedTransport.test.ts b/packages/client/src/correlatedTransport.test.ts index 582bbd8358..1a0f973c8c 100644 --- a/packages/client/src/correlatedTransport.test.ts +++ b/packages/client/src/correlatedTransport.test.ts @@ -1,10 +1,156 @@ import { describe, expect, it } from 'vitest'; import { + CorrelatedTransport, EngineRequestError, engineErrorCode, isRecoverableEngineError, } from './correlatedTransport.js'; +import type { SnapshotDescriptor, WriteHandle } from './worker/protocol.js'; + +function unsupported(): never { + throw new Error('outside this probe'); +} + +/** + * A concrete transport wiring only the request skeleton: `pushChunk` carries a + * transfer, `open`/`breakDown` drive the gate and the terminal latch, and the + * rest of the engine surface is out of scope here. + */ +class ProbeTransport extends CorrelatedTransport { + private resolveGate!: () => void; + private rejectGate!: (error: Error) => void; + private readonly gate = new Promise((resolve, reject) => { + this.resolveGate = resolve; + this.rejectGate = reject; + }); + + constructor(private readonly onSend: (id: number) => void = () => undefined) { + super(); + this.gate.catch(() => undefined); + } + + pushChunk(_handle: WriteHandle, chunk: ArrayBuffer): Promise { + return this.dispatch(this.gate, (id) => this.onSend(id), [chunk]); + } + + open(): void { + this.resolveGate(); + } + + shut(error: Error): void { + this.rejectGate(error); + } + + breakDown(error: Error): void { + this.fail(error); + } + + answer(id: number): void { + this.settle(id, true); + } + + start(): Promise { + return unsupported(); + } + command(): Promise { + return unsupported(); + } + beginWrite(): Promise { + return unsupported(); + } + commitWrite(): Promise { + return unsupported(); + } + abortWrite(): Promise { + return unsupported(); + } + snapshot(): Promise { + return unsupported(); + } + siweChallenge(): Promise { + return unsupported(); + } + download(): Promise { + return unsupported(); + } + openContentStream(): Promise { + return unsupported(); + } + readStream(): Promise { + return unsupported(); + } + closeStream(): Promise { + return unsupported(); + } + close(): void { + unsupported(); + } +} + +const plaintext = (): Uint8Array => Uint8Array.of(1, 2, 3, 4); + +describe('CorrelatedTransport chunk ownership', () => { + it('leaves the chunk alone once the send has taken it', async () => { + const sent: number[] = []; + const probe = new ProbeTransport((id) => sent.push(id)); + probe.open(); + const chunk = plaintext(); + + const pushed = probe.pushChunk(1n, chunk.buffer as ArrayBuffer); + await Promise.resolve(); + probe.answer(sent[0]); + + // A real send transfers the buffer; wiping one that left is at best a no-op + // and at worst zeroes the bytes the receiver is about to seal. + await expect(pushed).resolves.toBeUndefined(); + expect(chunk).toEqual(plaintext()); + }); + + it('wipes the chunk of a request refused by an already-terminal transport', async () => { + const probe = new ProbeTransport(); + probe.open(); + probe.breakDown(new Error('engine transport closed')); + const chunk = plaintext(); + + await expect(probe.pushChunk(1n, chunk.buffer as ArrayBuffer)).rejects.toThrow('closed'); + expect(chunk).toEqual(new Uint8Array(4)); + }); + + it('wipes the chunk of a request the readiness gate refuses', async () => { + const probe = new ProbeTransport(); + const chunk = plaintext(); + + const pushed = probe.pushChunk(1n, chunk.buffer as ArrayBuffer); + probe.shut(new Error('leader changed; retry')); + + await expect(pushed).rejects.toThrow('leader changed; retry'); + expect(chunk).toEqual(new Uint8Array(4)); + }); + + it('wipes the chunk of a request the transport outlives its gate to refuse', async () => { + const probe = new ProbeTransport(); + const chunk = plaintext(); + + const pushed = probe.pushChunk(1n, chunk.buffer as ArrayBuffer); + probe.breakDown(new Error('engine transport closed')); + probe.open(); + + await expect(pushed).rejects.toThrow('closed'); + expect(chunk).toEqual(new Uint8Array(4)); + }); + + it('wipes the chunk of a send that throws', async () => { + const probe = new ProbeTransport(() => { + throw new Error('port is dead'); + }); + probe.open(); + const chunk = plaintext(); + + await expect(probe.pushChunk(1n, chunk.buffer as ArrayBuffer)).rejects.toThrow('port is dead'); + expect(chunk).toEqual(new Uint8Array(4)); + }); +}); describe('engineErrorCode', () => { it('reads the code off an engine failure and nothing else', () => { diff --git a/packages/client/src/correlatedTransport.ts b/packages/client/src/correlatedTransport.ts index 5afb742052..4e96f756e0 100644 --- a/packages/client/src/correlatedTransport.ts +++ b/packages/client/src/correlatedTransport.ts @@ -72,6 +72,19 @@ export function unknownHandle(kind: HandleKind): EngineRequestError { ); } +/** + * Scrubs the buffers a send would have transferred. A request that rejects + * before its send leaves this frame their terminal owner — nothing detaches + * them and no callee can reach them — so the plaintext is cleared here + * (AGENTS.md 7). A transferred buffer reads as empty, so a send that did run + * leaves this a no-op. + */ +function wipeCarried(transfer: Transferable[] | undefined): void { + for (const item of transfer ?? []) { + if (item instanceof ArrayBuffer && item.byteLength > 0) new Uint8Array(item).fill(0); + } +} + /** * Delivers one event to every listener, isolating a throwing subscriber so it * cannot drop the event for the rest. @@ -125,16 +138,24 @@ export abstract class CorrelatedTransport implements EngineTransport { * synchronous `send` failure deletes the pending entry before rejecting so it * is never stranded. Resolves with the response's result value (`undefined` * for a plain ack). + * + * `transfer` is what the send would have moved out of this realm; every route + * to a rejection without it scrubs them ([`wipeCarried`]). */ protected request( readyGate: Promise, - send: (id: number, gate: G) => void + send: (id: number, gate: G) => void, + transfer?: Transferable[] ): Promise { - if (this.terminalError) return Promise.reject(this.terminalError); + if (this.terminalError) { + wipeCarried(transfer); + return Promise.reject(this.terminalError); + } return readyGate.then( (gate) => new Promise((resolve, reject) => { if (this.terminalError) { + wipeCarried(transfer); reject(this.terminalError); return; } @@ -144,15 +165,24 @@ export abstract class CorrelatedTransport implements EngineTransport { send(id, gate); } catch (error) { this.pending.delete(id); + wipeCarried(transfer); reject(error instanceof Error ? error : new Error(String(error))); } - }) + }), + (error: unknown) => { + wipeCarried(transfer); + throw error; + } ); } /** The void-ack variant of [`request`](CorrelatedTransport.request). */ - protected dispatch(readyGate: Promise, send: (id: number) => void): Promise { - return this.request(readyGate, send); + protected dispatch( + readyGate: Promise, + send: (id: number) => void, + transfer?: Transferable[] + ): Promise { + return this.request(readyGate, send, transfer); } /** Correlates a response to its request id, resolving or rejecting it. */ diff --git a/packages/client/src/engineClient.test.ts b/packages/client/src/engineClient.test.ts index e312e0e110..031ddec9ee 100644 --- a/packages/client/src/engineClient.test.ts +++ b/packages/client/src/engineClient.test.ts @@ -214,9 +214,13 @@ describe('EngineClient leadership + transport swap', () => { expect.objectContaining({ type: 'pushChunk', handle: 1n }) ); - await expect(follower.pushChunk(stale, Uint8Array.of(9).buffer)).rejects.toMatchObject({ + const refused = Uint8Array.of(9, 9, 9, 9); + await expect(follower.pushChunk(stale, refused.buffer as ArrayBuffer)).rejects.toMatchObject({ code: 'unknownWriteHandle', }); + // Refused before any transfer, so this seam stayed the plaintext's terminal + // owner: a failed-over tab keeps no readable chunk in its heap. + expect(refused).toEqual(new Uint8Array(4)); await expect(follower.commitWrite(stale)).rejects.toMatchObject({ code: 'unknownWriteHandle', }); diff --git a/packages/client/src/engineClient.ts b/packages/client/src/engineClient.ts index fc5fdab8f6..b50fe7e8f0 100644 --- a/packages/client/src/engineClient.ts +++ b/packages/client/src/engineClient.ts @@ -187,7 +187,12 @@ export class EngineClient implements EngineTransport { pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { const inner = this.writes.resolve(handle); - if (inner === undefined) return Promise.reject(unknownHandle('write')); + if (inner === undefined) { + // Refused before any transfer, so this seam is the plaintext's terminal + // owner (security rule 7), exactly as `start` is for a secret it declines. + new Uint8Array(chunk).fill(0); + return Promise.reject(unknownHandle('write')); + } return this.current.pushChunk(inner, chunk); } diff --git a/packages/client/src/transport.ts b/packages/client/src/transport.ts index 6125e0cb0c..0b4afc6220 100644 --- a/packages/client/src/transport.ts +++ b/packages/client/src/transport.ts @@ -113,14 +113,19 @@ export class LocalTransport extends CorrelatedTransport { } start(secret: ArrayBuffer): Promise { - return this.dispatch(this.ready, (id) => - this.worker.postMessage({ type: 'start', id, secret }, [secret]) + const transfer = [secret]; + return this.dispatch( + this.ready, + (id) => this.worker.postMessage({ type: 'start', id, secret }, transfer), + transfer ); } command(command: CommandDescriptor, transfer: Transferable[]): Promise { - return this.dispatch(this.ready, (id) => - this.worker.postMessage({ type: 'command', id, command }, transfer) + return this.dispatch( + this.ready, + (id) => this.worker.postMessage({ type: 'command', id, command }, transfer), + transfer ); } @@ -131,8 +136,11 @@ export class LocalTransport extends CorrelatedTransport { } pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { - return this.dispatch(this.ready, (id) => - this.worker.postMessage({ type: 'pushChunk', id, handle, chunk }, [chunk]) + const transfer = [chunk]; + return this.dispatch( + this.ready, + (id) => this.worker.postMessage({ type: 'pushChunk', id, handle, chunk }, transfer), + transfer ); } diff --git a/packages/client/src/worker/commandCodec.test.ts b/packages/client/src/worker/commandCodec.test.ts index 3977f9196e..087d1fedac 100644 --- a/packages/client/src/worker/commandCodec.test.ts +++ b/packages/client/src/worker/commandCodec.test.ts @@ -102,11 +102,11 @@ describe('buildCommand', () => { it('rejects a wrong-typed string field rather than letting wasm-bindgen coerce it', () => { expect(refuses({ kind: 'rename', node: new Uint8Array(16), newName: 12345 })).toThrow( - 'invalid command field newName: number' + 'invalid request field newName: number' ); expect( refuses({ kind: 'create', parent: new Uint8Array(16), name: null, nodeKind: 'file' }) - ).toThrow('invalid command field name: null'); + ).toThrow('invalid request field name: null'); }); it('rejects a wrong-typed byte-array field', () => { @@ -117,22 +117,22 @@ describe('buildCommand', () => { recipientIdentityPublicKey: 'deadbeef', permission: 'read', }) - ).toThrow('invalid command field recipientIdentityPublicKey: string'); - expect(refuses({ kind: 'delete', node: [1, 2, 3] })).toThrow('invalid command field node'); + ).toThrow('invalid request field recipientIdentityPublicKey: string'); + expect(refuses({ kind: 'delete', node: [1, 2, 3] })).toThrow('invalid request field node'); }); it('rejects an unknown node kind or permission rather than defaulting one', () => { expect( refuses({ kind: 'create', parent: new Uint8Array(16), name: 'a', nodeKind: 'symlink' }) - ).toThrow('invalid command field nodeKind: string'); + ).toThrow('invalid request field nodeKind: string'); expect( refuses({ kind: 'createInviteLink', node: new Uint8Array(16), permission: 'admin' }) - ).toThrow('invalid command field permission: string'); + ).toThrow('invalid request field permission: string'); }); it('rejects an op id that is not the engine bigint', () => { expect(refuses({ kind: 'cancelUpload', opId: 7 })).toThrow( - 'invalid command field opId: number' + 'invalid request field opId: number' ); }); }); diff --git a/packages/client/src/worker/commandCodec.ts b/packages/client/src/worker/commandCodec.ts index d4cb7aa817..ff0077b6ca 100644 --- a/packages/client/src/worker/commandCodec.ts +++ b/packages/client/src/worker/commandCodec.ts @@ -28,14 +28,21 @@ import type { } from './engineWasm.js'; /** - * A descriptor crosses a realm boundary as plain data, so its fields arrive + * A request crosses a realm boundary as plain data, so its fields arrive * untrusted however they are typed here: a version-skewed peer can carry a * wrong-typed one, and wasm-bindgen would coerce it — a `12345` newName - * marshalled as `"12345"` — rather than reject it. Hence the checkers below - * take `unknown`, and every field a builder reads passes through one. + * marshalled as `"12345"`, a 16-character string set into a `Vec` as + * sixteen zero bytes — rather than reject it. Hence the checkers below take + * `unknown`, and every field the worker reads off a request passes through one. */ function invalidField(field: string, value: unknown): Error { - return new Error(`invalid command field ${field}: ${value === null ? 'null' : typeof value}`); + return new Error(`invalid request field ${field}: ${value === null ? 'null' : typeof value}`); +} + +/** An untrusted wire object; a non-object carries no fields at all. */ +export function record(value: unknown, field: string): Record { + if (typeof value !== 'object' || value === null) throw invalidField(field, value); + return value as Record; } function bytes(value: unknown, field: string): Uint8Array { @@ -43,17 +50,29 @@ function bytes(value: unknown, field: string): Uint8Array { return value; } -function text(value: unknown, field: string): string { +export function text(value: unknown, field: string): string { if (typeof value !== 'string') throw invalidField(field, value); return value; } +/** + * A byte count or offset. The number ABI coerces rather than rejects — a string + * or a `NaN` arrives as a valid-looking integer — so the range the engine can + * actually act on is checked here. + */ +export function count(value: unknown, field: string): number { + if (typeof value !== 'number' || !Number.isSafeInteger(value) || value < 0) { + throw invalidField(field, value); + } + return value; +} + function opId(value: unknown, field: string): bigint { if (typeof value !== 'bigint') throw invalidField(field, value); return value; } -function nodeId(wasm: EngineWasm, value: unknown, field: string): WasmNodeId { +export function nodeId(wasm: EngineWasm, value: unknown, field: string): WasmNodeId { return wasm.NodeId.fromBytes(bytes(value, field)); } diff --git a/packages/client/src/worker/engineHost.test.ts b/packages/client/src/worker/engineHost.test.ts index 9d3eeff0a6..0f0c2942ec 100644 --- a/packages/client/src/worker/engineHost.test.ts +++ b/packages/client/src/worker/engineHost.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { EngineHost } from './engineHost.js'; import type { EngineWasm } from './engineWasm.js'; +import type { WriteTarget } from './protocol.js'; /** The arguments one `EngineHandle` construction crossed the WASM boundary with. */ interface Constructed { @@ -42,6 +43,31 @@ function recordingWasm(): { wasm: EngineWasm; constructed: Constructed[] } { return { wasm, constructed }; } +/** + * A host over a wasm whose every call succeeds and records its arguments, so + * only the host's own field checks can refuse a request. + */ +function permissiveHost(): { host: EngineHost; calls: unknown[][] } { + const calls: unknown[][] = []; + const record = + (name: string) => + (...args: unknown[]): Promise => { + calls.push([name, ...args]); + return Promise.resolve(new Uint8Array(0)); + }; + const wasm = { + EngineHandle: class { + beginWrite = record('beginWrite'); + snapshot = record('snapshot'); + download = record('download'); + openContentStream = record('openContentStream'); + readStream = record('readStream'); + }, + NodeId: { fromBytes: (bytes: Uint8Array) => ({ bytes }) }, + } as unknown as EngineWasm; + return { host: new EngineHost(wasm, {}, { apiBaseUrl: 'https://api.example.test' }), calls }; +} + /** A host whose WASM `pushChunk` hands the view it was given to `onPush`. */ function pushingHost(onPush: (chunk: Uint8Array) => Promise): EngineHost { const wasm = { @@ -130,3 +156,93 @@ describe('EngineHost', () => { expect(plaintext).toEqual(new Uint8Array(4)); }); }); + +/** + * Request fields arrive off a worker message, so a version-skewed sender can + * carry a wrong-typed one. The WASM ABI coerces rather than refuses — a + * 16-character string sets into a `Vec` as sixteen zero bytes, a string or + * `NaN` ToInt32s into an offset — turning a malformed request into a valid one + * against the wrong node or window. + */ +describe('EngineHost request fields', () => { + const node = new Uint8Array(16).fill(3); + + it('opens a write on well-typed fields', async () => { + const { host, calls } = permissiveHost(); + + await host.beginWrite({ parent: node, name: 'a.txt' }, 4); + await host.beginWrite({ node }, 8); + + expect(calls[0]).toEqual(['beginWrite', { bytes: node }, 'a.txt', undefined, 4]); + expect(calls[1]).toEqual(['beginWrite', undefined, undefined, { bytes: node }, 8]); + }); + + it('reads a stream window on well-typed bounds', async () => { + const { host, calls } = permissiveHost(); + + await host.readStream(7n, 0, 1024); + + expect(calls[0]).toEqual(['readStream', 7n, 0, 1024]); + }); + + it.each([ + ['a string parent', { parent: 'sixteen bytes!!!', name: 'a.txt' }, 4, 'parent: string'], + ['a numeric name', { parent: node, name: 12345 }, 4, 'name: number'], + ['a string node', { node: 'sixteen bytes!!!' }, 4, 'node: string'], + ['a non-object target', 'a.txt', 4, 'target: string'], + ['a null target', null, 4, 'target: null'], + ['a string size', { node }, '4', 'size: string'], + ['a NaN size', { node }, Number.NaN, 'size: number'], + ['a fractional size', { node }, 1.5, 'size: number'], + ['a negative size', { node }, -1, 'size: number'], + ])('refuses a beginWrite carrying %s', async (_case, target, size, message) => { + const { host, calls } = permissiveHost(); + + await expect(host.beginWrite(target as WriteTarget, size as number)).rejects.toThrow( + `invalid request field ${message}` + ); + expect(calls).toEqual([]); + }); + + it('refuses a snapshot of a folder that is not bytes', async () => { + const { host, calls } = permissiveHost(); + + await expect(host.snapshot('root' as unknown as Uint8Array)).rejects.toThrow( + 'invalid request field folder: string' + ); + // `null` is the vault root, the one non-`Uint8Array` folder the wire allows. + await expect(host.snapshot(undefined as unknown as Uint8Array)).rejects.toThrow( + 'invalid request field folder: undefined' + ); + expect(calls).toEqual([]); + }); + + it.each(['download', 'openContentStream'] as const)( + 'refuses a %s of a non-node', + async (call) => { + const { host, calls } = permissiveHost(); + + await expect(host[call]('sixteen bytes!!!' as unknown as Uint8Array)).rejects.toThrow( + 'invalid request field node: string' + ); + expect(calls).toEqual([]); + } + ); + + it.each([ + ['offset', '0', 1024, 'offset: string'], + ['offset', Number.NaN, 1024, 'offset: number'], + ['length', 0, Number.POSITIVE_INFINITY, 'length: number'], + ['length', 0, -1, 'length: number'], + ])( + 'refuses a stream window whose %s is not a byte count', + async (_field, offset, length, message) => { + const { host, calls } = permissiveHost(); + + await expect(host.readStream(7n, offset as number, length as number)).rejects.toThrow( + `invalid request field ${message}` + ); + expect(calls).toEqual([]); + } + ); +}); diff --git a/packages/client/src/worker/engineHost.ts b/packages/client/src/worker/engineHost.ts index 0560e6ad5a..59696f6cb0 100644 --- a/packages/client/src/worker/engineHost.ts +++ b/packages/client/src/worker/engineHost.ts @@ -14,7 +14,15 @@ import type { } from './protocol.js'; import type { EngineWasm } from './engineWasm.js'; import type { EngineHostConfig } from '../spawnEngineWorker.js'; -import { buildCommand, readEvent, readSnapshot } from './commandCodec.js'; +import { + buildCommand, + count, + nodeId, + readEvent, + readSnapshot, + record, + text, +} from './commandCodec.js'; /** * The engine-facing surface the protocol server ([`serveEngine`]) drives. The @@ -107,20 +115,23 @@ export class EngineHost implements EngineHostLike { await this.handle.command(buildCommand(this.wasm, command)); } - beginWrite(target: WriteTarget, size: number): Promise { - if ('node' in target) { + // `async` here and below: a refused field rejects, never throws synchronously. + async beginWrite(target: WriteTarget, size: number): Promise { + const reserved = count(size, 'size'); + const fields = record(target, 'target'); + if ('node' in fields) { return this.handle.beginWrite( undefined, undefined, - this.wasm.NodeId.fromBytes(target.node), - size + nodeId(this.wasm, fields.node, 'node'), + reserved ); } return this.handle.beginWrite( - this.wasm.NodeId.fromBytes(target.parent), - target.name, + nodeId(this.wasm, fields.parent, 'parent'), + text(fields.name, 'name'), undefined, - size + reserved ); } @@ -138,7 +149,7 @@ export class EngineHost implements EngineHostLike { async snapshot(folder: Uint8Array | null): Promise { const view = await this.handle.snapshot( - folder === null ? undefined : this.wasm.NodeId.fromBytes(folder) + folder === null ? undefined : nodeId(this.wasm, folder, 'folder') ); return readSnapshot(this.wasm, view); } @@ -148,15 +159,17 @@ export class EngineHost implements EngineHostLike { } async download(node: Uint8Array): Promise { - return ownedBuffer(await this.handle.download(this.wasm.NodeId.fromBytes(node))); + return ownedBuffer(await this.handle.download(nodeId(this.wasm, node, 'node'))); } - openContentStream(node: Uint8Array): Promise { - return this.handle.openContentStream(this.wasm.NodeId.fromBytes(node)); + async openContentStream(node: Uint8Array): Promise { + return this.handle.openContentStream(nodeId(this.wasm, node, 'node')); } async readStream(handle: StreamHandle, offset: number, length: number): Promise { - return ownedBuffer(await this.handle.readStream(handle, offset, length)); + return ownedBuffer( + await this.handle.readStream(handle, count(offset, 'offset'), count(length, 'length')) + ); } async closeStream(handle: StreamHandle): Promise { From ae6c8e5f73e48b9c877ca58136b57190dc1aa784 Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Sun, 9 Aug 2026 23:25:15 +0200 Subject: [PATCH 2/4] refactor: tighten the request-field and chunk-wipe diff Name the transfer scrub `wipeTransfer` so it no longer collides with the relay's payload-shaped `wipeCarried`, state each ownership rationale once at its home, and cover the transport wiring that declares what a send would have moved: without a test, undoing either `LocalTransport` argument left every suite green. --- .../client/src/broadcastTransport.test.ts | 3 +- .../client/src/correlatedTransport.test.ts | 3 +- packages/client/src/correlatedTransport.ts | 12 +++---- packages/client/src/engineClient.test.ts | 2 -- packages/client/src/engineClient.ts | 3 +- packages/client/src/transport.test.ts | 13 +++++++ packages/client/src/worker/engineHost.test.ts | 36 +++++++++++++------ packages/client/src/worker/engineHost.ts | 1 - 8 files changed, 47 insertions(+), 26 deletions(-) diff --git a/packages/client/src/broadcastTransport.test.ts b/packages/client/src/broadcastTransport.test.ts index 23a386ee69..466f79b4aa 100644 --- a/packages/client/src/broadcastTransport.test.ts +++ b/packages/client/src/broadcastTransport.test.ts @@ -195,8 +195,7 @@ describe('broadcast transport ↔ leader relay', () => { const bus = new FakeBus(); const ports = new FakeCourierNetwork(); relayOn(bus, new FakeEngineTransport(), ports.courier('leader')); - // No port to move the plaintext out over, so this tab stays its terminal - // owner: a follower that cannot reach the leader keeps no readable chunk. + // No port to move the plaintext out over, so this tab stays its owner. const follower = followerOn(bus, 'follower-1', unavailableCourier); const plaintext = Uint8Array.of(4, 3, 2, 1); diff --git a/packages/client/src/correlatedTransport.test.ts b/packages/client/src/correlatedTransport.test.ts index 1a0f973c8c..8a77c87b04 100644 --- a/packages/client/src/correlatedTransport.test.ts +++ b/packages/client/src/correlatedTransport.test.ts @@ -101,8 +101,7 @@ describe('CorrelatedTransport chunk ownership', () => { await Promise.resolve(); probe.answer(sent[0]); - // A real send transfers the buffer; wiping one that left is at best a no-op - // and at worst zeroes the bytes the receiver is about to seal. + // Wiping a sent chunk would zero the bytes the receiver is about to seal. await expect(pushed).resolves.toBeUndefined(); expect(chunk).toEqual(plaintext()); }); diff --git a/packages/client/src/correlatedTransport.ts b/packages/client/src/correlatedTransport.ts index 4e96f756e0..e8fd2052ee 100644 --- a/packages/client/src/correlatedTransport.ts +++ b/packages/client/src/correlatedTransport.ts @@ -79,7 +79,7 @@ export function unknownHandle(kind: HandleKind): EngineRequestError { * (AGENTS.md 7). A transferred buffer reads as empty, so a send that did run * leaves this a no-op. */ -function wipeCarried(transfer: Transferable[] | undefined): void { +function wipeTransfer(transfer: Transferable[] | undefined): void { for (const item of transfer ?? []) { if (item instanceof ArrayBuffer && item.byteLength > 0) new Uint8Array(item).fill(0); } @@ -140,7 +140,7 @@ export abstract class CorrelatedTransport implements EngineTransport { * for a plain ack). * * `transfer` is what the send would have moved out of this realm; every route - * to a rejection without it scrubs them ([`wipeCarried`]). + * to a rejection without it scrubs them ([`wipeTransfer`]). */ protected request( readyGate: Promise, @@ -148,14 +148,14 @@ export abstract class CorrelatedTransport implements EngineTransport { transfer?: Transferable[] ): Promise { if (this.terminalError) { - wipeCarried(transfer); + wipeTransfer(transfer); return Promise.reject(this.terminalError); } return readyGate.then( (gate) => new Promise((resolve, reject) => { if (this.terminalError) { - wipeCarried(transfer); + wipeTransfer(transfer); reject(this.terminalError); return; } @@ -165,12 +165,12 @@ export abstract class CorrelatedTransport implements EngineTransport { send(id, gate); } catch (error) { this.pending.delete(id); - wipeCarried(transfer); + wipeTransfer(transfer); reject(error instanceof Error ? error : new Error(String(error))); } }), (error: unknown) => { - wipeCarried(transfer); + wipeTransfer(transfer); throw error; } ); diff --git a/packages/client/src/engineClient.test.ts b/packages/client/src/engineClient.test.ts index 031ddec9ee..f2c03f1d97 100644 --- a/packages/client/src/engineClient.test.ts +++ b/packages/client/src/engineClient.test.ts @@ -218,8 +218,6 @@ describe('EngineClient leadership + transport swap', () => { await expect(follower.pushChunk(stale, refused.buffer as ArrayBuffer)).rejects.toMatchObject({ code: 'unknownWriteHandle', }); - // Refused before any transfer, so this seam stayed the plaintext's terminal - // owner: a failed-over tab keeps no readable chunk in its heap. expect(refused).toEqual(new Uint8Array(4)); await expect(follower.commitWrite(stale)).rejects.toMatchObject({ code: 'unknownWriteHandle', diff --git a/packages/client/src/engineClient.ts b/packages/client/src/engineClient.ts index b50fe7e8f0..6e105b266e 100644 --- a/packages/client/src/engineClient.ts +++ b/packages/client/src/engineClient.ts @@ -188,8 +188,7 @@ export class EngineClient implements EngineTransport { pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { const inner = this.writes.resolve(handle); if (inner === undefined) { - // Refused before any transfer, so this seam is the plaintext's terminal - // owner (security rule 7), exactly as `start` is for a secret it declines. + // Refused before any transfer: this seam is the chunk's terminal owner. new Uint8Array(chunk).fill(0); return Promise.reject(unknownHandle('write')); } diff --git a/packages/client/src/transport.test.ts b/packages/client/src/transport.test.ts index 422d584bba..87502d82b2 100644 --- a/packages/client/src/transport.test.ts +++ b/packages/client/src/transport.test.ts @@ -237,6 +237,19 @@ describe('LocalTransport', () => { expect(posted.transfer).toEqual([secret]); }); + it.each([ + ['the secret', (t: LocalTransport, buffer: ArrayBuffer) => t.start(buffer)], + ['an upload chunk', (t: LocalTransport, buffer: ArrayBuffer) => t.pushChunk(1n, buffer)], + ])('wipes %s a torn-down worker never took', async (_case, send) => { + const transport = new LocalTransport(new FakeWorker()); + transport.close(); + const plaintext = Uint8Array.of(1, 2, 3, 4); + + await expect(send(transport, plaintext.buffer as ArrayBuffer)).rejects.toThrow('closed'); + + expect(plaintext).toEqual(new Uint8Array(4)); + }); + it('transfers the chunk buffer on pushChunk and resolves the write handle and op id', async () => { const worker = new FakeWorker(); const transport = new LocalTransport(worker); diff --git a/packages/client/src/worker/engineHost.test.ts b/packages/client/src/worker/engineHost.test.ts index 0f0c2942ec..1f1d7c7a1e 100644 --- a/packages/client/src/worker/engineHost.test.ts +++ b/packages/client/src/worker/engineHost.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest'; +import { fakeWasmEnums } from '../testkit.js'; import { EngineHost } from './engineHost.js'; import type { EngineWasm } from './engineWasm.js'; import type { WriteTarget } from './protocol.js'; @@ -43,6 +44,17 @@ function recordingWasm(): { wasm: EngineWasm; constructed: Constructed[] } { return { wasm, constructed }; } +const emptyView = { + root: new Uint8Array(16), + folder: new Uint8Array(16), + folderName: '', + children: [], + ancestors: [], + deadLetters: [], + retainedRecords: 0, + staleness: fakeWasmEnums.Staleness.Fresh, +}; + /** * A host over a wasm whose every call succeeds and records its arguments, so * only the host's own field checks can refuse a request. @@ -50,15 +62,16 @@ function recordingWasm(): { wasm: EngineWasm; constructed: Constructed[] } { function permissiveHost(): { host: EngineHost; calls: unknown[][] } { const calls: unknown[][] = []; const record = - (name: string) => + (name: string, result: unknown = new Uint8Array(0)) => (...args: unknown[]): Promise => { calls.push([name, ...args]); - return Promise.resolve(new Uint8Array(0)); + return Promise.resolve(result); }; const wasm = { + ...fakeWasmEnums, EngineHandle: class { beginWrite = record('beginWrite'); - snapshot = record('snapshot'); + snapshot = record('snapshot', emptyView); download = record('download'); openContentStream = record('openContentStream'); readStream = record('readStream'); @@ -157,13 +170,7 @@ describe('EngineHost', () => { }); }); -/** - * Request fields arrive off a worker message, so a version-skewed sender can - * carry a wrong-typed one. The WASM ABI coerces rather than refuses — a - * 16-character string sets into a `Vec` as sixteen zero bytes, a string or - * `NaN` ToInt32s into an offset — turning a malformed request into a valid one - * against the wrong node or window. - */ +/** Untrusted request fields, refused rather than coerced (`invalidField`). */ describe('EngineHost request fields', () => { const node = new Uint8Array(16).fill(3); @@ -204,13 +211,20 @@ describe('EngineHost request fields', () => { expect(calls).toEqual([]); }); + it('lists the vault root for the one folder that is not bytes', async () => { + const { host, calls } = permissiveHost(); + + await host.snapshot(null); + + expect(calls).toEqual([['snapshot', undefined]]); + }); + it('refuses a snapshot of a folder that is not bytes', async () => { const { host, calls } = permissiveHost(); await expect(host.snapshot('root' as unknown as Uint8Array)).rejects.toThrow( 'invalid request field folder: string' ); - // `null` is the vault root, the one non-`Uint8Array` folder the wire allows. await expect(host.snapshot(undefined as unknown as Uint8Array)).rejects.toThrow( 'invalid request field folder: undefined' ); diff --git a/packages/client/src/worker/engineHost.ts b/packages/client/src/worker/engineHost.ts index 59696f6cb0..3ae5bbb5cd 100644 --- a/packages/client/src/worker/engineHost.ts +++ b/packages/client/src/worker/engineHost.ts @@ -115,7 +115,6 @@ export class EngineHost implements EngineHostLike { await this.handle.command(buildCommand(this.wasm, command)); } - // `async` here and below: a refused field rejects, never throws synchronously. async beginWrite(target: WriteTarget, size: number): Promise { const reserved = count(size, 'size'); const fields = record(target, 'target'); From 816668cda701d7298cef2ac89218edcd058f0170 Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Sun, 9 Aug 2026 23:36:19 +0200 Subject: [PATCH 3/4] fix: check the transferred payloads and scrub a closed client's secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security-review follow-ups on the same two boundaries. `start` and `pushChunk` took their payload straight into `new Uint8Array(buffer)`, which coerces: a view is copied rather than referenced, so the scrub afterwards zeroes the copy and leaves the sender's plaintext, and a transferable that is not an `ArrayBuffer` yields a zero-length push. Both now go through a buffer check like every other field. `EngineClient.start` refused a closed client before scrubbing the secret it had already decided not to forward — the same refuse-before-transfer shape as the chunk. The seam contract now says outright that a buffer is consumed on every outcome, so a retry re-reads its source rather than re-sending one a retryable rejection scrubbed. --- packages/client/src/engineClient.test.ts | 11 +++++++++++ packages/client/src/engineClient.ts | 7 ++++--- packages/client/src/transport.ts | 9 ++++++++- packages/client/src/worker/commandCodec.ts | 10 ++++++++++ packages/client/src/worker/engineHost.test.ts | 16 ++++++++++++++++ packages/client/src/worker/engineHost.ts | 9 +++++---- 6 files changed, 54 insertions(+), 8 deletions(-) diff --git a/packages/client/src/engineClient.test.ts b/packages/client/src/engineClient.test.ts index f2c03f1d97..b17a0f8d45 100644 --- a/packages/client/src/engineClient.test.ts +++ b/packages/client/src/engineClient.test.ts @@ -186,6 +186,17 @@ describe('EngineClient leadership + transport swap', () => { await leader.dispose(); }); + it('scrubs the login secret a closed client refuses', async () => { + const { tab } = origin(); + const client = tab(); + await client.dispose(); + const secret = Uint8Array.of(1, 2, 3, 4); + + await expect(client.start(secret.buffer as ArrayBuffer)).rejects.toThrow('closed'); + + expect(secret).toEqual(new Uint8Array(4)); + }); + it('refuses a write handle minted by a leadership that has been replaced', async () => { const { tab, workers } = origin(); const secretSource = { diff --git a/packages/client/src/engineClient.ts b/packages/client/src/engineClient.ts index 6e105b266e..dd723f47d6 100644 --- a/packages/client/src/engineClient.ts +++ b/packages/client/src/engineClient.ts @@ -161,13 +161,14 @@ export class EngineClient implements EngineTransport { // --- EngineTransport --- start(secret: ArrayBuffer): Promise { - if (this.role === 'closed') return Promise.reject(new Error('engine client closed')); // This seam is the secret's terminal owner (security rule 7). On the leader // path the worker becomes the terminal owner — `LocalTransport.start` // transfers the buffer in (neutered), never copied. On the follower path the - // keyless transport gets no secret: we scrub the buffer we decided not to use - // right here, rather than in a callee that would be zeroing someone else's. + // keyless transport gets no secret, and a closed client no transport at all: + // we scrub the buffer we decided not to use right here, rather than in a + // callee that would be zeroing someone else's. if (this.role !== 'leader') new Uint8Array(secret).fill(0); + if (this.role === 'closed') return Promise.reject(new Error('engine client closed')); return this.current.start(secret).then(() => { this.started = true; }); diff --git a/packages/client/src/transport.ts b/packages/client/src/transport.ts index 0b4afc6220..0c4a05f8b6 100644 --- a/packages/client/src/transport.ts +++ b/packages/client/src/transport.ts @@ -25,7 +25,14 @@ import type { export type EngineEventListener = (event: EventDescriptor) => void; export interface EngineTransport { - /** Hands the login secret to the engine once (transferred, not copied). */ + /** + * Hands the login secret to the engine once (transferred, not copied). + * + * Every buffer this seam takes is consumed on **every** outcome: transferred + * away when the send runs, scrubbed in place when the call is refused before + * it (security rule 7). A retry must therefore re-read its source rather than + * re-send the buffer a retryable rejection handed back. + */ start(secret: ArrayBuffer): Promise; /** Sends one command; `transfer` lists any owned buffers to move, not copy. */ command(command: CommandDescriptor, transfer: Transferable[]): Promise; diff --git a/packages/client/src/worker/commandCodec.ts b/packages/client/src/worker/commandCodec.ts index ff0077b6ca..7d73ae6c92 100644 --- a/packages/client/src/worker/commandCodec.ts +++ b/packages/client/src/worker/commandCodec.ts @@ -50,6 +50,16 @@ function bytes(value: unknown, field: string): Uint8Array { return value; } +/** + * A transferred payload. `new Uint8Array(value)` coerces anything else into a + * plausible view — a string of digits becomes that many zero bytes — so the + * buffer is checked before a view is taken over it. + */ +export function buffer(value: unknown, field: string): ArrayBuffer { + if (!(value instanceof ArrayBuffer)) throw invalidField(field, value); + return value; +} + export function text(value: unknown, field: string): string { if (typeof value !== 'string') throw invalidField(field, value); return value; diff --git a/packages/client/src/worker/engineHost.test.ts b/packages/client/src/worker/engineHost.test.ts index 1f1d7c7a1e..ad12767648 100644 --- a/packages/client/src/worker/engineHost.test.ts +++ b/packages/client/src/worker/engineHost.test.ts @@ -70,6 +70,8 @@ function permissiveHost(): { host: EngineHost; calls: unknown[][] } { const wasm = { ...fakeWasmEnums, EngineHandle: class { + start = record('start'); + pushChunk = record('pushChunk'); beginWrite = record('beginWrite'); snapshot = record('snapshot', emptyView); download = record('download'); @@ -211,6 +213,20 @@ describe('EngineHost request fields', () => { expect(calls).toEqual([]); }); + it('refuses a transferred payload that is not a buffer', async () => { + const { host, calls } = permissiveHost(); + + await expect(host.start('hunter2' as unknown as ArrayBuffer)).rejects.toThrow( + 'invalid request field secret: string' + ); + // A view is not the transfer the wire declares, and `new Uint8Array(view)` + // would copy it — leaving the sender's plaintext for the scrub to miss. + await expect(host.pushChunk(7n, Uint8Array.of(1, 2) as unknown as ArrayBuffer)).rejects.toThrow( + 'invalid request field chunk: object' + ); + expect(calls).toEqual([]); + }); + it('lists the vault root for the one folder that is not bytes', async () => { const { host, calls } = permissiveHost(); diff --git a/packages/client/src/worker/engineHost.ts b/packages/client/src/worker/engineHost.ts index 3ae5bbb5cd..45a62854d2 100644 --- a/packages/client/src/worker/engineHost.ts +++ b/packages/client/src/worker/engineHost.ts @@ -15,6 +15,7 @@ import type { import type { EngineWasm } from './engineWasm.js'; import type { EngineHostConfig } from '../spawnEngineWorker.js'; import { + buffer, buildCommand, count, nodeId, @@ -107,8 +108,8 @@ export class EngineHost implements EngineHostLike { } } - start(secret: ArrayBuffer): Promise { - return this.scrubbing(secret, (view) => this.handle.start(view)); + async start(secret: ArrayBuffer): Promise { + return this.scrubbing(buffer(secret, 'secret'), (view) => this.handle.start(view)); } async command(command: CommandDescriptor): Promise { @@ -134,8 +135,8 @@ export class EngineHost implements EngineHostLike { ); } - pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { - return this.scrubbing(chunk, (view) => this.handle.pushChunk(handle, view)); + async pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { + return this.scrubbing(buffer(chunk, 'chunk'), (view) => this.handle.pushChunk(handle, view)); } commitWrite(handle: WriteHandle): Promise { From b718e2f63168891aa9f2fcdb92a7344468dfa166 Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Mon, 10 Aug 2026 20:59:47 +0000 Subject: [PATCH 4/4] fix: brand buffers by realm and check the envelope and handles at the worker Three fields reached past the boundary this PR exists to guard. wipeTransfer branded transferables with instanceof ArrayBuffer, which is false for one minted in another realm, so a secret arriving from a worker or a frame skipped the scrub. Branded by the byteLength getter now, which answers across realms and returns null for everything else. buildCommand read descriptor.kind before validating the envelope, so a null or primitive command answered with a TypeError or an unknown-kind error naming undefined, rather than the invalid-field refusal every other malformed input gets. Write and stream handles crossed to WASM unchecked while their sibling fields were validated. minted() is the shared bigint check, applied at pushChunk, commitWrite, abortWrite, readStream and closeStream. commitWrite becomes async so its refusal rejects like the other four rather than throwing synchronously out of the call. --- .../client/src/correlatedTransport.test.ts | 17 +++++++++++++++++ packages/client/src/correlatedTransport.ts | 19 ++++++++++++++++++- .../client/src/worker/commandCodec.test.ts | 11 +++++++++++ packages/client/src/worker/commandCodec.ts | 14 ++++++++++++-- packages/client/src/worker/engineHost.test.ts | 15 +++++++++++++++ packages/client/src/worker/engineHost.ts | 18 ++++++++++++------ 6 files changed, 85 insertions(+), 9 deletions(-) diff --git a/packages/client/src/correlatedTransport.test.ts b/packages/client/src/correlatedTransport.test.ts index 8a77c87b04..db23190d8d 100644 --- a/packages/client/src/correlatedTransport.test.ts +++ b/packages/client/src/correlatedTransport.test.ts @@ -149,6 +149,23 @@ describe('CorrelatedTransport chunk ownership', () => { await expect(probe.pushChunk(1n, chunk.buffer as ArrayBuffer)).rejects.toThrow('port is dead'); expect(chunk).toEqual(new Uint8Array(4)); }); + + it('wipes a chunk minted in another realm, which instanceof does not answer for', async () => { + // A buffer from a worker or a frame is an ArrayBuffer that `instanceof` + // calls false, and a secret that arrived from there needs the same scrub. + const { runInNewContext } = await import('node:vm'); + const foreign = runInNewContext( + 'const b = new ArrayBuffer(4); new Uint8Array(b).set([9, 9, 9, 9]); ({ b, v: new Uint8Array(b) })' + ) as { b: ArrayBuffer; v: Uint8Array }; + expect(foreign.b instanceof ArrayBuffer).toBe(false); + + const probe = new ProbeTransport(); + probe.open(); + probe.breakDown(new Error('engine transport closed')); + + await expect(probe.pushChunk(1n, foreign.b)).rejects.toThrow('closed'); + expect([...foreign.v]).toEqual([0, 0, 0, 0]); + }); }); describe('engineErrorCode', () => { diff --git a/packages/client/src/correlatedTransport.ts b/packages/client/src/correlatedTransport.ts index e8fd2052ee..d1d9428efc 100644 --- a/packages/client/src/correlatedTransport.ts +++ b/packages/client/src/correlatedTransport.ts @@ -72,6 +72,22 @@ export function unknownHandle(kind: HandleKind): EngineRequestError { ); } +/** + * An `ArrayBuffer`'s length, or `null` for anything that is not one. Branding + * by the `byteLength` getter rather than `instanceof`, which answers false for + * a buffer minted in another realm — a worker's, a frame's — leaving a secret + * that reached this list from there unscrubbed. + */ +const byteLengthOf = Object.getOwnPropertyDescriptor(ArrayBuffer.prototype, 'byteLength')?.get; + +function bufferLength(item: Transferable): number | null { + try { + return (byteLengthOf?.call(item) as number | undefined) ?? null; + } catch { + return null; + } +} + /** * Scrubs the buffers a send would have transferred. A request that rejects * before its send leaves this frame their terminal owner — nothing detaches @@ -81,7 +97,8 @@ export function unknownHandle(kind: HandleKind): EngineRequestError { */ function wipeTransfer(transfer: Transferable[] | undefined): void { for (const item of transfer ?? []) { - if (item instanceof ArrayBuffer && item.byteLength > 0) new Uint8Array(item).fill(0); + const length = bufferLength(item); + if (length !== null && length > 0) new Uint8Array(item as ArrayBuffer).fill(0); } } diff --git a/packages/client/src/worker/commandCodec.test.ts b/packages/client/src/worker/commandCodec.test.ts index 087d1fedac..b3117e8421 100644 --- a/packages/client/src/worker/commandCodec.test.ts +++ b/packages/client/src/worker/commandCodec.test.ts @@ -100,6 +100,17 @@ describe('buildCommand', () => { expect(refuses({ kind: 'telepathy' })).toThrow('unknown command kind: telepathy'); }); + it('refuses an envelope that is not a command before it reads a kind off it', () => { + // A non-object answers `undefined` for every field, so without this the + // refusal is a TypeError on null, or an unknown-kind error naming + // `undefined` — neither of which tells a peer which field was wrong. + expect(refuses(null)).toThrow('invalid request field command: null'); + expect(refuses(42)).toThrow('invalid request field command: number'); + expect(refuses('rename')).toThrow('invalid request field command: string'); + expect(refuses({ kind: 12345 })).toThrow('invalid request field command.kind: number'); + expect(refuses({})).toThrow('invalid request field command.kind: undefined'); + }); + it('rejects a wrong-typed string field rather than letting wasm-bindgen coerce it', () => { expect(refuses({ kind: 'rename', node: new Uint8Array(16), newName: 12345 })).toThrow( 'invalid request field newName: number' diff --git a/packages/client/src/worker/commandCodec.ts b/packages/client/src/worker/commandCodec.ts index 7d73ae6c92..819433f88e 100644 --- a/packages/client/src/worker/commandCodec.ts +++ b/packages/client/src/worker/commandCodec.ts @@ -77,7 +77,13 @@ export function count(value: unknown, field: string): number { return value; } -function opId(value: unknown, field: string): bigint { +/** + * A value the engine minted and a peer is handing back — an op id, or a write + * or stream handle. The bigint ABI throws on a non-bigint where the number one + * would coerce, so the refusal is spelled here in the same words as its + * neighbours rather than left to wasm-bindgen. + */ +export function minted(value: unknown, field: string): bigint { if (typeof value !== 'bigint') throw invalidField(field, value); return value; } @@ -108,6 +114,10 @@ function unknownCommand(descriptor: never): Error { } export function buildCommand(wasm: EngineWasm, descriptor: CommandDescriptor): WasmCommand { + // The envelope is a field like any other: read `kind` off a non-object and + // the refusal is a TypeError, or an unknown-kind error naming `undefined`, + // rather than the invalid-field answer every other malformed input gets. + text(record(descriptor, 'command').kind, 'command.kind'); switch (descriptor.kind) { case 'create': return wasm.Command.create( @@ -128,7 +138,7 @@ export function buildCommand(wasm: EngineWasm, descriptor: CommandDescriptor): W nodeId(wasm, descriptor.newParent, 'newParent') ); case 'cancelUpload': - return wasm.Command.cancelUpload(opId(descriptor.opId, 'opId')); + return wasm.Command.cancelUpload(minted(descriptor.opId, 'opId')); case 'setFocus': return wasm.Command.setFocus( descriptor.node === null ? undefined : nodeId(wasm, descriptor.node, 'node') diff --git a/packages/client/src/worker/engineHost.test.ts b/packages/client/src/worker/engineHost.test.ts index ad12767648..94e47856fa 100644 --- a/packages/client/src/worker/engineHost.test.ts +++ b/packages/client/src/worker/engineHost.test.ts @@ -213,6 +213,21 @@ describe('EngineHost request fields', () => { expect(calls).toEqual([]); }); + it.each([ + ['pushChunk', (host: EngineHost) => host.pushChunk('7' as never, new ArrayBuffer(2))], + ['commitWrite', (host: EngineHost) => host.commitWrite(7 as never)], + ['abortWrite', (host: EngineHost) => host.abortWrite(null as never)], + ['readStream', (host: EngineHost) => host.readStream('7' as never, 0, 8)], + ['closeStream', (host: EngineHost) => host.closeStream(undefined as never)], + ])('refuses a %s carrying a handle the engine never minted', async (_case, call) => { + const { host, calls } = permissiveHost(); + + // A handle is a bigint the engine minted. The number ABI would coerce one + // of another type into a plausible table index rather than refuse it. + await expect(call(host)).rejects.toThrow('invalid request field handle'); + expect(calls).toEqual([]); + }); + it('refuses a transferred payload that is not a buffer', async () => { const { host, calls } = permissiveHost(); diff --git a/packages/client/src/worker/engineHost.ts b/packages/client/src/worker/engineHost.ts index 45a62854d2..d793ed25fd 100644 --- a/packages/client/src/worker/engineHost.ts +++ b/packages/client/src/worker/engineHost.ts @@ -18,6 +18,7 @@ import { buffer, buildCommand, count, + minted, nodeId, readEvent, readSnapshot, @@ -136,15 +137,16 @@ export class EngineHost implements EngineHostLike { } async pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { - return this.scrubbing(buffer(chunk, 'chunk'), (view) => this.handle.pushChunk(handle, view)); + const write = minted(handle, 'handle'); + return this.scrubbing(buffer(chunk, 'chunk'), (view) => this.handle.pushChunk(write, view)); } - commitWrite(handle: WriteHandle): Promise { - return this.handle.commitWrite(handle); + async commitWrite(handle: WriteHandle): Promise { + return this.handle.commitWrite(minted(handle, 'handle')); } async abortWrite(handle: WriteHandle): Promise { - await this.handle.abortWrite(handle); + await this.handle.abortWrite(minted(handle, 'handle')); } async snapshot(folder: Uint8Array | null): Promise { @@ -168,12 +170,16 @@ export class EngineHost implements EngineHostLike { async readStream(handle: StreamHandle, offset: number, length: number): Promise { return ownedBuffer( - await this.handle.readStream(handle, count(offset, 'offset'), count(length, 'length')) + await this.handle.readStream( + minted(handle, 'handle'), + count(offset, 'offset'), + count(length, 'length') + ) ); } async closeStream(handle: StreamHandle): Promise { - await this.handle.closeStream(handle); + await this.handle.closeStream(minted(handle, 'handle')); } async nextEvent(): Promise {