nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.26k stars 545 forks source link

Normalize headers in interceptors #3860

Open DTrombett opened 2 days ago

DTrombett commented 2 days ago

This would solve...

Some interceptors assume the request headers are necessarily objects: https://github.com/nodejs/undici/blob/6551919a3e492746806604cec2d241385c391f35/lib/interceptor/cache.js#L238-L242 https://github.com/nodejs/undici/blob/6551919a3e492746806604cec2d241385c391f35/lib/interceptor/dns.js#L360-L363 This creates problems when headers are not objects or when they get converted to an array, like in the redirect interceptor: https://github.com/nodejs/undici/blob/6551919a3e492746806604cec2d241385c391f35/lib/handler/redirect-handler.js#L231-L236 Using the redirect handler with the dns one, in fact, will completely break headers (see example below)

The implementation should look like...

IMHO, the interceptors should convert the request headers to an object before using them, considering that it makes them easy to manipulate and most users will provide them already as objects, so no conversion is needed.

I have also considered...

An alternative could be to fix the interceptors to handle the headers based on the possible types, but this would make very difficult to implement some logics, like checking the value of some headers (which is needed in the cache interceptor)

Additional context

Using redirect and dns interceptors:

createServer((req, res) => {
    if (req.url?.endsWith("/redirect")) {
        console.log(req.headers);
        exit();
    }
    res.setHeader("location", "/redirect");
    res.statusCode = 302;
    res.end();
}).listen(8008);

request("http://localhost:8008", {
    headers: { "user-agent": "something", hello: "world" },
    dispatcher: new Agent().compose(
        interceptors.dns(),
        interceptors.redirect({ maxRedirections: 1 }),
    ),
});
// This logs
// {
//   '0': 'user-agent',
//   '1': 'something',
//   '2': 'hello',
//   '3': 'world',
//   host: 'localhost',
//   connection: 'keep-alive'
// }
metcoder95 commented 1 day ago

It is true that interceptors are making wrong assumptions of the headers, and happy to receive PR for helping fixing them.

But I doubt will be easy to normalize them as we allow callers to use dispatch in different forms when it regards to headers. Either Array or Objects are passed, and as we are somehow patching the dispatch in an stack manner, the top most interceptor should be the one normalizing the headers.

I'd prefer fixing them accordingly in sync with the contract we set for dispatch, unless there's an easier way of doing so.

mcollina commented 1 day ago

cc @ronag

ronag commented 1 day ago

on my list