diff --git a/.changeset/sanitize-graph-failures.md b/.changeset/sanitize-graph-failures.md new file mode 100644 index 000000000..9150db28a --- /dev/null +++ b/.changeset/sanitize-graph-failures.md @@ -0,0 +1,20 @@ +--- +"@solidjs/web": patch +--- + +Sanitize a failure that escapes through a server function's result graph. +`sanitizeServerError` guarded the one road a thrown error takes out of +dispatch; a rejected promise, an async iterable that throws, or a stream +that errors reaches the codec as a value to encode instead, and shipped +its `message` and every own-property to the client verbatim — a driver +error's failing query, connection string and bound params included — +under a 200 carrying no error tag, because the head was already +committed. Those channels are now wrapped before either serializer sees +them: the response encoder and the frames flight sink, which encodes its +outcome with a serializer of its own. + +The walk covers plain objects, arrays, `Map` and `Set`. A channel held by +a class instance or behind an accessor is left alone — rebuilding one and +invoking the other are not the runtime's to do. `markSafeError` remains +the escape hatch, an `Error` that is a returned value is untouched, and +the wire format is unchanged, cycles and shared references included. diff --git a/packages/web/frames/src/frame-sink.ts b/packages/web/frames/src/frame-sink.ts index 604ed2087..8166f4056 100644 --- a/packages/web/frames/src/frame-sink.ts +++ b/packages/web/frames/src/frame-sink.ts @@ -145,7 +145,10 @@ import { frameAddress, serializeStream } from "../../server-functions/src/shared.js"; -import { getEventServerFunctionInvocation } from "../../server-functions/src/server.js"; +import { + getEventServerFunctionInvocation, + guardFailures +} from "../../server-functions/src/server.js"; import { isResponseEnvelope } from "../../src/response.js"; import { FRAME_STREAM_HEADER, @@ -2023,7 +2026,13 @@ export function frameFlightResponse({ primary, regions = [], outcome, codec }, i // Component-valued entries serialize as flight references — the // protocol injects its own plugin (see `flightCodec`), so nothing // registers it. - const reader = new ChunkReader(serializeStream(outcome, flightCodec(codec))); + // Guarded like every other server-function body: this sink has its + // own serializer, so a rejection nested in flight data would + // otherwise reach the wire with its message and own-properties + // intact — under a 200, since the head is long committed. + const reader = new ChunkReader( + serializeStream(guardFailures(outcome), flightCodec(codec)) + ); for (let node = await reader.next(); !node.done; node = await reader.next()) { write({ type: "outcome", payload: node.value }); } diff --git a/packages/web/server-functions/src/server.ts b/packages/web/server-functions/src/server.ts index b7f00dde4..7cf523dcf 100644 --- a/packages/web/server-functions/src/server.ts +++ b/packages/web/server-functions/src/server.ts @@ -1351,6 +1351,168 @@ function isFormPost(request) { ); } +// `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. Worse than the thrown +// case, because the head is already committed — the answer is a 200 +// carrying no error tag. +// +// So the CHANNELS are wrapped before the codec sees them. Not the rejection +// (it 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. +// +// Each container is recorded BEFORE its children are walked, so a cycle — +// which seroval encodes natively as a back-reference — resolves to the +// container being built instead of recursing forever. A cycle also forces +// the rebuild to stand, since a descendant already holds it. Containers +// that changed nothing are passed through by reference so identity survives +// for the codec, though the rebuilt shell is allocated either way: it has to +// exist before the walk that decides whether it was needed. +// +// Left alone deliberately, so a channel behind either is unguarded: class +// instances, whose own properties are not ours to rebuild (private fields, +// getters, invariants), and accessors, which are not ours to invoke. +/** @internal */ +export function guardFailures(value, state) { + if (value === null || typeof value !== "object") return value; + if (!state) state = { seen: new WeakMap(), cyclic: new WeakSet() }; + if (state.seen.has(value)) { + state.cyclic.add(value); + return state.seen.get(value); + } + + // Ahead of the async-iterable branch: a ReadableStream is async-iterable + // on every server runtime, so that branch would claim it and this one + // would never run. + if (typeof ReadableStream !== "undefined" && value instanceof ReadableStream) { + let reader; + const guardedStream = new ReadableStream({ + async pull(controller) { + try { + if (!reader) reader = value.getReader(); + const { done, value: chunk } = await reader.read(); + // the chunk is walked like a step value: a channel nested in one + // would otherwise reach the codec unguarded + done ? controller.close() : controller.enqueue(guardFailures(chunk, state)); + } catch (error) { + controller.error(sanitizeServerError(error)); + } + }, + cancel(reason) { + return reader ? reader.cancel(reason) : value.cancel(reason); + } + }); + state.seen.set(value, guardedStream); + return guardedStream; + } + + if (typeof value.then === "function") { + const guardedPromise = Promise.resolve(value).then( + resolved => guardFailures(resolved, state), + error => { + throw sanitizeServerError(error); + } + ); + state.seen.set(value, guardedPromise); + return guardedPromise; + } + + if (typeof value[Symbol.asyncIterator] === "function") { + const source = value; + const guardedIterable = { + [Symbol.asyncIterator]() { + const iterator = source[Symbol.asyncIterator](); + return { + next: () => + iterator.next().then( + step => + step.done ? step : { done: false, value: guardFailures(step.value, state) }, + error => { + throw sanitizeServerError(error); + } + ), + return: iterator.return && (() => iterator.return()) + }; + } + }; + state.seen.set(value, guardedIterable); + return guardedIterable; + } + + if (Array.isArray(value)) { + const next = value.slice(); + state.seen.set(value, next); + let changed = false; + for (let index = 0; index < value.length; index++) { + const guarded = guardFailures(value[index], state); + if (guarded === value[index]) continue; + next[index] = guarded; + changed = true; + } + return keepGuarded(value, next, changed, state); + } + + if (value instanceof Map) { + const next = new Map(); + state.seen.set(value, next); + let changed = false; + for (const [key, entry] of value) { + const guarded = guardFailures(entry, state); + if (guarded !== entry) changed = true; + next.set(key, guarded); + } + return keepGuarded(value, next, changed, state); + } + + if (value instanceof Set) { + const next = new Set(); + state.seen.set(value, next); + let changed = false; + for (const entry of value) { + const guarded = guardFailures(entry, state); + if (guarded !== entry) changed = true; + next.add(guarded); + } + return keepGuarded(value, next, changed, state); + } + + const prototype = Object.getPrototypeOf(value); + if (prototype !== Object.prototype && prototype !== null) { + state.seen.set(value, value); + return value; + } + + // Data properties only: reading through a getter would invoke it here as + // well as when the codec encodes, and a throwing one would escape into + // dispatch's catch to be reported as the function itself failing — the + // phantom error over a call that succeeded. Descriptors carry across, so + // a frozen or non-writable shape survives the rebuild. + const descriptors = Object.getOwnPropertyDescriptors(value); + const next = Object.create(prototype, descriptors); + state.seen.set(value, next); + let changed = false; + for (const key of Object.keys(descriptors)) { + const descriptor = descriptors[key]; + if (!("value" in descriptor)) continue; + const guarded = guardFailures(descriptor.value, state); + if (guarded === descriptor.value) continue; + Object.defineProperty(next, key, { ...descriptor, value: guarded }); + changed = true; + } + return keepGuarded(value, next, changed, state); +} + +/** A rebuild stands if anything below changed, or if a cycle already took it. */ +function keepGuarded(value, next, changed, state) { + if (changed || state.cyclic.has(value)) return next; + state.seen.set(value, value); + return value; +} + /** * The response-side codec stream: `serializeStream` (shared.js) hardened * with request-lifetime teardown. Server-only on purpose — the shared half @@ -1367,6 +1529,7 @@ function isFormPost(request) { * the codec directly and stay untouched. */ export function serializeResponseStream(value, codecOptions, signal) { + value = guardFailures(value); let closeIterator = null; let closed = false; let cancelSerialize = null; diff --git a/packages/web/test/server/server-functions-failure-sanitization.spec.tsx b/packages/web/test/server/server-functions-failure-sanitization.spec.tsx new file mode 100644 index 000000000..3ccba6045 --- /dev/null +++ b/packages/web/test/server/server-functions-failure-sanitization.spec.tsx @@ -0,0 +1,280 @@ +/** + * `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. + * Same failure, different road, and 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. + * + * Every case here round-trips through the client as well as reading the + * wire. Asserting only that the body lacks the secret passes just as + * happily when the body is empty or undecodable, which is how a broken + * first attempt at this fix went green. + * + * Like the other server-function specs, these run against the built + * bundles (server-functions/dist/*, wired up in vite.config.server.mjs), + * which are the production variant — `DEV` is false, so the sanitizer is + * live. + */ +import { AsyncLocalStorage } from "node:async_hooks"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { markSafeError } from "@solidjs/web"; +import { + handleServerFunctionRequest, + registerServerFunction +} from "@solidjs/web/server-functions/server"; +import { createServerReference } from "@solidjs/web/server-functions/client"; +import { frameTransformFlightResult } from "@solidjs/web/frames/server"; + +const RequestContext = Symbol.for("solid.RequestContext"); + +beforeAll(() => { + (globalThis as any)[RequestContext] = new AsyncLocalStorage(); +}); + +afterAll(() => { + delete (globalThis as any)[RequestContext]; +}); + +/** A driver error as one actually arrives: secrets in message and own-props. */ +function databaseError() { + return Object.assign(new Error("connect ECONNREFUSED postgres://app:hunter2@10.0.0.5:5432"), { + connectionString: "postgres://app:hunter2@10.0.0.5:5432", + query: "SELECT * FROM users WHERE token = 'abc123'" + }); +} + +const SECRETS = ["hunter2", "10.0.0.5", "SELECT * FROM users", "abc123"]; + +/** Routes the client stub's fetch straight into the handler. */ +function connectTransport() { + const original = globalThis.fetch; + globalThis.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const address = input instanceof Request ? input.url : input.toString(); + const request = new Request( + new URL(address, "http://localhost"), + input instanceof Request ? input : init + ); + request.headers.set("Sec-Fetch-Site", "same-origin"); + return handleServerFunctionRequest(request); + }) as typeof fetch; + return () => { + globalThis.fetch = original; + }; +} + +async function wireBody(id: string) { + const response = await handleServerFunctionRequest( + new Request(`https://app.example/_server/data/${id}`, { + method: "POST", + body: "[]", + headers: { + "Sec-Fetch-Site": "same-origin", + "X-Server-Function-Instance": "server-function:test" + } + }) + ); + return response.text(); +} + +async function callThrough(id: string) { + const restore = connectTransport(); + try { + return { resolved: true as const, value: await createServerReference(id)() }; + } catch (error) { + return { resolved: false as const, error }; + } finally { + restore(); + } +} + +describe("a failure escaping through the result graph", () => { + it("is sanitized when an async iterable throws", async () => { + registerServerFunction("graph-failure-iterable", async function* () { + yield { page: 1 }; + throw databaseError(); + }); + + const body = await wireBody("graph-failure-iterable"); + for (const secret of SECRETS) expect(body).not.toContain(secret); + expect(body).toContain("Internal Server Error"); + }); + + it("is sanitized when a promise in the graph rejects", async () => { + registerServerFunction("graph-failure-promise", async () => ({ + deferred: Promise.reject(databaseError()) + })); + + const body = await wireBody("graph-failure-promise"); + for (const secret of SECRETS) expect(body).not.toContain(secret); + expect(body).toContain("Internal Server Error"); + }); + + it("is sanitized when a stream in the graph errors", async () => { + registerServerFunction("graph-failure-stream", async () => ({ + rows: new ReadableStream({ + start(controller) { + controller.enqueue("first"); + queueMicrotask(() => controller.error(databaseError())); + } + }) + })); + + const body = await wireBody("graph-failure-stream"); + for (const secret of SECRETS) expect(body).not.toContain(secret); + expect(body).toContain("Internal Server Error"); + }); + + it("is sanitized when the rejection sits in a Map", async () => { + registerServerFunction("graph-failure-map", async () => + new Map([["pending", Promise.reject(databaseError())]]) + ); + + const body = await wireBody("graph-failure-map"); + for (const secret of SECRETS) expect(body).not.toContain(secret); + expect(body).toContain("Internal Server Error"); + }); + + it("keeps an error the author branded as intentional", async () => { + registerServerFunction("graph-failure-safe", async function* () { + yield { page: 1 }; + throw markSafeError(new Error("Order 42 not found")); + }); + + expect(await wireBody("graph-failure-safe")).toContain("Order 42 not found"); + }); +}); + +describe("what the guard must not disturb", () => { + // The road that already worked. A first attempt at this fix replaced the + // encoded Error with a node the client could not read, breaking every + // sanitized error rather than only the unsanitized ones — and no test + // noticed, because nothing decoded a sanitized error through the client. + it("still delivers an ordinary thrown error as a readable Error", async () => { + registerServerFunction("graph-failure-thrown", async () => { + throw databaseError(); + }); + + const outcome = await callThrough("graph-failure-thrown"); + expect(outcome.resolved).toBe(false); + expect((outcome as { error: any }).error).toBeInstanceOf(Error); + expect((outcome as { error: Error }).error.message).toBe("Internal Server Error"); + }); + + // An Error reached as a VALUE was never thrown, so it is data and the + // author's to ship. The guard wraps failure channels, not errors. + it("leaves an error that is a value alone", async () => { + registerServerFunction("graph-value-error", async () => ({ + ok: 1, + failure: new Error("returned, not thrown") + })); + + const outcome = await callThrough("graph-value-error"); + expect(outcome.resolved).toBe(true); + expect((outcome as { value: any }).value.failure.message).toBe("returned, not thrown"); + }); + + // The walk reads data properties only. Reading through a getter would + // invoke it here as well as when the codec encodes, and a throwing one + // would escape 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. + it("does not invoke an accessor while walking", async () => { + registerServerFunction("graph-accessor", async () => ({ + ok: 1, + get lazy() { + throw new Error("an accessor the runtime must not call"); + } + })); + + const response = await handleServerFunctionRequest( + new Request("https://app.example/_server/data/graph-accessor", { + method: "POST", + body: "[]", + headers: { + "Sec-Fetch-Site": "same-origin", + "X-Server-Function-Instance": "server-function:test" + } + }) + ); + + expect(response.status).toBe(200); + }); + + // The walk records each container before descending, so a self-reference + // resolves to the container being built. Getting this wrong recursed + // until the stack gave out, and the RangeError escaped into dispatch's + // catch as a 500 — on a shape the codec encodes natively as a back + // reference. + it("does not choke on a cycle", async () => { + registerServerFunction("graph-cycle", async () => { + const node: any = { name: "n" }; + node.self = node; + return node; + }); + + const outcome = await callThrough("graph-cycle"); + expect(outcome.resolved).toBe(true); + const value = (outcome as { value: any }).value; + expect(value.self).toBe(value); + }); + + it("keeps a null-prototype object null-prototyped", async () => { + registerServerFunction("graph-null-proto", async () => { + const bare = Object.create(null); + bare.deferred = Promise.reject(databaseError()); + return bare; + }); + + const body = await wireBody("graph-null-proto"); + for (const secret of SECRETS) expect(body).not.toContain(secret); + // NullConstructor rather than a plain Object: rebuilding must not + // hand the value a prototype it did not have. + expect(body).toContain('"t":11'); + }); + + it("passes a healthy result through untouched, nested promise included", async () => { + registerServerFunction("graph-healthy", async () => ({ + items: [1, 2], + nested: { deferred: Promise.resolve("value") } + })); + + const outcome = await callThrough("graph-healthy"); + expect(outcome.resolved).toBe(true); + const value = (outcome as { value: any }).value; + expect(value.items).toEqual([1, 2]); + await expect(value.nested.deferred).resolves.toBe("value"); + }); +}); + +describe("the frames flight sink", () => { + // It encodes its outcome with its own serializer, so the guard has to be + // applied there too — the same rejection that is sanitized on the plain + // response path reached the wire intact through this one. + it("sanitizes a failure nested in flight data", async () => { + // a markup-valued result is what routes the response through frames + registerServerFunction("flight-markup", async () => () => "ok"); + + const response = await handleServerFunctionRequest( + new Request("https://app.example/_server/data/flight-markup", { + method: "POST", + body: "[]", + headers: { + "Sec-Fetch-Site": "same-origin", + "X-Server-Function-Instance": "server-function:test", + "X-Single-Flight": "true" + } + }), + { + collectFlightData: () => ({ "/notes": { pending: Promise.reject(databaseError()) } }), + transformFlightResult: frameTransformFlightResult + } + ); + + const body = await response.text(); + for (const secret of SECRETS) expect(body).not.toContain(secret); + expect(body).toContain("Internal Server Error"); + }); +}); diff --git a/packages/web/vite.config.server.mjs b/packages/web/vite.config.server.mjs index c96078b27..2e640d8ce 100644 --- a/packages/web/vite.config.server.mjs +++ b/packages/web/vite.config.server.mjs @@ -23,6 +23,7 @@ export default defineConfig({ alias: { "@solidjs/web/server-functions/server": resolve(rootDir, "server-functions/dist/server.js"), "@solidjs/web/server-functions/client": resolve(rootDir, "server-functions/dist/client.js"), + "@solidjs/web/frames/server": resolve(rootDir, "frames/dist/server.js"), // the transport's lazy codec imports — without these the bare // "@solidjs/web" alias below swallows the subpath "@solidjs/web/serialization/decode": resolve(rootDir, "serialization/dist/decode.js"),