-
Notifications
You must be signed in to change notification settings - Fork 80
Fix handling of IPv6 literal hosts in Net::HTTPGenericRequest
#237
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
Conversation
Since the cause of the CI errors has been resolved in power_assert v3.0.1, rerunning the CI should make the tests pass. |
|
At this point you could just use https://docs.ruby-lang.org/en/master/URI/HTTP.html#method-i-authority ? |
8d7b79e to
8eea621
Compare
@HoneyryderChuck Thank you for the comment. The
|
|
@sorah Thank you for reviewing. I've fixed the parts you pointed out in your comments. Only the following caused errors with older versions of Ruby or the uri gem, so I kept the original implementation: |
I agree raising minimum version to 3.0 as the present minimum version is too outdated. It's also worth to specify version to |
Should this Pull Request handle raising the minimum Ruby version? Personally, since this Pull Request is a bug fix, I think it's better not to include breaking changes such as raising the minimum Ruby version. As for the CI errors, I believe they will pass if you rerun the workflow. |
Yes, that's definitely fine along with a fix requires newer Ruby version. However if URI 0.10.0 still supports Ruby 2.6, then specifying |
9155b59 to
94523de
Compare
|
Since uri was part of the standard library (before it was gemified) in Ruby 2.6, I have updated the minimum Ruby version to 2.7. The uri version is specified as @sorah Does the version constraint
require 'uri'
URI::VERSION
=> "0.11.1"
URI.parse("//[::1]")
=> #<URI::Generic //[::1]>For reference, here are the uri gem versions included with each Ruby version:
|
|
oops, then |
|
|
I misread it. I will change it to |
Update uri dependency to version 0.11.0 or later to use `URI::HTTP#authority` and `URI#parse` without scheme Co-authored-by: 0x1eef <[email protected]> Co-authored-by: Sorah Fukumori <[email protected]>
ff8f9b4 to
a3a5bc4
Compare
@sorah I have fixed it. |
Background
When an IPv6 literal URI is passed as
uri_or_pathtoNet::HTTPGenericRequest, an issue in IPv6 handling causes the HTTP request headerHostto become incorrect.For example, it becomes
::1:8000instead of[::1]:8000.This incorrect
Hostheader leads to the following problems:http://[::1]result in an error with uri v1.0.4 - 1.1.0HostheaderDetails
This Pull Request fixes the handling of IPv6 literal hosts in
Net::HTTPGenericRequestwhen a URI with an IPv6 literal is passed.(This Pull Request revises and improves upon Pull Request #156.)
With this change, the
Hostheader in HTTP requests generated from IPv6 literal URIs will now have the correct format.Since this fix only resolves cases that previously caused HTTP 400 Bad Request errors, I think it does not introduce any compatibility issues.
Host: 2001:db8::1:8000Host: [2001:db8::1]:8000The behavior for non-IPv6 literal URIs remains unchanged.
Expected behavior
Hostheader (req['Host']) returns an correct valueNet::HTTPGenericRequest#update_uriwithhttp://[::1]Actual behavior
Hostheader (req['Host']) returns an incorrect valueNet::HTTPGenericRequest#update_uriraisesURI::InvalidComponentErrorwithhttp://[::1]when using uri v1.0.4 - 1.1.0System configuration
Additional Information
The CI errors intest (2.7 / windows-latest)andtest (2.6 / windows-latest)are unrelated to this fix and will be resolved once #236 is merged.(It resolved by power_assert v3.0.1.)