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.

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

test: http2 util passing array in te header #16246

Closed
wants to merge 1 commit into from
Closed

test: http2 util passing array in te header #16246

wants to merge 1 commit into from

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Oct 17, 2017

This adds a test case in which array with two values is passed to param value in isIllegalConnectionSpecificHeader:

node/lib/internal/http2/util.js

Line 363 in 212de3c

const val = Array.isArray(value) ? value.join(', ') : value;

Refs: #14985

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, http2

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 17, 2017
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 17, 2017
mcollina
mcollina approved these changes Oct 17, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

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

@apapirovski
Copy link
Member

apapirovski commented Oct 17, 2017 •
edited
Loading

Just a note for future reference if someone wants to take it on, the pathway this is testing could actually be refactored a bit:

node/lib/internal/http2/util.js

Lines 363 to 364 in ff747e3

const val = Array.isArray(value) ? value.join(', ') : value;
return val !== 'trailers';

value will never be an array with length < 2 because of

node/lib/internal/http2/util.js

Lines 405 to 416 in ff747e3

const isArray = Array.isArray(value);
if (isArray) {
switch (value.length) {
case 0:
continue;
case 1:
value = String(value[0]);
break;
default:
if (kSingleValueHeaders.has(key))
return new errors.Error('ERR_HTTP2_HEADER_SINGLE_VALUE', key);
}

so it could just be rewritten as:

return value !== 'trailers';

@trivikr
Copy link
Member Author

trivikr commented Oct 17, 2017

Thanks @apapirovski, I've updated the commit to add toString() to cover both cases as follows:

return value.toString() !== 'trailers';

Commit is at trivikr@596df98
However, I'm not able to see it in this PR https://github.com/nodejs/node/pull/16246/files
Could this be because CI is running on older commit?

@trivikr trivikr force-pushed the test-http2-util-headers-list branch from 7b2c4ef to 596df98 Compare October 17, 2017 16:22
apapirovski
apapirovski reviewed Oct 17, 2017
View reviewed changes
lib/internal/http2/util.js Outdated
@@ -360,8 +360,7 @@ function isIllegalConnectionSpecificHeader(name, value) {
case HTTP2_HEADER_TRANSFER_ENCODING:
return true;
case HTTP2_HEADER_TE:
const val = Array.isArray(value) ? value.join(', ') : value;
return val !== 'trailers';
return value.toString() !== 'trailers';
Copy link
Member

@apapirovski apapirovski Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString() isn't necessary here (and is bad for perf). If an array slips through then it won't match anyway (because it has at least two items in it). Should just be value !== 'trailers'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed toString() in the updated commit

@trivikr
test: http2 util passing array in te header
9a03c2a 
This adds a test case in which array with two values is passed to
param value in isIllegalConnectionSpecificHeader

Refs: nodejs#14985
@trivikr trivikr force-pushed the test-http2-util-headers-list branch from 596df98 to 9a03c2a Compare October 17, 2017 17:59
apapirovski
apapirovski approved these changes Oct 17, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@apapirovski
Copy link
Member

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

apapirovski pushed a commit to apapirovski/node that referenced this pull request Oct 19, 2017
@trivikr @apapirovski
http2: simplify te header validation, add test
1fd662c 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs#16246
Refs: nodejs#14985
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@apapirovski
Copy link
Member

Landed in 1fd662c

@trivikr trivikr mentioned this pull request Oct 19, 2017
Closed
@trivikr trivikr deleted the test-http2-util-headers-list branch October 22, 2017 21:56
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@trivikr @MylesBorins
http2: simplify te header validation, add test
25ef372 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: #16246
Refs: #14985
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@trivikr @addaleax
http2: simplify te header validation, add test
e851661 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs/node#16246
Refs: nodejs/node#14985
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@trivikr @addaleax
http2: simplify te header validation, add test
49cbad1 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs/node#16246
Refs: nodejs/node#14985
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@trivikr @mcollina @apapirovski @jasnell @cjihrig @mscdex @MylesBorins @nodejs-github-bot