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, "

Not found

", "text/html"), + foreign(401, "login", "text/html"), + foreign(403, null) + ]) { + const restore = connectTransport({ answer }); + try { + await expect(createServerReference("fail-foreign")()).rejects.toMatchObject({ + status: answer().status + }); + } finally { + restore(); + } + } + }); + + it("fails one carrying integration metadata of its own", async () => { + registerServerFunction("fail-interstitial", async () => "ok"); + // an SSO interstitial answers 403 with a Location, and a gateway can + // answer 404 with anything — neither is the runtime's control flow, so + // neither may take the passthrough that control flow uses + for (const [status, headers] of [ + [403, { "content-type": "text/html", Location: "https://sso.example/login" }], + [404, { "X-Revalidate": "stories" }] + ] as const) { + const restore = connectTransport({ answer: () => new Response(null, { status, headers }) }); + try { + await expect(createServerReference("fail-interstitial")()).rejects.toMatchObject({ + status + }); + } finally { + restore(); + } + } + }); + + it("fails with an Error even when the foreign body sniffs as a known encoding", async () => { + registerServerFunction("fail-sniffed", async () => "ok"); + // content-type sniffing would decode this as URLSearchParams; a refusal's + // body is not a payload, and a caller reads `.status`, not a form + const restore = connectTransport({ + answer: foreign(403, "reason=blocked", "application/x-www-form-urlencoded") + }); + try { + await expect(createServerReference("fail-sniffed")()).rejects.toMatchObject({ status: 403 }); + await expect(createServerReference("fail-sniffed")()).rejects.toBeInstanceOf(Error); + } finally { + restore(); + } + }); + + it("resolves a value-shaped status the function itself produced", async () => { + registerServerFunction("fail-validated", async () => + respond({ field: "required" }, { status: 400 }) + ); + registerServerFunction("fail-void", async () => respond(undefined, { status: 400 })); + registerServerFunction("fail-empty", async () => new Response(null, { status: 404 })); + const restore = connectTransport(); + try { + expect(await createServerReference("fail-validated")()).toEqual({ field: "required" }); + // nothing, with a status on it — both spellings answer the same way + expect(await createServerReference("fail-void")()).toBeUndefined(); + expect(await createServerReference("fail-empty")()).toBeNull(); + } finally { + restore(); + } + }); + + it("leaves the runtime's own control flow on its passthrough", async () => { + registerServerFunction("fail-redirect", async () => { + throw redirect("/login"); + }); + const restore = connectTransport(); + try { + const response = await createServerReference("fail-redirect")(); + expect(response).toBeInstanceOf(Response); + expect((response as Response).headers.get("Location")).toBe("/login"); + } finally { + restore(); + } + }); + + it("leaves a redirect the peer answered with to the passthrough", async () => { + registerServerFunction("fail-3xx", async () => "ok"); + // `fetch` follows redirects, so an interstitial arrives as its page at + // 200 and a 3xx only reaches the transport where something opted out of + // following one — control flow the runtime does not produce and the + // caller may still want to read + const restore = connectTransport({ + answer: () => new Response(null, { status: 302, headers: { Location: "/login" } }) + }); + try { + const response = await createServerReference("fail-3xx")(); + expect((response as Response).status).toBe(302); + } finally { + restore(); + } + }); + + it("cannot judge a 2xx, and does not try", async () => { + registerServerFunction("fail-spa", async () => "ok"); + // a login page or an SPA index served at 200 is indistinguishable from a + // void result by header alone; the status is all this rule reads + const restore = connectTransport({ answer: foreign(200, "login", "text/html") }); + try { + expect(await createServerReference("fail-spa")()).toBeUndefined(); + } finally { + restore(); + } + }); + + it("fails on the gates that answer without a body", async () => { + registerServerFunction("fail-gated", async () => "ok"); + // the method allowlist and the origin gate answer 405 and 403 with + // nothing in the body, which is the shape that used to decode to + // `undefined` + const method = connectTransport({ method: "GET" }); + try { + await expect(createServerReference("fail-gated")()).rejects.toMatchObject({ status: 405 }); + } finally { + method(); + } + const origin = connectTransport({ site: "cross-site" }); + try { + await expect(createServerReference("fail-gated")()).rejects.toMatchObject({ status: 403 }); + } finally { + origin(); + } + }); + + it("reads a void result the way a peer without the tag wrote it", async () => { + registerServerFunction("fail-untagged", async () => "ok"); + // a server built before `BodyFormat.Void`: a void result with a status + // and no format header is indistinguishable from a refusal, and fails + const restore = connectTransport({ answer: () => new Response(null, { status: 400 }) }); + try { + await expect(createServerReference("fail-untagged")()).rejects.toMatchObject({ status: 400 }); + } finally { + restore(); + } + }); + + it("fails a verbatim passthrough that carries a refusal's status", async () => { + registerServerFunction( + "fail-raw", + async () => new Response("nope", { status: 404, headers: { "X-Content-Raw": "1" } }) + ); + registerServerFunction( + "fail-raw-ok", + async () => new Response("here", { status: 200, headers: { "X-Content-Raw": "1" } }) + ); + const restore = connectTransport(); + try { + // a raw body is for whoever claims the response; a call site with no + // integration could not read it at either status, and a status beats + // minting `undefined` + await expect(createServerReference("fail-raw")()).rejects.toMatchObject({ status: 404 }); + expect(await createServerReference("fail-raw-ok")()).toBeUndefined(); + } finally { + restore(); + } + }); + + it("leaves the response to an integration that claims it", async () => { + registerServerFunction( + "fail-claimed", + async () => new Response("frame", { status: 404, headers: { "X-Content-Raw": "1" } }) + ); + const restore = connectTransport(); + configureServerFunctionsClient({ + responseHandler: { handle: (response: Response) => response.status } + }); + try { + // the seam runs before the check, which is what keeps server + // components free to answer with a status of their own + expect(await createServerReference("fail-claimed")()).toBe(404); + } finally { + configureServerFunctionsClient({ responseHandler: null as any }); + restore(); + } + }); + + it("leaves an ordinary call alone", async () => { + registerServerFunction("fail-plain", async (word: string) => word.toUpperCase()); + const restore = connectTransport(); + try { + expect(await createServerReference("fail-plain")("solid")).toBe("SOLID"); + } finally { + restore(); + } + }); +});