mirage / ocaml-uri

RFC3986 URI parsing library for OCaml
Other
97 stars 57 forks source link

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

Open reynir opened 1 year ago

reynir commented 1 year ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.