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

Http2 reverses order of headers starting with ":" prefix #38797

Closed
blakebyrnes opened this issue May 24, 2021 · 2 comments
Closed

Http2 reverses order of headers starting with ":" prefix #38797

blakebyrnes opened this issue May 24, 2021 · 2 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@blakebyrnes
Copy link

blakebyrnes commented May 24, 2021

  • Version: 14.x, 15.x
  • Platform: Darwin Kernel Version 18.7.0: Mon Mar 8 22:11:48 PST 2021; root:xnu-4903.278.65~1/RELEASE_X86_64 x86_64
  • Subsystem:

What steps will reproduce the bug?

  const server = http2
    .createSecureServer(sslCerts(), (req, res) => {
      const headerOrder = req.rawHeaders.map((x, i) => {
        if (i % 2 === 0) return x;
        return undefined;
      });
      // expect headerOrder to be sent order, but will be [':path:',':scheme',':authority',:'method', 'accept-encoding','accept-language']
    })
    .unref()
    .listen(1664);

  const client = http2.connect(`https://localhost:1664`);

  const headers = {
    ':method': 'GET',
    ':authority': `localhost:1664`,
    ':scheme': 'https',
    ':path': '/temp1',
    'accept-encoding': 'gzip, deflate, br',
    'accept-language': 'en-US,en;q=0.9',
  };
  const h2stream = client.request(headers);

How often does it reproduce? Is there a required condition?

Everytime

What is the expected behavior?

Header order matching provided headers

What do you see instead?

Headers are reversed

Additional information

NodeJs is following the spec to insert special http2 headers in the front of the list, but since it traverses headers in order and prepends each one, the prefixed headers will be reversed:

ret = `${key}\0${value}\0${flags}${ret}`;

@blakebyrnes blakebyrnes reopened this May 24, 2021
@blakebyrnes blakebyrnes changed the title h http2 reverses headers starting with ":" prefix May 24, 2021
@blakebyrnes blakebyrnes changed the title http2 reverses headers starting with ":" prefix Http2 reverses order of headers starting with ":" prefix May 24, 2021
@jasnell
Copy link
Member

jasnell commented May 24, 2021

HTTP/2 assigns no significance to the ordering of pseudo-header fields beyond requiring that they appear before the other headers.

@blakebyrnes
Copy link
Author

That's a good point @jasnell. It doesn't violate the spec. I just found it to be unexpected behavior that all other http request functions in nodejs maintain the order you send them in. In http2, node will tell you the "headers sent" match the ones you send in, but if you look in wireshark, something different is sent. It seems unintentional - simply a byproduct of the header sorting that's happening. It seems like a minor change to create a second string for prefixed headers that's "appended", and then just concatenating with the rest of the headers to return.

@targos targos added the http2 Issues or PRs related to the http2 subsystem. label Aug 9, 2021
ofirbarak pushed a commit to ofirbarak/node that referenced this issue Jan 28, 2022
Keep prefixed headers order same as sent order

Fixes: nodejs#38797
ofirbarak pushed a commit to ofirbarak/node that referenced this issue Jan 28, 2022
Keep prefixed headers order same as sent order

Fixes: nodejs#38797
ofirbarak pushed a commit to ofirbarak/node that referenced this issue Jan 30, 2022
Keep pseudo-headers order same as sent order

Fixes: nodejs#38797
@jasnell jasnell closed this as completed in ee4bd95 Feb 6, 2022
ruyadorno pushed a commit that referenced this issue Feb 8, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 2, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 3, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Mar 4, 2022
Keep pseudo-headers order same as sent order

Fixes: nodejs#38797

PR-URL: nodejs#41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Mar 4, 2022
Keep pseudo-headers order same as sent order

Fixes: nodejs#38797

PR-URL: nodejs#41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 8, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 14, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants