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

sagitsofan · 2018-10-26T18:46:28Z

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

jasnell

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

lib/internal/http2/util.js

sagitsofan · 2018-10-27T07:57:31Z

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

apapirovski

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

lib/internal/http2/compat.js

sagitsofan · 2018-11-23T06:01:01Z

@apapirovski @jasnell @dougwilson

sagitsofan · 2018-11-30T19:10:39Z

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

dougwilson · 2018-11-30T19:18:22Z

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.

mcollina

LGTM

@jasnell @apapirovski can you check again?

mcollina · 2018-12-03T10:08:34Z

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

mcollina · 2018-12-03T10:11:11Z

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

sagitsofan · 2018-12-04T06:16:26Z

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

sagitsofan · 2018-12-08T08:33:40Z

@jasnell @apapirovski Can you please review?

sagitsofan · 2018-12-11T13:22:11Z

@jasnell ?

mcollina · 2018-12-11T13:36:55Z

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

sagitsofan · 2018-12-15T16:16:15Z

@jasnell @mcollina Added test, please review.

mcollina

The test should verify if the warning is emitted.

sagitsofan · 2018-12-15T16:32:18Z

@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.

mcollina

LGTM

sagitsofan · 2018-12-19T16:30:24Z

@jasnell Pinging again :-)

sagitsofan · 2018-12-20T16:49:50Z

Thanx.
Who is landing this PR?

BridgeAR · 2018-12-20T16:54:59Z

lib/internal/http2/compat.js

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


Nit: this could be simplified to:

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

BridgeAR · 2018-12-20T16:56:35Z

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

BridgeAR · 2019-03-04T02:07:02Z

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

lib/internal/http2/util.js Outdated Show resolved Hide resolved

apapirovski suggested changes Oct 27, 2018

View reviewed changes

lib/internal/http2/compat.js Outdated Show resolved Hide resolved

lib/internal/http2/compat.js Outdated Show resolved Hide resolved

lib/internal/http2/compat.js Outdated Show resolved Hide resolved

dougwilson reviewed Oct 27, 2018

View reviewed changes

lib/internal/http2/compat.js Outdated Show resolved Hide resolved

jasnell reviewed Oct 29, 2018

View reviewed changes

lib/internal/http2/compat.js Outdated Show resolved Hide resolved

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 pending requests for changes, 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

Uh oh!

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

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