Skip to content

Don't crash when a remote beforeRequest callback returns nothing - #213

Merged
pimterry merged 1 commit into
httptoolkit:mainfrom
philipp-winterle:fix-remote-beforerequest-void-result
Sep 22, 2026
Merged

pimterry merged 1 commit into
httptoolkit:mainfrom
philipp-winterle:fix-remote-beforerequest-void-result

Conversation

@philipp-winterle

Copy link
Copy Markdown
Contributor

The problem

beforeRequest is documented and typed as returning nothing when a callback only
wants to inspect a request:

beforeRequest?: (req: CompletedRequest) => MaybePromise<CallbackRequestResult | void> | void;

Both halves of the local path honour that. PassThroughStepImpl.handle reads the
result as modifiedReq?.response / modifiedReq?.method, and
PassThroughStep.serialize deliberately puts undefined on the wire for it:

const serializedResult = callbackResult
    ? withSerializedCallbackBuffers(callbackResult)
    : undefined;

The receiving half doesn't. PassThroughStepImpl.deserialize passed that value
straight into withDeserializedCallbackBuffers, which reads .body off it, and
typed the channel response as non-optional so the compiler couldn't see it
coming. Every inspection-only beforeRequest therefore failed its request when
the rule was running behind an admin server:

TypeError: Cannot read properties of undefined (reading 'body')

beforeResponse, a few lines further down, already guards exactly this with
if (callbackResult && typeof callbackResult !== 'string').

Reproduction

The remote equivalent of the existing test
"should be able to run a callback that checks the request's data":

await remoteServer.forGet(targetServer.url).thenPassThrough({
    beforeRequest: (req) => { seenMethod = req.method; }
});
await request.get(targetServer.url, { proxy: remoteServer.url });
// => StatusCodeError: 500 - "TypeError: Cannot read properties of undefined (reading 'body')"

Still reproduces on 4.6.3, the current release.

The change

beforeRequest now returns the callback result untouched when it is falsy, the
way beforeResponse does, and its channel.request type admits undefined,
matching what serialize() declares on the other end. No behaviour changes for
callbacks that do return a result.

Test plan

  • Added the failing case to test/integration/remote-client.spec.ts, next to
    the other beforeRequest passthrough tests. Confirmed it fails with the
    error above before the fix and passes after.
  • mocha 'test/**/*.spec.ts' (the test:node suite) on this branch: 830
    passing, 6 pending. Two full runs each produced one failure, but a
    different one each time (client-error-events header overflow, then
    https.spec.ts TLS-bypass timeout), and neither touches this code path -
    they look like pre-existing flakiness in this environment. A run on
    unmodified main was clean.
  • Browser and perf suites not run locally.

beforeRequest is typed as returning CallbackRequestResult | void, and an
inspection-only callback that returns nothing is explicitly supported:
the in-process path reads its result as modifiedReq?.response, and
serialize() sends undefined over the wire for exactly that case.

The receiving side didn't expect it. It passed the result straight into
withDeserializedCallbackBuffers, which reads .body off it, so every such
callback failed the request with "Cannot read properties of undefined
(reading 'body')" whenever the rule ran behind an admin server.

beforeResponse, a few lines below, already guards this. beforeRequest now
does the same, and its channel.request type admits undefined, matching
what serialize() declares on the other end.
@CLAassistant

CLAassistant commented Sep 17, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@philipp-winterle

philipp-winterle commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

#214 fixes that flaky test

@pimterry
pimterry merged commit 7d03b2d into httptoolkit:main Sep 22, 2026
12 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants