A GET() declaration outlives the function it was made about #3129

Describe the bug

GET() records its declaration against a function id. registerServerFunction rebinds that id freely. Nothing connects the two, so a declaration made about one function can end up governing a different one — and it carries both of the things a declaration grants: GET dispatch, and the skip of the origin gate (#3114).

// packages/web/server-functions/src/server.ts:653-658
const REGISTRATIONS = new Map();
const METHODS = new Map();

// :690  — rebinds the id, silently, and never touches METHODS
export function registerServerFunction(id, callback) {
  provideRPC();
  REGISTRATIONS.set(id, callback);
  return callback;
}

// :917  — records the declaration against the id, and only the id
export function GET(fn) {
  ...
  METHODS.set(fn.id, "GET");

// :2219 — dispatch consults METHODS alone
const declaredRead =
  (method === "GET" || method === "HEAD") &&
  functionId !== null &&
  METHODS.get(functionId) === "GET";

declaredRead then feeds both gates: the method allowlist at :2277 and protectsRequest at :2227. So the function that now answers to the id inherits GET-reachability and the origin-gate exemption, whatever it does.

declared read, bare GET             -> 200 {"which":"READ"}
after re-registration, no headers   -> 200 {"which":"MUTATION","mutated":1}
after re-registration, cross-site   -> 200 {"which":"MUTATION","mutated":2}

v1 declares GET, bare GET           -> 200 {"v":1}
v2 does NOT declare GET, cross-site -> 200 {"v":2,"sideEffect":"sent mail"}

never declared GET (control)        -> 403
cross-site POST (control)           -> 403

mutations executed over GET: 2

The two controls are what make it a bug rather than a curiosity: a function that never declared GET is refused, and POST to the very same id is refused. The only thing separating them is a declaration made about a function that is no longer there.

Why it matters in production

Two ways to get there, and they are not equally likely — worth separating so the severity is not overstated.

1. An id collision. registerServerFunction is documented public API — "the low-level registry write for integrations registering functions outside the compiler (e.g. a router registering its own endpoints)" — and it takes an author-chosen id. Compiler ids are <name>-<xxhash32(root-relative path)>[-<ordinal>] (pinned by #3120 in packages/compiler/__tests__/directives-id-scheme.test.js), so a hand-picked id colliding with a compiled one takes an unlucky guess; two integrations both hand-registering, or an app re-using an id it saw in a manifest, is the realistic version. Nothing warns about it either way: the second write just wins.

2. Re-evaluating a module in a live process. This is the one that worries me more, because the id scheme makes it easy. The hash is of the path, not the contents — editing a file never changes its ids. So export const load = GET(...) in src/api.ts keeps the id load-<hash> across every edit, and if the author edits that file to drop the GET() wrapper (or to make load do something else), a re-evaluation re-registers the new function under the same id while METHODS keeps the old declaration. That is exactly scenario v1/v2 above, and the function stays GET-reachable and origin-gate-exempt for the rest of the process's life. The <ordinal> suffix gives the same shape without any edit to the declaration: two same-named functions in one module are numbered in traversal order, and #3120's own commit message notes that a traversal change "re-points deployed addresses".

What I did not verify: I measured the runtime mechanic — the rebinding, and what dispatch does with it — not a dev server actually producing it. @solidjs/vite-plugin's hotUpdate sends a full-reload on non-client runnable environments, which for runner-based dev servers means the SSR module is re-evaluated in the same process, and that is the shape the repro simulates. I have not driven a real dev server through the edit, so treat the HMR framing as the plausible trigger rather than a measured one. Scenario 1 needs no such assumption, and in a production build (modules evaluated once) it is the only route.

The consequence in either case is the same, and it is #3114's exemption pointed at a function that never signed it: a mutation reachable over GET, from any origin, with no origin proof, carrying the user's SameSite=Lax cookies.

Steps to reproduce

# Run against a build of the `next` BRANCH.
git checkout next && pnpm install
pnpm --filter @solidjs/web build:js
node repro.mjs        # from the repo root, so @solidjs/web resolves

repro.mjs:

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

const get = async (id, headers = {}) => {
  const r = await srv.handleServerFunctionRequest(
    new Request(`http://app.example/_server/${id}`, { method: "GET", headers }));
  return `${r.status}${r.headers.get("allow") ? ` Allow: ${r.headers.get("allow")}` : ""} ${await r.text()}`.trim();
};

// --- a declared read, as a build emits it
srv.GET(srv.createServerReference(srv.registerServerReference("shared-id", async () => ({ which: "READ" }))));
console.log("declared read, bare GET             ->", await get("shared-id"));

// --- the same id registered again with a DIFFERENT function
//     (an id collision, or a dev module runner re-evaluating an edited module)
let mutated = 0;
srv.registerServerFunction("shared-id", async () => ({ which: "MUTATION", mutated: ++mutated }));
console.log("after re-registration, no headers   ->", await get("shared-id"));
console.log("after re-registration, cross-site   ->", await get("shared-id", { "Sec-Fetch-Site": "cross-site" }));

// --- the same thing in the shape an edit actually takes: the id is
//     <name>-<xxhash32(path)>, so editing the file does not change it
srv.GET(srv.createServerReference(srv.registerServerReference("edited", async () => ({ v: 1 }))));
console.log("\nv1 declares GET, bare GET           ->", await get("edited"));
srv.createServerReference(srv.registerServerReference("edited", async () => ({ v: 2, sideEffect: "sent mail" })));
console.log("v2 does NOT declare GET, cross-site ->", await get("edited", { "Sec-Fetch-Site": "cross-site" }));

// --- control: a function that never declared GET
srv.createServerReference(srv.registerServerReference("never", async () => "NEVER"));
console.log("\nnever declared GET (control)        ->", await get("never"));
console.log("cross-site POST (control)           ->", await (async () => {
  const r = await srv.handleServerFunctionRequest(new Request("http://app.example/_server/shared-id",
    { method: "POST", headers: { "Sec-Fetch-Site": "cross-site", "X-Server-Function-Instance": "i" } }));
  return `${r.status} ${await r.text()}`.trim();
})());
console.log("\nmutations executed over GET:", mutated);

Output on next (d6a4a52f) — the block above.

Expected behavior

A GET() declaration governs the function it was made about. Rebinding the id revokes it; a function that never declared GET answers GET the way any other undeclared function does — 405 to a same-origin caller, refused to one that offers no origin proof.

Fix

I tried one — it sits at registerServerFunction rebinding an id without clearing METHODS — and a regression test that fails without it. Both are noise in an issue, so they are held for a PR rather than pasted here; say the word if you would rather see them inline.

Options

1. Revoke on rebind — the diff above. Smallest possible change and it reads as the contract. The cost is that registerServerFunction, a public registry write, gains a side effect on declaration state, and an integration that (unusually) called GET() before registering the implementation would silently lose its declaration. The compiler never emits that order.
2. Bind the declaration to the function, not the id — store METHODS.set(id, { method: "GET", fn: REGISTRATIONS.get(id) }) in GET(), and require METHODS.get(id).fn === REGISTRATIONS.get(id) at :2219. This is the precise statement of the contract and keeps registerServerFunction pure; it costs a second map read per dispatch and touches the Allow-header read at :2282. If the declaration state is going to be reasoned about again, I would rather it said this.
3. Refuse the collision loudly — throw, or warn, when registerServerFunction rebinds an id that already has a declaration. Surfaces scenario 1, which is the case where something is genuinely wrong, but it cannot be a hard error on its own: module re-evaluation rebinds legitimately. Useful as a dev-only warning alongside (1) or (2), not instead of them.
4. Make GET() the registration rather than a separate write over an id, so a declaration cannot be orphaned by construction. Cleanest, and an ABI change to what the compiler emits.
5. Leave it and treat a colliding id as the author's problem. Defensible for scenario 1 in a production build; the cost is that the failure is silent and it grants exactly the two things A GET-declared read is invocable cross-site, and nothing enforces that it is a read #3114 already flagged as the sharp edge.

I would take (2) for what it says and (1) if the smallest diff is what is wanted — they are behaviourally identical for every order the compiler emits — plus (3) as a dev warning either way.

Related

• A GET-declared read is invocable cross-site, and nothing enforces that it is a read #3114 is what makes this worth more than a 405: a declared read skips the origin gate deliberately, so an orphaned declaration does not merely open a method, it removes the origin check for a function that never asserted it was a safe read.
• Recent server-function fixes landed without tests that pin what they established #3120 pinned the id scheme as a wire contract. Its two properties are what make scenario 2 reachable: the hash is over the path (so edits do not re-key), and ordinals are assigned in traversal order (so a re-point needs no edit to the declaration at all).
• HEAD requests bypass the server function method gate and execute any registered function #3069 established the method allowlist that METHODS gates; this is that gate consulting a record whose subject has changed.

Environment