-
Notifications
You must be signed in to change notification settings - Fork 58
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.
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
Handle ipv6 addresses in host-header correctly with TLS #53
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@mattiash Nice work! Can you add a test case for this bug fix? |
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? |
The node-project accepted my PR for nodejs/node#14736. It contains the same code as this PR. Can this be merged now? |
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.
7efec2e
to
5b544a5
Compare
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... |
@mattiash @mattiasholmlund Thanks a lot! |
3.4.1 Released |
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