A failure escaping through the result graph ships its message and own-properties to the client

[frenzzy]

Describe the bug

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's own doc comment describes:

A raw Error (or thrown string/object) serialized to the client would ship its message and every own-property verbatim over the wire — an ORM/driver error's failing query, connection string, or bound params included.

That is exactly what happens, and it is worse than the thrown case: the head is already committed when the failure arrives, so the answer is a 200 carrying no error tag.

upfront    500 | leaked: (nothing)
iterable   200 | leaked: hunter2, 10.0.0.5, SELECT * FROM users, abc123
promise    200 | leaked: hunter2, 10.0.0.5, SELECT * FROM users, abc123
stream     200 | leaked: hunter2, 10.0.0.5, SELECT * FROM users, abc123

There is a fourth road, found later and worth calling out separately because it survives a fix to the first three: the frames flight sink encodes its outcome with a serializer of its own (frames/src/frame-sink.ts, serializeStream rather than serializeResponseStream). A rejection nested inside a flight-data slice leaks there even when the plain response path is already sanitized, on the same build:

frames flight path   200 application/x-frame-stream | leaks: hunter2, SELECT * FROM users

It needs the response to route through frames — a mutation whose result is markup — and the failure to sit 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 this one looked covered.

Any streamed answer, deferred value or live source reaches these paths, so it is the ordinary shape of a failure rather than an exotic one. Anyone who can make a query fail — an invalid id is usually enough — reads the connection string.

.stack is stripped (that defence works); the message and every own-property are not.

Steps to reproduce

# Run against a build of the `next` BRANCH. The published `next` dist-tag is
# 2.0.0-rc.4, which predates the `<endpoint>/data/<id>` address (#3094) and
# answers 404 to every request below.
node repro.mjs

repro.mjs:

import { AsyncLocalStorage } from "node:async_hooks";
globalThis[Symbol.for("solid.RequestContext")] = new AsyncLocalStorage();
const srv = await import("@solidjs/web/server-functions/server");

// a driver error as one actually arrives: secrets in the message and own-props
const dbError = () =>
  Object.assign(new Error("connect ECONNREFUSED postgres://app:hunter2@10.0.0.5:5432/prod"), {
    connectionString: "postgres://app:hunter2@10.0.0.5:5432/prod",
    query: "SELECT * FROM users WHERE token = 'abc123'"
  });

srv.registerServerFunction("upfront", async () => { throw dbError(); });
srv.registerServerFunction("iterable", async function* () { yield { page: 1 }; throw dbError(); });
srv.registerServerFunction("promise", async () => ({ deferred: Promise.reject(dbError()) }));
srv.registerServerFunction("stream", async () => ({
  rows: new ReadableStream({
    start(controller) {
      controller.enqueue("first");
      queueMicrotask(() => controller.error(dbError()));
    }
  })
}));

for (const id of ["upfront", "iterable", "promise", "stream"]) {
  const response = await srv.handleServerFunctionRequest(
    new Request(`http://localhost/_server/data/${id}`, {
      method: "POST",
      body: "[]",
      headers: { "Sec-Fetch-Site": "same-origin", "X-Server-Function-Instance": "i" }
    })
  );
  let body = "";
  try { body = await response.text(); } catch { body = "[stream errored]"; }
  const leaked = ["hunter2", "10.0.0.5", "SELECT * FROM users", "abc123"].filter(s => body.includes(s));
  console.log(id.padEnd(10), response.status, "| leaked:", leaked.join(", ") || "(nothing)");
}

Output on next (e2b21041) — the table above. The production build is the default resolution, so this is what a deployment does.

Expected behavior

A failure is sanitized whichever road it takes out of the function. markSafeError stays the way an author says otherwise.

Fix

I have opened #3113 with a fix and tests: the channels are wrapped before either serializer sees them, so a failure arriving through one is sanitized like any other. An Error reached as a value is left alone — it was never thrown, so it is data — and the wire format is unchanged.

Filing it separately because the PR is a proposal about where the seam belongs, and the bug stands whatever you decide about that.

Options

1. Wrap the channels before either encoder — what fix(web): sanitize failures that escape through the result graph #3113 does. No protocol change, an Error that is a value stays untouched, and markSafeError keeps working unchanged. Costs a walk of the result and leaves two holes it declines to reach into: a channel held by a class instance, and one behind an accessor.
2. Sanitize in the serializer instead, where .stack stripping already lives (serializeOnlyDisabledFeatures) and is already global — "stripped from everything serialized outside development". One place, no walk, and it would cover SSR hydration payloads, which have the same exposure. The blast radius is the argument: it changes what an Error means on every wire, not just this one.
3. A codec plugin claiming Error. I tried this first and it does not work as it looks: a plugin wraps rather than replaces, so the wire carries a node the peer cannot read unless the plugin is in the shared set — and putting it there brings SSR along anyway, i.e. it collapses into (2) with extra steps.
4. Document it and leave authors to sanitize their own rejections. Consistent with the RFC's "the server side of a server function is your function body", and it asks every author to remember something the runtime already does for them on the thrown road.

I would take (1) and treat (2) as the question worth deciding deliberately rather than by default.

Related

#3117 is the same commitment point seen from the other side: there a failure after the head is committed is lost rather than delivered, and a fix for either should check it does not hand the other a new road.

[ryansolid]

Fixed by #3113 (with the review extension in 3d0bbb5 covering Map/Set channels, cyclic results, and the frames flight sink). Thanks for the exceptional report — the repro made the wire-level verification trivial.