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

Add failing test for relative url starting with // #168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reynir
Copy link
Member

@reynir reynir commented Jan 27, 2023

See #167.

The naming could be better.

I'm not sure you can present this URL as a string, so maybe Uri.to_string should raise Invalid_argument _ or similar?

@dinosaure
Copy link
Member

This PR was rebased. However, it includes a test which fails (reported by @aantron and integrated by @reynir). Not sure how to deal with that when it will (again, if we count #169) break the uri behavior. Let's stay stingy about releases and post-pone this PR to a next release.

@dinosaure
Copy link
Member

A possible fix from @reynir:

diff --git a/lib/uri.ml b/lib/uri.ml
index 7b5af34..a41c3f6 100644
--- a/lib/uri.ml
+++ b/lib/uri.ml
@@ -670,6 +670,10 @@ let to_string ?(pct_encoder=pct_encoder ()) uri =
   );
   (match uri.path with (* Handle relative paths correctly *)
   | [] -> ()
+  | "/" :: "/" :: _ when
+      Option.is_none uri.scheme &&
+      Option.is_none uri.userinfo && Option.is_none uri.host && Option.is_none uri.port ->
+    raise (Invalid_argument "relative URI with leading double slash in path")
   | "/"::_ ->
     Buffer.add_string buf (Pct.uncast_encoded
                               (encoded_of_path ?scheme ~component:pct_encoder.path uri.path))
diff --git a/lib_test/test_runner.ml b/lib_test/test_runner.ml
index 29ed1ba..97ee461 100644
--- a/lib_test/test_runner.ml
+++ b/lib_test/test_runner.ml
@@ -684,9 +684,13 @@ let test_make_path_rel_identity =
   let path = "//aaa/bbb" in
   sprintf "of_string (to_string _) identity:%s" path >:: fun () ->
     let u = Uri.make ~path () in
-    let u' = Uri.of_string (Uri.to_string u) in
-    assert_equal ~printer:Uri.to_string ~cmp:Uri.equal
-      u u'
+    match Uri.to_string u with
+    | u_str ->
+      let u' = Uri.of_string u_str in
+      assert_equal ~printer:Uri.to_string ~cmp:Uri.equal
+        u u'
+    | exception Invalid_argument _ ->
+      ()
 
 let compat_uris =
   [ "http://\nhost"

@reynir
Copy link
Member Author

reynir commented Sep 25, 2023

It seems a quirk of rfc3986 relative references is that paths starting with an empty segment (//...) are not representable. The above tries to solve this by raising an exception if such a URI is attempted serialized as a string. With the goal of this library in mind maybe it should not be possible to pass a path with double slash to Uri.make without an authority instead of raising in Uri.to_string.

In any case it's an annoying corner case.

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.

2 participants