Improve TS stream primordial safety by jasnell · Pull Request #7409 · cloudflare/workerd

jasnell · 2026-09-17T13:44:12Z

There were still a handful of gaps in the primordial/prototype-polution protection in the TS streams implementation. Fill gaps and expand coverage.

Body consumption sized its result buffer and limit message with the bare Number global, and FixedLengthStream derived its high-water-mark cap the same way; a replaced globalThis.Number changed the outcome. Number joins the captured constructors, and the three call sites use the capture.

Body collection and concatenation walked their chunk arrays with for...of, and the native tee destructured the pair its hook returns; both run the patchable %ArrayIteratorPrototype%.next, so a patched iterator emptied Response.text() and arrayBuffer() and stalled native tee branches. Index loops and indexed access replace them.

The queued byte cursor copied chunks for tee branches and released or remainder bytes with ArrayBuffer.prototype.slice and %TypedArray%.prototype.slice. Both create the copy through the species constructor, so a patched ArrayBuffer[Symbol.species] or %TypedArray%[Symbol.species] received every internal copy. A cloneArrayBuffer helper allocates a fresh %ArrayBuffer% and copies into it, which is what the spec's CloneArrayBuffer does.

A reader's closed promise and a writer's ready and closed promises hold either a pending resolvers record or, once settled, the promise itself. The two were told apart by `typeof x.resolve === 'function'`, which on a promise reads Object.prototype: with Object.prototype.resolve and .promise polluted, the settled branch called the polluted function and stored its `promise` member, so writer.ready stopped being a promise. The native utils.isPromise brand check discriminates instead; its type declaration gains the type argument that lets it narrow.

@asynciterator

Several internal paths called public prototype methods on internal objects: the queued controllers errored themselves through this.error(), readAtLeast() went through the BYOB reader's read(), @@asynciterator called this.values(), the Node interop hooks called controller.error(), and the encoding transformers enqueued through the transform controller's prototype. A user patching any of those methods redirected the internal call: a spied ReadableStreamDefaultController.prototype.error kept a stream from ever erroring, a patched enqueue rewrote TextDecoderStream output. Each controller now errors through a private #error behind the public method, reached across classes by a controllerError slot that spans the default, byte and native controllers (nativeControllerError joins the native internals); the BYOB reader's body moves to a private #read that read() and readAtLeast() call without an async wrapper, which would have cost two microtasks per result; the writable hook uses a controllerErrorIfNeeded slot; encoding.ts captures the transform controller's enqueue at load time as transform.ts already does. ReadableStream.prototype[Symbol.asyncIterator] is now the values() function object and not enumerable, as WebIDL specifies; it was a separate enumerable method. The C++ implementation installs a separate, non-enumerable function.

The stream constructors read their dictionaries with ordinary property access, as WebIDL's dictionary conversion does, so the reads walk the prototype chain. That is the specified behavior for a user's object, but it also reached Object.prototype from the implementation's own objects: the `= {}` defaults standing in for omitted arguments (WebIDL reads nothing for those), the source, sink, strategy and transformer literals the internal pairs build, and the C++ native source, whose members live on its prototype and which never declares type or autoAllocateChunkSize. With Object.prototype.type set to 'bytes', every Response body failed to construct and so did every IdentityTransformStream, CompressionStream and TextEncoderStream; a polluted start, size, highWaterMark or expectedLength changed construction or the bytes accounted for. Omitted arguments now default to a frozen null-prototype kEmptyDictionary (kEmptyStrategy in identity.ts), the internal literals carry `__proto__: null`, and the native path checks type and autoAllocateChunkSize with declaresMember, which looks at the source and its own prototype only.

Each affected suite gains a pollution.js module that patches what the implementation must not observe — the array iterator, the Number global, controller and reader prototype methods, the ArrayBuffer and %TypedArray% species, Object.prototype resolvers, and Object.prototype members standing in for omitted or internal dictionaries — and asserts that bodies, reads, round trips and desired sizes are unchanged. Every mutation is undone before the assertions run. The tests pass on both implementations; the C++ one never read any of it. The per_isolate docs now state what the promise primordials do and do not protect: PromisePrototypeThen is the real Promise.prototype.then, so it still runs the species lookup, and the plain {value, done} read results the spec requires are open to Object.prototype.then; both are accepted exposures, since a species-free then needs a C++ call or extra microtasks per reaction and the same pollution breaks user code in every engine. The createReadResult and createDrainResult comments say the same instead of claiming immunity.

Co-authored-by: James M Snell <jasnell@gmail.com>

ask-bonk · 2026-09-17T13:52:37Z

LGTM

github run

jasnell added 7 commits September 17, 2026 13:42

jasnell requested review from guybedford and npaun September 17, 2026 13:44

jasnell requested review from a team as code owners September 17, 2026 13:44