http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding "connection' into header by sagitsofan · Pull Request #23908 · nodejs/node · GitHub

lib: http2 compatibility connection header

When adding the connection header into
http2 response, an ERR_HTTP2_INVALID_CONNECTION_HEADERS
error is thrown.

This PR is ignoring the connection header and disable the
ERR_HTTP2_INVALID_CONNECTION_HEADERS error.
A new warning log is emitted on the compatibility.

Fixes: #23748

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

This would change the behavior for non-compat mode also, which is not ideal. The check should be done in compat.js rather than in util.js

Ok.
I will move it into the compatibility API, and update this PR.

This is unfortunately not the right approach. I've left some comments.

@apapirovski @jasnell @dougwilson

What about this PR, it is waiting for approval long time.

Hi @sagitsofan , I was just a passer by, similar to you and just had a comment; I don't have the ability to move this further along and eventually merge, sorry.

LGTM

@jasnell @apapirovski can you check again?

@sagitsofan can you please update the title/description on the PR to something more related the actual change?

CI: https://ci.nodejs.org/job/node-test-pull-request/19145/

@mcollina i updated the title and description and fix the conflicts

@jasnell @apapirovski Can you please review?

@jasnell ?

Can you please add a unit test?

a unit test is needed

Ignoring the connection header and disable the `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. Added a warning log on the compatibility. Fixes: nodejs#23748

@jasnell @mcollina Added test, please review.

The test should verify if the warning is emitted.

@mcollina, I have verified that the warning is emitted and also i am making sure that adding the connection into the header is not throwing any exception.

LGTM

@jasnell Pinging again :-)

Thanx.
Who is landing this PR?

Nit: this could be simplified to:

return name !== constants.HTTP2_HEADER_CONNECTION ||
       value === 'trailers';

CI https://ci.nodejs.org/job/node-test-pull-request/19694/

Landed in 8c597df 🎉

I took the liberty to improve the commit message and addressed my nit while landing.

When using the compatibility API the connection header is from now on ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. This logs a warning in such case to notify the user about the ignored header. PR-URL: nodejs#23908 Fixes: nodejs#23748 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

When using the compatibility API the connection header is from now on ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. This logs a warning in such case to notify the user about the ignored header. PR-URL: #23908 Fixes: #23748 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Oct 26, 2018

sagitsofan force-pushed the http_compat branch from ec4bdb1 to 6f9c3f6 Compare October 26, 2018 19:33

jasnell requested changes Oct 26, 2018

View reviewed changes

jasnell reviewed Oct 26, 2018

View reviewed changes

Comment thread lib/internal/http2/util.js Outdated

apapirovski suggested changes Oct 27, 2018

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

Comment thread lib/internal/http2/compat.js Outdated

Comment thread lib/internal/http2/compat.js Outdated

dougwilson reviewed Oct 27, 2018

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

jasnell reviewed Oct 29, 2018

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

kanongil mentioned this pull request Nov 12, 2018

UnhandledPromiseRejection: Improper headers on http2 response hapijs/hapi#3830

Closed

sagitsofan mentioned this pull request Nov 30, 2018

http2/http1 compatibility API error with connection header #23748

Closed

mcollina previously approved these changes Dec 3, 2018

View reviewed changes

sagitsofan changed the title Http compat Lib: Http2, Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" when adding connection header Dec 3, 2018

sagitsofan changed the title Lib: Http2, Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" when adding connection header http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" when adding connection header Dec 3, 2018

sagitsofan changed the title http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" when adding connection header http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding connection into header Dec 3, 2018

sagitsofan changed the title http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding connection into header http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding "connection' into header Dec 3, 2018

sagitsofan force-pushed the http_compat branch from 9417763 to 3852abc Compare December 3, 2018 18:14

joyeecheung mentioned this pull request Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

apapirovski approved these changes Dec 9, 2018

View reviewed changes

lib: http2 compatibility error with connection header

685d968

Ignoring the connection header and disable the `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. Added a warning log on the compatibility. Fixes: nodejs#23748

sagitsofan added 4 commits December 14, 2018 21:46

c

d7fd930

new line

7066aa7

added test for the connection header

058a029

Merge branch 'master' of https://github.com/nodejs/node into http_compat

56f165c

sagitsofan force-pushed the http_compat branch from 967d805 to 56f165c Compare December 15, 2018 10:24

mcollina reviewed Dec 15, 2018

View reviewed changes

verify warning is emitted

0a3cedc

mcollina approved these changes Dec 15, 2018

View reviewed changes

jasnell approved these changes Dec 20, 2018

View reviewed changes

BridgeAR approved these changes Dec 20, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 3, 2019

BridgeAR closed this Mar 4, 2019

BridgeAR mentioned this pull request Mar 4, 2019

v11.11.0 proposal #26322

Merged

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

+                if (name !== constants.HTTP2_HEADER_CONNECTION)
+                  return true;
+                else
+                  return value === 'trailers';

http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding "connection' into header by sagitsofan · Pull Request #23908 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding "connection' into header - #23908

http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding "connection' into header#23908sagitsofan wants to merge 22 commits intonodejs:masternodejs/node:masterfrom sagitsofan:http_compatsagitsofan/node:http_compatCopy head branch name to clipboard

Conversation

sagitsofan commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sagitsofan commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sagitsofan commented Nov 23, 2018

Uh oh!

sagitsofan commented Nov 30, 2018

Uh oh!

dougwilson commented Nov 30, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 3, 2018

Uh oh!

mcollina commented Dec 3, 2018

Uh oh!

sagitsofan commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagitsofan commented Dec 8, 2018

Uh oh!

sagitsofan commented Dec 11, 2018

Uh oh!

mcollina commented Dec 11, 2018

Uh oh!

sagitsofan commented Dec 15, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

sagitsofan commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

sagitsofan commented Dec 19, 2018

Uh oh!

sagitsofan commented Dec 20, 2018

Uh oh!

BridgeAR Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 20, 2018

Uh oh!

BridgeAR commented Mar 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

http2: Fix "ERR_HTTP2_INVALID_CONNECTION_HEADERS" error when adding "connection' into header#23908
sagitsofan wants to merge 22 commits into
nodejs:masterfrom
sagitsofan:http_compat

sagitsofan commented Oct 26, 2018 •
edited
Loading

sagitsofan commented Oct 27, 2018 •
edited
Loading

sagitsofan commented Dec 4, 2018 •
edited
Loading

sagitsofan commented Dec 15, 2018 •
edited
Loading