-
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
base: master
Are you sure you want to change the base?
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 ? |
8caf622 to
8d7b79e
Compare
Co-authored-by: 0x1eef <0x1eef@users.noreply.github.com> Co-authored-by: Sorah Fukumori <sora134@gmail.com>
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. |
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.)