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

docs: forbidden headers not documented #1470

Open
shellscape opened this issue May 29, 2022 · 2 comments
Open

docs: forbidden headers not documented #1470

shellscape opened this issue May 29, 2022 · 2 comments
Labels
Docs Changes related to the documentation Status: help-wanted This issue/pr is open for contributions

Comments

@shellscape
Copy link
Contributor

(We don't have a documentation template here, so please forgive the use of a blank issue)

I recently ran into an invalid connection header error on refactoring some older code from axios to undici. I found this error to be particularly curious, because it wasn't documented in this repo and I couldn't find any reference to that error in the Node docs. For what it's worth, here's some of the code being called:

const headers = {
  Accept:
    'text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9',
  'Accept-Language': 'en-US,en;q=0.9,he-IL;q=0.8,he;q=0.7',
  'Cache-Control': 'no-cache',
  Connection: 'keep-alive',
  Pragma: 'no-cache',
  'Sec-Fetch-Dest': 'document',
  'Sec-Fetch-Site': 'same-origin',
  'Sec-Fetch-Mode': 'navigate',
  'Sec-Fetch-User': '?1',
  'Upgrade-Insecure-Requests': '1',
  'User-Agent':
    'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.163 Safari/537.36'
};

...

const { body, statusCode } = await request(uri, { headers, bodyTimeout: 5e3 });

Tracing this in code, it looks like undici marks a connection header as invalid in two places:

const forbiddenHeaderNames = [

and

undici/lib/core/request.js

Lines 281 to 285 in 1f2cca6

} else if (
key.length === 10 &&
key.toLowerCase() === 'connection'
) {
throw new InvalidArgumentError('invalid connection header')

With the latter being the spot that raised the error I ran into. Now, I don't believe the original dev was correct in their use of the keep-alive connection header here and it'll be removed, but I was really surprised that none of these forbidden headers were documented anywhere easily reachable, nor the conditions in which they are invalid. Perhaps this is another spot of documentation that was overlooked due to institutional knowledge of the contributors?

I love what's being done with this project, but documentation here seems to be more of an afterthought and that's a bummer.

@KhafraDev KhafraDev added Status: help-wanted This issue/pr is open for contributions Docs Changes related to the documentation labels May 30, 2022
@mcollina
Copy link
Member

mcollina commented May 30, 2022

Thanks for reporting the issue! Indeed, this project was born out of a proof-of-concept and the docs where added later.

Now, I don't believe the original dev was correct in their use of the keep-alive connection header here and it'll be removed

Original developer here. Can you clarify a little bit? Why do you think it was not correct? What are you trying to accomplish?

The reason connection is a banned header is that Undici manages the connection pools for you. Its
whole purpose is it allocate the requests to available connections.

@shellscape
Copy link
Contributor Author

Original developer here. Can you clarify a little bit?

Sorry for not being clear here. I meant the original developer of the code I was refactoring in our repo. I believe their use of keep-alive was erroneous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Changes related to the documentation Status: help-wanted This issue/pr is open for contributions
Projects
None yet
Development

No branches or pull requests

3 participants