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.

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

fix parsing / printing of IPv6 addresses in URIs #169

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

anmonteiro
Copy link
Contributor

fixes #163

@bikallem
Copy link

bikallem commented Feb 16, 2023

I recently came across the same issue referenced by this PR.

Are the CI failures related to this PR? From the tests, the implementation looks correct.

@tmcgilchrist
Copy link
Member

@anmonteiro if you rebase this the build should pass.
see https://github.com/tmcgilchrist/ocaml-uri/pull/2/checks?check_run_id=12145037435

@anmonteiro anmonteiro force-pushed the anmonteiro/fix-ipv6-sq-bracket branch from eb2eee1 to cd56175 Compare March 27, 2023 06:08
Copy link
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

These changes look reasonable to me. Just noting this would be an API breaking change and needs a major version bump.

@avsm
Copy link
Member

avsm commented Apr 19, 2023

This looks good to me after a read through the code, but I'd like to just run it through crowbar a bit more. It should be ok for a uri.5.0 release soon.

@anmonteiro anmonteiro force-pushed the anmonteiro/fix-ipv6-sq-bracket branch from cd56175 to 94786d3 Compare April 28, 2023 08:57
@anmonteiro anmonteiro force-pushed the anmonteiro/fix-ipv6-sq-bracket branch from 94786d3 to cca065b Compare April 28, 2023 09:34
@lindig
Copy link

lindig commented Jul 5, 2023

Could you consider merging this? We just hit this bug when trying to move to IPv6.

benjamreis added a commit to xcp-ng-rpms/xs-opam-repo that referenced this pull request Jul 7, 2023
@bernhardkaindl
Copy link

@avsm We likewise had to take this change as patch into XenServer as we need it working too!

BTW:

  • Pau also manually tested it to work as expected using an utop session:
    # #require "uri" ;;
    # let uri = Uri.of_string "http://[::1]";;
    # Uri.path uri ;;
    - : string = ""
    # Uri.host uri ;;

I hope this helps you to review it for merging.

benjamreis added a commit to xcp-ng-rpms/xs-opam-repo that referenced this pull request Jul 18, 2023
benjamreis added a commit to xcp-ng-rpms/xs-opam-repo that referenced this pull request Jul 18, 2023
@benjamreis
Copy link

FYI I backported this fix in a previous version of the lib also presenting the IPv6 URI bug and it has indeed solved it.
Eager to see this PR merged and to get the fix from upstream! 👍

let host uri =
match uri.host with
| None -> None
| Some (`Ipv4_literal h | `Ipv6_literal h) -> Some h
Copy link

Choose a reason for hiding this comment

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

It's not clear to me whether the ipv6 here should be returned surrounded by square brackets. @mseri thinks it should, it would fix an issue in cohttp: mirage/ocaml-cohttp#997

Copy link
Member

Choose a reason for hiding this comment

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

It seems correct, the domain part of email addresses also use brackets to delimit user-defined domains or IPv6 address.

Copy link

@bernhardkaindl bernhardkaindl Sep 20, 2023

Choose a reason for hiding this comment

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

Yes, @dinosaure: I think you refer to @mseri's proposal to fix it in ocalm-uri as correct?: As psafont mentioned above, @mseri proposed that it should be fixed here in the uri package in his comment mirage/ocaml-cohttp#997 (comment):

I think this should be fixed in the uri package, then all dependent libraries would benefit directly from the fix.

@anmonteiro, can you have a look and update this PR to fix mirage/ocaml-cohttp#997?

Here is the link to the code location in the full review diff:
https://github.com/mirage/ocaml-uri/pull/169/files#r1330212509

I don't know Ocaml, but there is similar code (just with host instead of h) just in the previous diff part above: Maybe, that can be used as a starting point for updating this function:

  |Some (`Ipv4_literal host) -> Buffer.add_string buf host
  |Some (`Ipv6_literal host) ->
      Buffer.add_char buf '[';
      Buffer.add_string buf host;
      Buffer.add_char buf ']'
  );

@dinosaure
Copy link
Member

It seems that this PR is highly required but it breaks the uri behavior. From the RFC point-of-view, the fix seems right as explained in #169. I need to convince myself more to merge & cut a release due to the implication that this PR can have on the OCaml ecosystem. However, I consider to merge it as soon as I can.

@dinosaure dinosaure self-assigned this Sep 20, 2023
lib/uri.ml Outdated Show resolved Hide resolved
@dinosaure dinosaure merged commit 1f1d31e into mirage:master Sep 25, 2023
4 checks passed
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Sep 25, 2023
CHANGES:

* **breaking change** Fix parsing & printing of IPv6 addresses in the host part of an uri

  If we follow the RFC3986 correctly, IPv6 must be surrounded by '[' and ']'. Old versions
  of `ocaml-uri` escaped these characters. The new version interprets these characters to
  recognize an IPv6 address.

  Users should take note of this change in behaviour, which fixes a number of bugs in HTTP
  requests. (@anmonteiro, review by several maintainers, mirage/ocaml-uri#169)
* Upgrade tests to `ounit2` (@Alessandro-Barbieri, mirage/ocaml-uri#161)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* **breaking change** Fix parsing & printing of IPv6 addresses in the host part of an uri

  If we follow the RFC3986 correctly, IPv6 must be surrounded by '[' and ']'. Old versions
  of `ocaml-uri` escaped these characters. The new version interprets these characters to
  recognize an IPv6 address.

  Users should take note of this change in behaviour, which fixes a number of bugs in HTTP
  requests. (@anmonteiro, review by several maintainers, mirage/ocaml-uri#169)
* Upgrade tests to `ounit2` (@Alessandro-Barbieri, mirage/ocaml-uri#161)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPv6 addresses in square brackets are parsed incorrectly
9 participants