nodejs / nodejs/undici

Handlers incorrectly assume some methods are defined

Open
#3,856 5 comments 0 reactions 1 assignee View on GitHub

@ronag is already working on this.

Since Nov 29, 2024.

bug
Dominant language
JavaScript
Stars
7.7k
Forks
880
Avg merge
2d 16h
Merged PRs (30d)
68

Description

Bug Description

Most built-in handlers check if the handler passed is valid (or assume it is) instead of letting the dispatch function handle everything by default. This means, for example, that if I use an interceptor that changes the handler to add methods like onConnect with a built-in interceptor, I will get an error.

Reproducible By

import { Agent, interceptors } from "undici";

// First example
new Agent()
    .compose(
        (dispatch) => (opts, handler) =>
            dispatch(opts, {
                ...handler,
                onConnect() {},
                onHeaders: () => true,
                onComplete() {},
            }),
        interceptors.redirect({ maxRedirections: 1 }),
    )
    .dispatch(
        { method: "GET", path: "/", origin: "https://example.com" },
        {
            onError() {},
            onData: () => true,
        },
    );
// Second example
new Agent()
    .compose(
        (dispatch) => (opts, handler) =>
            dispatch(opts, {
                ...handler,
                onConnect() {},
                onHeaders: () => true,
                onComplete() {},
            }),
        interceptors.retry(),
    )
    .dispatch(
        { method: "GET", path: "/", origin: "https://example.com" },
        {
            onError() {},
            onData: () => true,
        },
    );

Expected Behavior

The request should be successful, and the custom interceptor handler should be used correctly

Logs & Screenshots

# First example
D:\-\node_modules\undici\lib\core\util.js:515
    throw new InvalidArgumentError('invalid onConnect method')
          ^
InvalidArgumentError: invalid onConnect method
    at Object.assertRequestHandler (D:\-\node_modules\undici\lib\core\util.js:515:11)
    at new RedirectHandler (D:\-\node_modules\undici\lib\handler\redirect-handler.js:41:10)
    at Proxy.Intercept (D:\-\node_modules\undici\lib\interceptor\redirect.js:15:31)
    at file:///D:/-/test.js:14:3
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5) {
  code: 'UND_ERR_INVALID_ARG'
}

# Second example
D:\-\node_modules\undici\lib\handler\retry-handler.js:73
    this.handler.onConnect(reason => {
                 ^
TypeError: this.handler.onConnect is not a function
    at new RetryHandler (D:\-\node_modules\undici\lib\handler\retry-handler.js:73:18)
    at Proxy.retryInterceptor (D:\-\node_modules\undici\lib\interceptor\retry.js:9:9)
    at file:///D:/-/test.js:14:3
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5)

Environment

undici@7.0.0-alpha.6, Node v22.11.0, Windows 11 Home

Additional context

I would send a PR to address this issue, but the behavior is not the same in all interceptors so I am not sure what the expected result should be. For example, the redirect handler checks if the passed handler is completely valid and assumes that the various methods such as onUpgrade, onConnect and onError are defined:
https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/redirect-handler.js#L41 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/redirect-handler.js#L89 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/redirect-handler.js#L93 The retry handler, instead, doesn't check if the passed handler is valid but does assume that some methods are defined, like onConnect and onError but not onUpgrade:
https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/retry-handler.js#L73 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/retry-handler.js#L323 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/retry-handler.js#L90
In my opinion, no method should be assumed to be present (since it may be present in a subsequent interceptor) and thus the handler should not be checked for validity.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.