Skip to content

Handle ipv6 addresses in host-header correctly with TLS - #53

Merged
fengmk2 merged 5 commits into
node-modules:masterfrom
mattiash:issue-52-ipv6-tls
Mar 8, 2018
Merged

fengmk2 merged 5 commits into
node-modules:masterfrom
mattiash:issue-52-ipv6-tls

Conversation

@mattiash

Copy link
Copy Markdown
Contributor

When a url uses an ipv6-addresses as the host part, the host-header of the request will be

(for ipv6 address ::1). To verify the IP address against a TLS certificate, we need to extract the
IP-address correctly.

Requires node with nodejs/node#14736 resolved to work.

Resolves issue #52

@codecov

codecov Bot commented Sep 29, 2017

Copy link
Copy Markdown

Codecov Report

Merging #53 into master will decrease coverage by 1.3%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   93.47%   92.17%   -1.31%     
==========================================
  Files           4        4              
  Lines         276      281       +5     
==========================================
+ Hits          258      259       +1     
- Misses         18       22       +4
Impacted Files Coverage Δ
lib/_http_agent.js 90.73% <33.33%> (-1.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c46df1...664b086. Read the comment docs.

@codecov-io

codecov-io commented Oct 20, 2017 •

Copy link
Copy Markdown

Codecov Report

Merging #53 into master will decrease coverage by 0.23%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   93.52%   93.28%   -0.24%     
==========================================
  Files           4        4              
  Lines         278      283       +5     
==========================================
+ Hits          260      264       +4     
- Misses         18       19       +1
Impacted Files Coverage Δ
lib/_http_agent.js 92.27% <66.66%> (-0.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a7a5c...375fc7d. Read the comment docs.

@fengmk2

fengmk2 commented Oct 20, 2017

Copy link
Copy Markdown
Member

@mattiash Nice work! Can you add a test case for this bug fix?

@fengmk2 fengmk2 added the bug label Oct 20, 2017
@mattiash

Copy link
Copy Markdown
Contributor Author

I have now added more tests that seem to increase the total coverage by 1.18%. codecov/patch fails because the tests do not cover one of the lines in my patch. That line handles the case where the host/port contains a '[' but no ']', which is a broken url that will never work in practice, but my patch needs to handle that case without crashing.

Is there anything else holding this back?

@mattiasholmlund

Copy link
Copy Markdown

The node-project accepted my PR for nodejs/node#14736. It contains the same code as this PR. Can this be merged now?

@mattiasholmlund

Copy link
Copy Markdown

I would really like to have this fix merged. Is there anything else I can do?

When a url uses an ipv6-addresses as the host part, the host-header of the request will be

[::1]:3000

(for ipv6 address ::1). To verify the IP address against a TLS certificate, we need to extract the
IP-address correctly.

Requires node with nodejs/node#14736 resolved to work.
@mattiash
mattiash force-pushed the issue-52-ipv6-tls branch from 7efec2e to 5b544a5 Compare March 7, 2018 06:46
@mattiasholmlund

Copy link
Copy Markdown

The nodejs-project released version 8.10.0 (LTS) today which includes my patch for ipv6 addresses in certificates. I have updated this PR with checks for if IPv6 is available on the test-environment and if a version of node with my fixes is used. All tests now pass on both travis and appveyor. The coverage decreases in the codecov report, but that is (probably) because codecov seems to run the tests on an environment with IPv6 disabled and then the IPv6 tests will not run and thus the code will be untested. appveyor runs with IPv6 enabled, so the code is actually tested.

I would be very happy if you could merge this PR and make a new release...

@fengmk2

fengmk2 commented Mar 8, 2018

Copy link
Copy Markdown
Member

@mattiash @mattiasholmlund Thanks a lot!

@fengmk2
fengmk2 merged commit 4d3a3b1 into node-modules:master Mar 8, 2018
@fengmk2

fengmk2 commented Mar 8, 2018

Copy link
Copy Markdown
Member

3.4.1 Released

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants