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

http: fix IPv6 Host header check #13122

Merged
merged 1 commit into from
May 23, 2017
Merged

http: fix IPv6 Host header check #13122

merged 1 commit into from
May 23, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented May 19, 2017 •
edited
Loading

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

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)
  • http

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label May 19, 2017
@mscdex mscdex force-pushed the http-fix-ipv6-host-header branch from ede16b8 to d497722 Compare May 19, 2017 21:22
@refack
Copy link
Contributor

refack commented May 20, 2017

Windows fail:

135	parallel/test-http-host-header-ipv6-fail	
duration_ms	0.165
severity	fail
stack	
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: connect EADDRNOTAVAIL :::59105
    at Object.exports._errnoException (util.js:1026:11)
    at exports._exceptionWithHostPort (util.js:1049:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1156:14)

@mscdex need assistance on the Windows front?

@mscdex mscdex force-pushed the http-fix-ipv6-host-header branch from d497722 to fb2a7aa Compare May 20, 2017 04:47
@mscdex
Copy link
Contributor Author

mscdex commented May 20, 2017

It's passing now.

refack
refack approved these changes May 20, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

2 lite suggestions

test/parallel/test-http-host-header-ipv6-fail.js Outdated
this.close();
}, 2)).listen(0, function() {
const address = this.address();
http.get({
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you parameterize the literal 2s, and add a comment here that these are the N connection attempts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/_http_client.js
if (-1 !== (posColon = hostHeader.indexOf(':')) &&
-1 !== (posColon = hostHeader.indexOf(':', posColon)) &&
'[' !== hostHeader[0]) {
var posColon = hostHeader.indexOf(':');
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was sticking to the "style" of the original code. I'm also still reluctant to use const in runtime code because I've been burned so many times by it in the past (even when it hasn't triggered a permanent deopt).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack. Although before it was assigned to, and now it's just initiated.

@mscdex mscdex force-pushed the http-fix-ipv6-host-header branch from fb2a7aa to ae64ec9 Compare May 22, 2017 22:33
@mscdex
Copy link
Contributor Author

mscdex commented May 22, 2017 •
edited
Loading

New CI after test changes: https://ci.nodejs.org/job/node-test-pull-request/8238/

EDIT: CI is green.

refack
refack approved these changes May 22, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Even Better.

@mscdex
http: fix IPv6 Host header check
3774c99 
PR-URL: nodejs#13122
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@mscdex mscdex force-pushed the http-fix-ipv6-host-header branch from ae64ec9 to 3774c99 Compare May 23, 2017 03:06
@mscdex mscdex merged commit 3774c99 into nodejs:master May 23, 2017
@mscdex mscdex deleted the http-fix-ipv6-host-header branch May 23, 2017 03:07
jasnell pushed a commit that referenced this pull request May 23, 2017
@mscdex @jasnell
http: fix IPv6 Host header check
fe43027 
PR-URL: #13122
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell pushed a commit that referenced this pull request May 23, 2017
@mscdex @jasnell
http: fix IPv6 Host header check
414f93e 
PR-URL: #13122
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x?

@MylesBorins
Copy link
Contributor

ping @mscdex

@mscdex
Copy link
Contributor Author

mscdex commented Aug 14, 2017

Possibly, but I won't have time to backport anytime soon.

@MylesBorins
Copy link
Contributor

ping re: backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mscdex @refack @MylesBorins @jasnell