diff --git a/.changeset/transport-failure-on-foreign-response.md b/.changeset/transport-failure-on-foreign-response.md new file mode 100644 index 000000000..0021ac773 --- /dev/null +++ b/.changeset/transport-failure-on-foreign-response.md @@ -0,0 +1,11 @@ +--- +"@solidjs/web": patch +--- + +Fail a server function call on a response the runtime did not write, instead of resolving it to `undefined` (#3087). + +Only the protocol's error header and a 5xx counted as failure, so every other non-2xx was decoded as a result — and decoding a login page, or an empty 405, yields nothing. A response at 400 or above carrying no body format now fails the call with the status on the error, undecoded, and before the passthrough control flow uses: a refusal can carry a `Location` of its own, and the passthrough would have handed it back as control flow. Redirects are left alone — `fetch` follows them, so an interstitial arrives as its page at 200, and a 3xx only reaches the transport where something opted out of following one. + +`BodyFormat.Void` marks the one response the runtime encodes without a format to carry — a function that returned nothing — so `respond(undefined, { status: 400 })` stays a result alongside `new Response(null, { status: 404 })` and `respond(value, { status: 400 })`. A client that predates the tag decodes it the same way; a client that has it, talking to a server that does not, reads an untagged void 4xx as a refusal. + +A 2xx is not judged at all: a login page served at 200 is indistinguishable from a void result by header alone. One runtime-produced shape is caught with the foreign ones — a verbatim `X-Content-Raw` response at a non-2xx status, which an integration's `responseHandler` claims before the check. diff --git a/packages/web/server-functions/src/client.ts b/packages/web/server-functions/src/client.ts index 67083a75c..7dc723f90 100644 --- a/packages/web/server-functions/src/client.ts +++ b/packages/web/server-functions/src/client.ts @@ -584,6 +584,16 @@ async function fetchServerFunction(base, id, options, args, meta, callArgs = arg if (handled !== undefined) return handled; } + // Every response the runtime encodes carries the body format — a void one + // and a thrown one included — so at 400 and up its absence means the peer + // refused. Answered before the passthrough beneath, because a refusal can + // carry a `Location` of its own and the passthrough would hand it back as + // control flow; and undecoded, because its body is someone else's, not a + // payload for the caller. + if (response.status >= 400 && !response.headers.has(BODY_FORMAT_HEADER)) { + throw serverFunctionFailure(response, undefined); + } + // Proxies may omit the protocol error header on 5xx responses. const failed = response.headers.has(ERROR_HEADER) || response.status >= 500; diff --git a/packages/web/server-functions/src/server.ts b/packages/web/server-functions/src/server.ts index 986e9d6d1..f44acae71 100644 --- a/packages/web/server-functions/src/server.ts +++ b/packages/web/server-functions/src/server.ts @@ -1374,6 +1374,7 @@ function encodeResult(value, headers, status, codec, signal) { // client load its decode half (see shared.js loadSerializer). Negotiated // per response: mixed pages simply carry both formats. if (value === undefined) { + headers.set(BODY_FORMAT_HEADER, BodyFormat.Void); return new Response(null, { status, headers }); } // By the time a result is being encoded the function has already run — diff --git a/packages/web/server-functions/src/shared.ts b/packages/web/server-functions/src/shared.ts index 30536f810..54e530494 100644 --- a/packages/web/server-functions/src/shared.ts +++ b/packages/web/server-functions/src/shared.ts @@ -539,7 +539,14 @@ export const BodyFormat = { * legs: argument lists on the request, results (single-flight envelopes * included) on the response. */ - Json: "8" + Json: "8", + /** + * No body at all — a function that returned nothing. It marks the response + * as one the runtime encoded, which separates a void result with a status + * on it from a refusal answered by something else. Decoding falls through + * to `undefined`, which is what a peer predating the tag reads too. + */ + Void: "9" }; // Nesting deeper than this is not JSON-safe. The guard itself walks an diff --git a/packages/web/test/server/server-functions-transport-failure.spec.tsx b/packages/web/test/server/server-functions-transport-failure.spec.tsx new file mode 100644 index 000000000..c4a33554e --- /dev/null +++ b/packages/web/test/server/server-functions-transport-failure.spec.tsx @@ -0,0 +1,288 @@ +/** + * What the transport does with a response the runtime did not write (#3087). + * + * Its own responses are recognisable: they carry the error header, or the + * body format every encoding path stamps — a void result included. Anything + * else at 400 and up is the peer refusing, and decoding one yields nothing, + * which used to resolve the call to `undefined`. These pin the refusals that + * now fail, the value-shaped statuses that must not, and the limit of what a + * status can tell you. + * + * Like the extension specs, these run against the built bundles + * (server-functions/dist/*, wired up in vite.config.server.mjs). + */ +import { AsyncLocalStorage } from "node:async_hooks"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { redirect, respond } from "@solidjs/web"; +import { + handleServerFunctionRequest, + registerServerFunction +} from "@solidjs/web/server-functions/server"; +import { + configureServerFunctionsClient, + createServerReference +} from "@solidjs/web/server-functions/client"; + +const RequestContext = Symbol.for("solid.RequestContext"); + +beforeAll(() => { + (globalThis as any)[RequestContext] = new AsyncLocalStorage(); +}); + +afterAll(() => { + delete (globalThis as any)[RequestContext]; +}); + +/** + * The transport's fetch. `answer` replaces the handler with a response from + * somewhere else; `rewrite` sends the call to a different address. + */ +function connectTransport({ + answer, + rewrite, + method, + site = "same-origin" +}: { + answer?: () => Response; + rewrite?: (address: string) => string; + method?: string; + site?: string; +} = {}) { + const original = globalThis.fetch; + globalThis.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + if (answer) return Promise.resolve(answer()); + const address = input instanceof Request ? input.url : input.toString(); + const request = new Request(new URL(rewrite ? rewrite(address) : address, "http://localhost"), { + ...(input instanceof Request ? input : init), + ...(method ? { method, body: undefined } : {}) + }); + request.headers.set("Sec-Fetch-Site", site); + return handleServerFunctionRequest(request); + }) as typeof fetch; + return () => { + globalThis.fetch = original; + }; +} + +const foreign = (status: number, body: BodyInit | null, type?: string) => () => + new Response(body, { status, headers: type ? { "content-type": type } : undefined }); + +describe("server-function transport failures (#3087)", () => { + it("fails the call when the handler refuses it", async () => { + const restore = connectTransport(); + try { + // a client that outlived the build registering its function + await expect(createServerReference("fail-never-registered")()).rejects.toMatchObject({ + status: 404 + }); + } finally { + restore(); + } + }); + + it("fails the call when the handler rejects the request itself", async () => { + registerServerFunction("fail-args", async () => "ok"); + // something under `args` that is not an argument array: a 400 for every + // caller of that url + const restore = connectTransport({ rewrite: address => `${address}?args=nope` }); + try { + await expect(createServerReference("fail-args")()).rejects.toMatchObject({ status: 400 }); + } finally { + restore(); + } + }); + + it("fails the call on a response nothing in the runtime wrote", async () => { + registerServerFunction("fail-foreign", async () => "ok"); + for (const answer of [ + foreign(404, "