Skip to content

A throwing transformResult is contained on the return path and escapes the handler on the thrown path #3171

Description

@frenzzy

Summary

transformResult is invoked from two sites. The one on the success path sits inside dispatch()'s try; the one on the thrown path sits inside the catch, with no try of its own. The same hook, given the same failing implementation, therefore behaves in two opposite ways — and the uncontained one is the common mutation shape.

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

Reproduction

One transformResult that throws, two server functions — one returning, one doing throw redirect("/after"):

{ createEvent, transformResult: () => { throw new Error("HOOK-BOOM"); } }
return path : status=500  err=Internal Server Error  cookie=["sid=keepme"]  fn ran=1
thrown path : HANDLER REJECTED -> HOOK-BOOM  (no status, no cookie)         fn ran=1

Three differences, all in the wrong direction on the thrown path:

  • No response at all. The handler's promise rejects; the host adapter decides what the caller sees.
  • The event stub's Set-Cookie is lost. Refusals after createEvent drop the response stub's Set-Cookie silently #3159 made every exit fold the stub, but an escape is not an exit — it never reaches commitEventResponse.
  • Sanitization is bypassed. The raw HOOK-BOOM message reaches the caller, where the contained path correctly redacted it to Internal Server Error. That inverts the production error-disclosure policy for exactly the path that escaped.

It is broader than one hook

Every hook reached from that catch escapes the same way. transformFlightResult, invoked via foldFlightData, has the identical split — swap the hook for { collectFlightData: () => ({k:1}), transformFlightResult: () => { throw new Error("FLIGHT-BOOM"); } } and add X-Single-Flight: true:

ret -> status 500
thr -> HANDLER REJECTED -> FLIGHT-BOOM

So the fix wants to contain the whole thrown-path branch, not just the one call. Wrapping only transformResult leaves transformFlightResult escaping through the same hole.

Why the asymmetry matters more than it looks

The thrown path is not an edge case — throw redirect(...) is the standard shape for a mutation that navigates, and throw is how a server function signals failure at all. So a result policy that an author tested against every returned value works fine until the first redirect, and then takes down the request, loses the session cookie the stub was carrying, and leaks its own error text.

It is also the shape an author cannot easily discover: the hook is one function, its docblock describes one contract, and nothing at the call site suggests the containment differs by path.

Compare the sibling containment twenty lines away in the flight fold: collectFlightData is wrapped in a per-source try/catch with a comment explaining precisely why a hook's failure must not cost a committed mutation its outcome. transformFlightResult, in the same function, is bare. The reasoning already exists in the tree; it is applied unevenly.

Options

  1. Contain the whole thrown-path branch — wrap the catch's control-flow branch so its catch re-enters the existing sanitized-error tail. Verified: both paths answer 500 | Internal Server Error | Set-Cookie ["sid=keepme"], the flight-hook variant is contained too, and the suite is unchanged (48 files / 524 passed / 2 skipped). Wrapping only the transformResult call also passes but leaves transformFlightResult escaping.
  2. Wrap it and attribute it — same containment, but tag the resulting error as originating in the hook rather than in the function, so an author is not told "my function failed" when their result policy failed. Addresses the related misattribution problem (see below) at the same seam.
  3. Let it escape on both paths. Consistent in the other direction, and defensible if the position is that a result-policy failure is a programming error the host should see. But it would regress the return path's current behaviour, which is the safer one, and it would drop cookies on every hook failure.
  4. Document the asymmetry. I would argue against it: the contract cannot be honoured by an author who does not know which path a given call will take.

(1) is the obvious minimum; (2) is worth considering while the code is open, because the misattribution is a real cost — a contained hook failure is currently byte-identical at the client to a genuinely failing server function (Error: Internal Server Error), and for wrapInvocation the function has not even run. That is a separate finding and I am happy to file it separately if you would rather keep this one narrow.

Happy to send a PR with the containment plus a regression test asserting both paths produce the same status, the same sanitized message, and the same surviving cookie, with a non-throwing hook as the control.

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