Skip to content

prepareRequest's return replaces the init unvalidated, so a hook that returns a fresh object drops the payload, the signal and the protocol headers #3174

Description

@frenzzy

Summary

prepareRequest's return value replaces the request init wholesale with no validation:

init = (await config.prepareRequest(init, )) || init;

A hook that returns a fresh object — the natural way to write "add an auth header" — silently discards the argument payload, the abort signal, and every protocol header, and the call still dispatches.

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

Reproduction

The shape an auth-token hook is most likely to take:

prepareRequest: init => ({ headers: { Authorization: "Bearer " + token } })

What actually goes on the wire:

method: undefined | body: undefined | signal present: false
headers: {"Authorization":"Bearer t"}

Lost: the mutation's entire argument payload, the caller's AbortSignal, X-Server-Function-Instance, Content-Type, and X-Server-Function-Format. The request is still sent.

Returning a non-empty string also passes the || init guard and loses the body the same way.

Why the failure direction is the problem

The correct usage is to spread — init => ({ ...init, headers: { ...init.headers, Authorization: … } }) — and an author who does that is unaffected. But the incorrect usage does not fail: it produces a request that looks plausible, reaches the server, and is refused or mis-executed there, with nothing on the client naming the cause.

The two hooks either side of this one are stricter. A custom fetch returning the wrong shape rejects with a TypeError; wrapInvocation documents that replacing the result replaces the result. prepareRequest is the one that accepts anything object-shaped and quietly proceeds.

Correcting my own filing: that "Forward init — the call's signal rides on it…" paragraph belongs to the neighbouring fetch option's docblock (client.ts:147), not to prepareRequest's. prepareRequest's own docblock says only "Return (or mutate and return) the RequestInit the transport will use" and shows a spreading example. So the obligation is implied by example, not stated — which strengthens the case rather than weakening it.

Also more precise than I filed it: the string return does not reach the wire. The platform fetch refuses a string init and the call rejects with an opaque TypeError naming nothing. Only the object case is dispatched — as a bare GET, which the handler answers 405 with the function never invoked.

Options

  1. Validate the returned init — require the protocol headers to survive (X-Server-Function-Instance still present) and throw naming prepareRequest otherwise. Verified: both bad hooks throw at the call site naming the hook, the spreading control is unaffected, suite unchanged (48 files / 524 passed / 2 skipped).

    Do not also require the body, method or signal to be identical. I tried that first and it breaks test/server/server-functions-extensions.spec.tsx:292"does not consume a streaming body to show it to observers", which spreads init and then deliberately replaces body with a ReadableStream — a legitimate, tested use. Swapping the body is in contract; dropping the transport headers is not.

  2. Merge rather than replace — treat the return as a patch over the original init ({ ...init, ...returned, headers: merge(...) }). Makes the natural mistake correct by construction. Changes the hook's semantics for anyone deliberately replacing a field with undefined, which seems unlikely but is a behaviour change.

  3. Only accept a return that is the same object, i.e. document the hook as mutate-in-place and ignore any other return. Simplest to reason about; a larger break for existing hooks that return a new object correctly.

  4. Warn in dev when protocol headers went missing. Cheapest, keeps behaviour identical, and catches the mistake without constraining the hook.

I'd suggest (1), or (4) if adding a throw to this path is too strong. (2) is the most forgiving but changes what the hook means, which deserves a deliberate decision rather than a bug fix.

Happy to send a PR. The regression test shape: a hook returning a bare {headers} object either throws (option 1) or still carries the payload and signal (option 2), with a correctly-spreading hook as the control that passes today.

Related

Same shape at three seams — an integration hook's return is accepted unvalidated where the mistake is invisible: #3170 and #3172. Different packages and different fixes, so filed separately, but the enforcement question is one question and is cheaper to settle once.

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