fix(web): sanitize failures that escape through the result graph - #3113
Conversation
🦋 Changeset detectedLatest commit: 8f22636 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Moving this to draft — the fix as pushed is wrong, and in a way that matters. Review caught it; I have reproduced it. A plugin does not replace the value, it wraps it: the wire node is a seroval plugin node, not the plain
So the sentence in the commit message and changeset — "leaves the wire shape a plain Error node — the peer needs no matching plugin, and nothing about the protocol changes" — is false. My tests did not catch it because they were vacuous: the helper swallowed the decode failure and returned Reworking it. The premise I got wrong is worth stating, because it also invalidates the reasoning in the PR body: I claimed a pre-serialization walk "cannot work, because the rejection has not happened yet". Wrong — the walk does not need the rejection, only the channel, and a promise, async iterable or stream is present as an object before serialization. Wrapping it so its future rejection is sanitized is entirely possible, preserves the wire shape exactly, and leaves an |
|
Reviewed with repros against the branch, found three gaps, and pushed the fixes as a maintainer edit (3d0bbb5) since this closes an active leak — hope that's alright. Your two follow-up commits landed while I was mid-review, so the edit is rebased on top of them; the descriptor walk survives, with the copy pre-registered (see below).
Both containers are now rebuilt along channel paths, keyed on the exact constructor to match the codec's own dispatch (a subclass falls through to the passthrough, and the codec refuses it regardless). Cyclic results blew the stack. The comment said the WeakMap terminates cycles, but containers registered their copy only after walking children, so a cyclic result — which the codec happily encodes as a ref node — recursed forever, and dispatch reported the stack overflow as a sanitized 500. A working response shape on The non-async-iterable The flight-sink catch was a good one — that serializer split is exactly the kind of second road this whole PR is about. Suite is green locally across all three web configs; merging once CI agrees. |
sanitizeServerError guards the one road a thrown error takes out of dispatch. A failure can also escape through the RESULT GRAPH — a rejected promise, an async iterable that throws, a stream that errors — where it reaches the codec as a value to encode rather than as a throw, and never meets the sanitizer. Same failure, different road, and the leak is the exact one the sanitizer exists to stop: an ORM error's message and own-properties (failing query, connection string, bound params) riding the wire verbatim. Worse than the thrown case, because the head is already committed, so the answer is a 200 carrying no error tag. Claimed as a codec plugin rather than by walking the result: a walk would have to run before serialization, and a rejection has not happened yet at that point. The replacement is branded with markSafeError so the plugin does not claim it again, which also leaves the wire shape a plain Error node — the peer needs no matching plugin and the protocol is unchanged. The sanitizer composes ahead of an app's own plugins: a custom error type reaching the client is intent, and intent is spelled markSafeError. solidjs#3095's authoring error is branded for the same reason — it names the status the author got wrong, so it must stay readable.
sanitizeServerError guards the one road a thrown error takes out of dispatch. A failure can also escape through the RESULT GRAPH — a rejected promise, an async iterable that throws, a stream that errors — where it reaches the codec as a value to encode rather than as a throw, and never meets the sanitizer. The leak is the one the sanitizer exists to stop: a driver error's message and own-properties (failing query, connection string, bound params) riding the wire verbatim, under a 200 carrying no error tag because the head is already committed. The three channels are wrapped before the codec sees them. Not the rejection, which has not happened yet, and not the Errors already in the graph: an Error reached as a value was never thrown, so it is data and the author's to ship. Containers are rebuilt only along paths that actually contain a channel, so a healthy response allocates nothing and reference identity survives for the codec; a WeakMap keeps a repeated reference one object and terminates cycles. A first attempt claimed Error via a codec plugin. That was wrong twice over: a plugin WRAPS rather than replaces, so the wire carried a plugin node the peer had no plugin for, and sanitizeServerError's own unbranded replacement was claimed too — breaking every sanitized error, including the ordinary thrown one. Both are now regression tests. solidjs#3095's authoring error is branded markSafeError: it names the status the author got wrong, so it is intentional client-facing content.
Reading through a getter invoked it during the walk as well as when the codec encodes it, and a throwing one escaped into dispatch's catch to be reported as the function itself failing — the phantom error over a call that succeeded that encodeResult goes out of its way to avoid. Measured: a result with a throwing getter answered 500 with the guard, 200 without. Descriptors are carried across when a container is rebuilt, so a frozen or non-writable shape survives. A channel behind an accessor is left unguarded: invoking it is not ours to do.
It encodes its outcome with its own serializer (serializeStream, not serializeResponseStream), so the guard never reached it: a rejection nested inside a flight-data slice arrived with its message and own-properties intact, under a 200 with no error tag, on the same build where the plain response path was already sanitized. Reachable whenever the response routes through frames — a mutation whose result is markup — with the failure nested one level inside a slice, which is the ordinary shape of a cache entry. A rejection at the TOP of a slice already threw into dispatch's catch and was sanitized; that is why this looked covered. The spec needs the frames server entry, hence the alias alongside the other subpath aliases in vite.config.server.mjs.
Review found three defects in the first shape of the guard, all now tests: - A cycle recursed until the stack gave out, because the container was recorded in the WeakMap AFTER its children were walked. The RangeError then escaped into dispatch's catch as a 500 — on a shape seroval encodes natively as a back-reference. Containers are now recorded before descending, and a cycle forces the rebuild to stand since a descendant already holds it. - A rejection inside a Map or Set reached the wire raw: neither was walked, and the changeset promised more than the code delivered. - A null-prototype object on a channel path was rebuilt as a plain object, changing its node type from NullConstructor to Object. The prototype carries across now. Also: the ReadableStream branch was dead — a stream is async-iterable on every server runtime, so the iterator branch claimed it first — and it acquired the reader eagerly at walk time. It now runs ahead of the iterator branch and takes the reader on first pull. The markSafeError on solidjs#3095's authoring error is dropped: that error is encoded as a value and never routed through sanitizeServerError, so the brand did nothing. Verified by removing it and re-running the spec that asserts the message reaches the client.
3d0bbb5 to
8f22636
Compare
|
Apologies — I force-pushed over your maintainer edit. 3d0bbb5 is restored, authorship intact, and my duplicate of it is gone. What happened: I was mid-rework when your push landed, hit a rejected push, and checked whether the commit was upstream. It wasn't on The branch is now your edit on top of my four, which is what you pushed. I had independently written the same three fixes while yours was being erased; I have dropped mine — yours is the one that stands, and the constructor-keyed Verified on the restored branch against current One loose end from my side, take it or leave it: the |
2 participants
The bug
sanitizeServerErrorguards the one road a thrown error takes out of dispatch. A failure can also escape through the result graph — a rejected promise, an async iterable that throws, a stream that errors — where it reaches the codec as a value to encode rather than as a throw, and never meets the sanitizer.The leak is the one that sanitizer's own comment describes: "an ORM/driver error's failing query, connection string, or bound params included". Measured, the same driver error thrown four ways:
Worse than the thrown case: the head is already committed, so it is a
200carrying no error tag. Any streamed answer, deferred value or live source reaches it.Two encoders, not one. The frames flight sink encodes its outcome with
serializeStream, notserializeResponseStream, so a rejection nested inside a flight-data slice leaked there even after the response path was closed — same build, same process. It needs a response routed through frames (a mutation whose result is markup) and the failure nested one level inside a slice, which is the ordinary shape of a cache entry. A rejection at the top of a slice throws into dispatch's catch and is sanitized, which is why it looked covered.Filed as #3116.
The fix
Wrap the channels before either encoder sees them.
Not the rejection — it has not happened yet. Not the Errors already in the graph either: an
Errorreached as a value was never thrown, so it is data and the author's to ship. Only the shapes through which a future failure can arrive.Each container is recorded before its children are walked, so a cycle resolves to the container being built. Containers that changed nothing are passed through by reference, so identity and dedup survive for the codec. The walk covers plain objects, arrays,
MapandSet.Unchanged by design: the wire format (no new node types, nothing asked of the peer),
markSafeErroras the escape hatch, anErrorreturned as a value, cycles and shared references, and dev builds.Known limits, deliberate
Each is a place the walk declines to reach rather than an oversight, but they are holes, and worth your judgement: the alternative seam is one layer down in the serializer, where
.stackstripping already lives and is already global — "stripped from everything serialized outside development" — which would cover SSR payloads too, at a much larger blast radius. #3116 lays out both.What review found in this fix
Three rounds, and each defect is now a test. Recording them because the last one was a crash on a shape that works today:
WeakMapafter their children were walked, so a self-reference recursed until the stack gave out — and theRangeErrorescaped into dispatch's catch as a 500, on a shape seroval encodes natively as a back-reference.MapandSetwere not walked, so a rejection in either shipped raw while the changeset claimed otherwise.NullConstructortoObject— so "the wire format is unchanged" was not quite true.encodeResultgoes out of its way to avoid.ReadableStreambranch was dead, since a stream is async-iterable on every server runtime and the iterator branch claimed it first. It now runs ahead of that branch, takes the reader on first pull rather than eagerly, and guards chunk values.Errorvia a codec plugin. A plugin wraps rather than replaces, so the wire carried a node the peer had no plugin for, and every sanitized error broke — including the ordinary thrown one. That is why the spec now round-trips through the client instead of grepping the body.Tests
server-functions-failure-sanitization.spec.tsx— the channels, the escape hatch, and the shapes that must not change. Checked by mutation:markSafeErrorhatch ignoredMapnot walkedFull suite green on current
next: 44 files, 431 passed, 3 expected fail, 2 skipped.