8000 Ensure conflicting flat headers in HTTP/2 are combined correctly by pimterry · Pull Request #4196 · nodejs/undici · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Ensure conflicting flat headers in HTTP/2 are combined correctly #4196

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 9, 2025
Merged

Ensure conflicting flat headers in HTTP/2 are combined correctly #4196

merged 2 commits into from
May 9, 2025

Conversation

pimterry
Copy link
Member
@pimterry pimterry commented May 6, 2025

This relates to...

When passing a flat header array and using HTTP/2, duplicate keys were discarded and lost unexpectedly.

In addition, the value-combining formatting (effectively join(',')) for explicitly array-valued headers in HTTP/2 didn't quite match the formatting for both Undici HTTP/1 or for Node HTTP/2's value-combining behaviour for headers that differ only by case (both of which do join(', ') - i.e. with whitespace) which was a bit weird.

Rationale

const client = new undici.Client('https://testserver.host', { allowH2: true })

client.request({
  method: 'GET',
  path: '/anything',
  headers: ['a', 'b', 'a', 'c']
}).then(async (response) => {
  console.log(await response.body.text())
})

The response shows what the server receives: headers including a: c but without a: b (all but the last header value is lost).

The header array-to-object logic ignored duplicate keys like this.

Regarding the formatting:

  • In Undici HTTP/1, { 'a': ['b', 'c'] } is sent as a: b, c (whitespace)
  • In Node built-in HTTP/2, { 'a': 'b', 'A': 'c' } is sent as a: b, c (whitespace)
  • In Undici HTTP/2, until now, { 'a': ['b', 'c'] } was sent as a: b,c (no whitespace)

(I'm not claiming this formatting is particularly important, but I would've had to reproduce the inconsistency in the tests, and it's nicer to tidy it up & make these headers a bit more readable en route).

Once nodejs/node#57917 is released (e.g. Node v24) we will be able to skip this entirely, and pass raw headers as-is directly to Node's HTTP/2 APIs, so these headers won't be combined at all - but that will have to wait until later, and will only apply for Node versions including that change anyway.

Changes

N/A

Features

N/A

Bug Fixes

  • Duplicate keys in HTTP/2 flat header arrays are now combined correctly, not discarded
  • Multi-valued HTTP/2 header values which are combined now include whitespace, for consistency with HTTP/1 and Node itself.

Breaking Changes and Deprecations

N/A

Status

pimterry added 2 commits May 6, 2025 14:36
@pimterry
fix: preserve raw headers with multi-values when using HTTP/2
3d4db26
@pimterry
fix: align automatic comma-separation for different H2 header cases
c95e1fb 
Undici's header handling combined array-valued header values without a
space. Node's built-in HTTP/2 header handling (used only for keys that
differ only in case) combined them with a space (", "). This makes those
match so everything has a space, which is a little more readable &
consistent.
@pimterry
Copy link
Member Author
pimterry commented May 6, 2025

Failing tests (inspection & sqlite-cache-store) seem unrelated to me.

@metcoder95 metcoder95 merged commit bd24250 into nodejs:main May 9, 2025
28 of 31 checks passed
This was referenced May 10, 2025
Merged
Closed
caitp pushed a commit to caitp/undici that referenced this pull request May 15, 2025
@pimterry @caitp
fix: Ensure conflicting flat headers in HTTP/2 are combined correctly (…
9d8b2cc 
…nodejs#4196)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metcoder95 metcoder95 metcoder95 approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pimterry @metcoder95 @Ethan-Arrowood
0