Skip to content

The CSRF origin matcher's return is coerced with !!, so a truthy non-boolean opens the gate #3169

Description

@frenzzy

Summary

The CSRF origin matcher's return value is coerced with !!, so a matcher that returns a truthy non-boolean allows the request. The gate fails open on the exact shapes a multi-tenant matcher is most likely to return.

Tested against next @ a536e29b, built from source, Node 24.19.

Reproduction

A matcher that means "deny" and says so with a string:

handleServerFunctionRequest(request, {
  createEvent: q => createRequestEvent(q),
  csrf: { origin: (origin) => "no" }        // typed `boolean`, returns a string
});

Request carries Origin: https://evil.example and no Sec-Fetch-Site (so the matcher is actually consulted — with that header present the gate short-circuits before the matcher and the probe proves nothing):

status = 200   matcher called = 1   server function ran = 1

The call is allowed, cross-origin, and the function executes with ambient cookies.

Which returns are safe and which are not

matcher returns outcome
true allowed (correct)
false 403 (correct)
undefined 403 — fails closed, correct
"no" / "deny" 200 — allowed
{ allowed: false } 200 — allowed
0 / "" 403 (correct by accident)

So the undefined case — the one a naive implementation hits by forgetting a return — is already safe. The dangerous shapes are the deliberate-looking ones: a verdict object, or a string naming the decision. Both are natural things to write in a matcher that also wants to log or explain its decision, and both read as "denied" to the author.

The declared type is boolean | Promise<boolean>, so this is out of contract — but TypeScript does not survive the .js integration, the compiled config, or a matcher whose return type widens through a helper. And the failure direction is the one that matters: a type error here disables the CSRF gate rather than breaking the build.

Why it is worth changing rather than documenting

Every other bound at this boundary refuses on a value it cannot interpret — an unusable body-format header answers 400 rather than calling the function with undefined, and an over-deep argument list is refused rather than truncated. The origin gate is the one place where an uninterpretable value is resolved permissively.

It is also the only hook whose wrong return has a security consequence rather than a correctness one. wrapInvocation returning the wrong thing produces a wrong result; csrf.origin returning the wrong thing produces an open door.

The strict idiom is already used, 23 lines below

if (typeof matcher === "function") return !!(await matcher(origin, request));   // ← coerces

return options.allowRequestsWithoutOriginCheck === true;                        // ← strict

Both decide the same thing — whether this request is allowed through the gate — and the second one already refuses to trust a truthy value. Whatever reasoning produced === true there applies here.

Options

  1. Require a real boolean(await matcher(origin, request)) === true. Fails closed on every non-boolean, matches the declared type exactly, and a matcher returning true is unaffected. Verified: every non-true row flips to 403 and the suite is unchanged (48 files / 524 passed / 2 skipped).
  2. Refuse a non-boolean loudly — treat it as a configuration error: 500 in production, throw in dev. Louder, and arguably right since it is a misconfiguration rather than a denied request, but it turns a currently-serving deployment into a failing one on upgrade.
  3. Accept boolean and reject everything else at configuration time, when configureServerFunctionsServer runs, rather than per request. Catches it once at boot instead of on every call; cannot catch a matcher whose return varies.
  4. Document that the return is coerced. Weakest — the coercion is invisible at the call site and the consequence is an open gate.

(1) looks right: it is the smallest change, it makes the runtime match its own declared type, and it moves the failure to the safe side without changing any correct matcher's behaviour.

Happy to send a PR. The regression test shape is a matcher table — true, false, undefined, "no", {}, 0 — asserting 403 for everything that is not true, with a Sec-Fetch-Site-free request so the matcher is genuinely reached (with the header present the gate short-circuits and every row passes vacuously).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions