Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize headers in interceptors #3860

Open
DTrombett opened this issue Nov 21, 2024 · 6 comments
Open

Normalize headers in interceptors #3860

DTrombett opened this issue Nov 21, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@DTrombett
Copy link
Contributor

This would solve...

Some interceptors assume the request headers are necessarily objects:

headers: {
...opts.headers,
'if-modified-since': new Date(result.cachedAt).toUTCString(),
etag: result.etag
}
headers: {
host: origin.hostname,
...origDispatchOpts.headers
}
This creates problems when headers are not objects or when they get converted to an array, like in the redirect interceptor:
const entries = typeof headers[Symbol.iterator] === 'function' ? headers : Object.entries(headers)
for (const [key, value] of entries) {
if (!shouldRemoveHeader(key, removeContent, unknownOrigin)) {
ret.push(key, value)
}
}
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'
// }
@DTrombett DTrombett added the enhancement New feature or request label Nov 21, 2024
@metcoder95
Copy link
Member

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
Copy link
Member

cc @ronag

@ronag
Copy link
Member

ronag commented Nov 22, 2024

on my list

@metcoder95
Copy link
Member

Do you have something in mind for this @ronag?

@ronag
Copy link
Member

ronag commented Nov 25, 2024

@metcoder95 the new hook api always passes along normalized headers #3878

@ronag
Copy link
Member

ronag commented Nov 25, 2024

Ah, this is for the request part. Tricky... the compose API doesn't quite support this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants