diff --git a/packages/client/src/broadcastTransport.test.ts b/packages/client/src/broadcastTransport.test.ts index e09bf5b7b7..466f79b4aa 100644 --- a/packages/client/src/broadcastTransport.test.ts +++ b/packages/client/src/broadcastTransport.test.ts @@ -191,6 +191,19 @@ 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 owner. + 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..db23190d8d 100644 --- a/packages/client/src/correlatedTransport.test.ts +++ b/packages/client/src/correlatedTransport.test.ts @@ -1,10 +1,172 @@ 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]); + + // Wiping a sent chunk would zero 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)); + }); + + 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', () => { 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..d1d9428efc 100644 --- a/packages/client/src/correlatedTransport.ts +++ b/packages/client/src/correlatedTransport.ts @@ -72,6 +72,36 @@ 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 + * 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 wipeTransfer(transfer: Transferable[] | undefined): void { + for (const item of transfer ?? []) { + const length = bufferLength(item); + if (length !== null && length > 0) new Uint8Array(item as ArrayBuffer).fill(0); + } +} + /** * Delivers one event to every listener, isolating a throwing subscriber so it * cannot drop the event for the rest. @@ -125,16 +155,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 ([`wipeTransfer`]). */ 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) { + wipeTransfer(transfer); + return Promise.reject(this.terminalError); + } return readyGate.then( (gate) => new Promise((resolve, reject) => { if (this.terminalError) { + wipeTransfer(transfer); reject(this.terminalError); return; } @@ -144,15 +182,24 @@ export abstract class CorrelatedTransport implements EngineTransport { send(id, gate); } catch (error) { this.pending.delete(id); + wipeTransfer(transfer); reject(error instanceof Error ? error : new Error(String(error))); } - }) + }), + (error: unknown) => { + wipeTransfer(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..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 = { @@ -214,9 +225,11 @@ 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', }); + 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..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; }); @@ -187,7 +188,11 @@ 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: this seam is the chunk's terminal owner. + new Uint8Array(chunk).fill(0); + return Promise.reject(unknownHandle('write')); + } return this.current.pushChunk(inner, chunk); } 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/transport.ts b/packages/client/src/transport.ts index 6125e0cb0c..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; @@ -113,14 +120,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 +143,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..b3117e8421 100644 --- a/packages/client/src/worker/commandCodec.test.ts +++ b/packages/client/src/worker/commandCodec.test.ts @@ -100,13 +100,24 @@ 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 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 +128,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..819433f88e 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,45 @@ function bytes(value: unknown, field: string): Uint8Array { return value; } -function text(value: unknown, field: string): string { +/** + * 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; } -function opId(value: unknown, field: string): bigint { +/** + * 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; +} + +/** + * 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; } -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)); } @@ -79,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( @@ -99,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 9d3eeff0a6..94e47856fa 100644 --- a/packages/client/src/worker/engineHost.test.ts +++ b/packages/client/src/worker/engineHost.test.ts @@ -1,6 +1,8 @@ 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'; /** The arguments one `EngineHandle` construction crossed the WASM boundary with. */ interface Constructed { @@ -42,6 +44,45 @@ 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. + */ +function permissiveHost(): { host: EngineHost; calls: unknown[][] } { + const calls: unknown[][] = []; + const record = + (name: string, result: unknown = new Uint8Array(0)) => + (...args: unknown[]): Promise => { + calls.push([name, ...args]); + return Promise.resolve(result); + }; + const wasm = { + ...fakeWasmEnums, + EngineHandle: class { + start = record('start'); + pushChunk = record('pushChunk'); + beginWrite = record('beginWrite'); + snapshot = record('snapshot', emptyView); + 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 +171,123 @@ describe('EngineHost', () => { expect(plaintext).toEqual(new Uint8Array(4)); }); }); + +/** Untrusted request fields, refused rather than coerced (`invalidField`). */ +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.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(); + + 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(); + + 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' + ); + 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..d793ed25fd 100644 --- a/packages/client/src/worker/engineHost.ts +++ b/packages/client/src/worker/engineHost.ts @@ -14,7 +14,17 @@ 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 { + buffer, + buildCommand, + count, + minted, + nodeId, + readEvent, + readSnapshot, + record, + text, +} from './commandCodec.js'; /** * The engine-facing surface the protocol server ([`serveEngine`]) drives. The @@ -99,46 +109,49 @@ 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 { await this.handle.command(buildCommand(this.wasm, command)); } - beginWrite(target: WriteTarget, size: number): Promise { - if ('node' in target) { + 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 ); } - pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { - return this.scrubbing(chunk, (view) => this.handle.pushChunk(handle, view)); + async pushChunk(handle: WriteHandle, chunk: ArrayBuffer): Promise { + 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 { 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,19 +161,25 @@ 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( + 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 {