Repository: servo/rust-url
Pull request #817 (2023-02-28 11:08:40)

Fix anarchist URL where path starts with //
This PR closes https://github.com/servo/rust-url/issues/799 by refusing to parse URLs with no authority whose normalized path would start with a double-slash.

---

The issue comes from the [remove dot segments](https://www.rfc-editor.org/rfc/rfc3986#section-5.2.4) step. Let's consider the URI `m:/.//`. Then, according to the RFC:

> B.  if the input buffer begins with a prefix of "/./" […], then replace that prefix with "/"

So it should be normalized to `m://`, but this has different semantics (resulting in `\` being interpreted as being part of the authority in the original example).

I have conducted a few tests with some URI normalization libraries:

```
$ node test.js
http://example.com/a/c/d
m:/.//
$ cat test.php
<?php
require_once 'URLNormalizer.php';
echo (new Normalizer('http://example.com/a/b/../c/d'))->normalize(), "\n";
echo (new Normalizer('m:/.//'))->normalize(), "\n";
?>
$ php test.php
http://example.com/a/c/d
m:/
$ cat test.pl
use feature qw(say);
use URI::Normalize qw(normalize_uri);
say normalize_uri(URI->new('http://example.com/a/b/../c/d'));
say normalize_uri(URI->new('m:/.//'));
$ perl test.pl
http://example.com/a/c/d
m://
$ cat test.rb
require 'addressable/uri'
p Addressable::URI.parse('http://example.com/a/b/../c/d').normalize.to_s
p Addressable::URI.parse('m:/.//').normalize.to_s
$ ruby test.rb
"http://example.com/a/c/d"
/usr/share/rubygems-integration/all/gems/addressable-2.8.1/lib/addressable/uri.rb:2487:in `validate': Cannot have a path with two leading slashes without an authority set: 'm://' (Addressable::URI::InvalidURIError)
	from /usr/share/rubygems-integration/all/gems/addressable-2.8.1/lib/addressable/uri.rb:2410:in `defer_validation'
	from /usr/share/rubygems-integration/all/gems/addressable-2.8.1/lib/addressable/uri.rb:839:in `initialize'
	from /usr/share/rubygems-integration/all/gems/addressable-2.8.1/lib/addressable/uri.rb:2184:in `new'
	from /usr/share/rubygems-integration/all/gems/addressable-2.8.1/lib/addressable/uri.rb:2184:in `normalize'
	from test.rb:3:in `<main>'
```

For PHP, I am using https://github.com/glenscott/url-normalizer. In short:

- Node does not remove the dot before a double-slash; I think the logic is [somewhere in there](https://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#1815), but I never liked browsing Firefox's source code, so who knows?
- PHP removes the dot but avoids leaving a double-slash where it would be reinterpreted as the authority; this is done by [merging consecutive slashes](https://github.com/glenscott/url-normalizer/blob/master/src/URL/Normalizer.php#L262), which is does not conform to the RFC
- Perl exhibits the same bug as `rust-url`
- Ruby straight up refuses to parse a path with two leading slashes without an authority

I feel like Ruby's is the most consistent and straightforward solution.

This does mean that the [`no_panic` test](https://github.com/servo/rust-url/blob/master/url/tests/unit.rs#L951) from https://github.com/servo/rust-url/issues/654 must be amended. However, we can cover the `m:/.//` case in a dedicated unit test.

valenting: You reference an RFC. This crate explicitly only implements the [URL Standard](https://url.spec.whatwg.org/).
According to the reference URL parser `m:/.//\\` should [parse successfully](https://jsdom.github.io/whatwg-url/#url=bTovLi8vXFw=&base=YWJvdXQ6Ymxhbms=)
qsantos: My bad. Thanks for the proper reference. Then, it looks like this comes from the serialization part, more specifically, from [section 4.5](https://url.spec.whatwg.org/#url-serializing), item 3, whose note explicitly mentions the case we are discussing. I have correspondingly moved the correction to the serialization.
qsantos: Rebased to take into account the new tests
codecov[bot]: # [Codecov](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) Report
Patch coverage: **`93.33`**% and project coverage change: **`+0.02`** :tada:
> Comparison is base [(`edeaea7`)](https://codecov.io/gh/servo/rust-url/commit/edeaea7f1e8bd08148bcd535f921fbc645bce53e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) 82.74% compared to head [(`b0ba159`)](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) 82.76%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #817      +/-   ##
==========================================
+ Coverage   82.74%   82.76%   +0.02%     
==========================================
  Files          20       20              
  Lines        3303     3313      +10     
==========================================
+ Hits         2733     2742       +9     
- Misses        570      571       +1     
```


| [Impacted Files](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) | Coverage Δ | |
|---|---|---|
| [url/src/slicing.rs](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-dXJsL3NyYy9zbGljaW5nLnJz) | `91.93% <80.00%> (-1.17%)` | :arrow_down: |
| [url/src/lib.rs](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-dXJsL3NyYy9saWIucnM=) | `76.47% <100.00%> (+0.12%)` | :arrow_up: |
| [url/src/parser.rs](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-dXJsL3NyYy9wYXJzZXIucnM=) | `81.48% <100.00%> (+0.16%)` | :arrow_up: |
| [data-url/tests/wpt.rs](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-ZGF0YS11cmwvdGVzdHMvd3B0LnJz) | `84.41% <0.00%> (-1.30%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/servo/rust-url/pull/817?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo).

qsantos: I have moved the handling of empty fragment to `with_query_and_fragment`. That way, it should be able to normalize URLs whether they are a fresh parse, or a combination of an existing URL and a relative path.

Patch:
diff --git a/url/src/lib.rs b/url/src/lib.rs
index 0be004e..7624b6f 100644
--- a/url/src/lib.rs
+++ b/url/src/lib.rs
@@ -683,7 +683,14 @@ impl Url {
             assert_eq!(self.host_end, self.scheme_end + 1);
             assert_eq!(self.host, HostInternal::None);
             assert_eq!(self.port, None);
-            assert_eq!(self.path_start, self.scheme_end + 1);
+            if self.path().starts_with("//") {
+                // special case when first path segment is empty
+                assert_eq!(self.byte_at(self.scheme_end + 1), b'/');
+                assert_eq!(self.byte_at(self.scheme_end + 2), b'.');
+                assert_eq!(self.path_start, self.scheme_end + 3);
+            } else {
+                assert_eq!(self.path_start, self.scheme_end + 1);
+            }
         }
         if let Some(start) = self.query_start {
             assert!(start >= self.path_start);
diff --git a/url/src/parser.rs b/url/src/parser.rs
index 458d3a9..ca0865c 100644
--- a/url/src/parser.rs
+++ b/url/src/parser.rs
@@ -474,9 +474,8 @@ impl<'a> Parser<'a> {
         let host = HostInternal::None;
         let port = None;
         let remaining = if let Some(input) = input.split_prefix('/') {
-            let path_start = self.serialization.len();
             self.serialization.push('/');
-            self.parse_path(scheme_type, &mut false, path_start, input)
+            self.parse_path(scheme_type, &mut false, path_start as usize, input)
         } else {
             self.parse_cannot_be_a_base_path(input)
         };
@@ -1354,9 +1353,50 @@ impl<'a> Parser<'a> {
         host_end: u32,
         host: HostInternal,
         port: Option<u16>,
-        path_start: u32,
+        mut path_start: u32,
         remaining: Input<'_>,
     ) -> ParseResult<Url> {
+        // Special case for anarchist URL's with a leading empty path segment
+        // This prevents web+demo:/.//not-a-host/ or web+demo:/path/..//not-a-host/,
+        // when parsed and then serialized, from ending up as web+demo://not-a-host/
+        // (they end up as web+demo:/.//not-a-host/).
+        //
+        // If url’s host is null, url does not have an opaque path,
+        // url’s path’s size is greater than 1, and url’s path[0] is the empty string,
+        // then append U+002F (/) followed by U+002E (.) to output.
+        let scheme_end_as_usize = scheme_end as usize;
+        let path_start_as_usize = path_start as usize;
+        if path_start_as_usize == scheme_end_as_usize + 1 {
+            // Anarchist URL
+            if self.serialization[path_start_as_usize..].starts_with("//") {
+                // Case 1: The base URL did not have an empty path segment, but the resulting one does
+                // Insert the "/." prefix
+                self.serialization.insert_str(path_start_as_usize, "/.");
+                path_start += 2;
+            }
+            assert!(!self.serialization[scheme_end_as_usize..].starts_with("://"));
+        } else if path_start_as_usize == scheme_end_as_usize + 3
+            && &self.serialization[scheme_end_as_usize..path_start_as_usize] == ":/."
+        {
+            // Anarchist URL with leading empty path segment
+            // The base URL has a "/." between the host and the path
+            assert_eq!(self.serialization.as_bytes()[path_start_as_usize], b'/');
+            if self
+                .serialization
+                .as_bytes()
+                .get(path_start_as_usize + 1)
+                .copied()
+                != Some(b'/')
+            {
+                // Case 2: The base URL had an empty path segment, but the resulting one does not
+                // Remove the "/." prefix
+                self.serialization
+                    .replace_range(scheme_end_as_usize..path_start_as_usize, ":");
+                path_start -= 2;
+            }
+            assert!(!self.serialization[scheme_end_as_usize..].starts_with("://"));
+        }
+
         let (query_start, fragment_start) =
             self.parse_query_and_fragment(scheme_type, scheme_end, remaining)?;
         Ok(Url {
diff --git a/url/src/slicing.rs b/url/src/slicing.rs
index a90337b..c2e441e 100644
--- a/url/src/slicing.rs
+++ b/url/src/slicing.rs
@@ -149,7 +149,14 @@ impl Url {
                 }
             }
 
-            Position::AfterPort => self.path_start as usize,
+            Position::AfterPort => {
+                if let Some(port) = self.port {
+                    debug_assert!(self.byte_at(self.host_end) == b':');
+                    self.host_end as usize + ":".len() + port.to_string().len()
+                } else {
+                    self.host_end as usize
+                }
+            }
 
             Position::BeforePath => self.path_start as usize,
 


Test patch:
diff --git a/url/tests/unit.rs b/url/tests/unit.rs
index d27016b..5affa16 100644
--- a/url/tests/unit.rs
+++ b/url/tests/unit.rs
@@ -954,6 +954,16 @@ fn no_panic() {
     url::quirks::set_hostname(&mut url, "//eom/datcom/\\\\t\\://eom/data.cs").unwrap();
 }
 
+#[test]
+fn test_null_host_with_leading_empty_path_segment() {
+    // since Note in item 3 of URL serializing in the URL Standard
+    // https://url.spec.whatwg.org/#url-serializing
+    let url = Url::parse("m:/.//\\").unwrap();
+    let encoded = url.as_str();
+    let reparsed = Url::parse(encoded).unwrap();
+    assert_eq!(reparsed, url);
+}
+
 #[test]
 fn pop_if_empty_in_bounds() {
     let mut url = Url::parse("m://").unwrap();
diff --git a/url/tests/urltestdata.json b/url/tests/urltestdata.json
index e440be2..53d0368 100644
--- a/url/tests/urltestdata.json
+++ b/url/tests/urltestdata.json
@@ -7487,7 +7487,6 @@
     "hash": ""
   },
   "Serialize /. in path",
-  "skip next",
   {
     "input": "non-spec:/.//",
     "base": "about:blank",
@@ -7502,7 +7501,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "non-spec:/..//",
     "base": "about:blank",
@@ -7517,7 +7515,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "non-spec:/a/..//",
     "base": "about:blank",
@@ -7532,7 +7529,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "non-spec:/.//path",
     "base": "about:blank",
@@ -7547,7 +7543,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "non-spec:/..//path",
     "base": "about:blank",
@@ -7562,7 +7557,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "non-spec:/a/..//path",
     "base": "about:blank",
@@ -7592,7 +7586,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "/..//path",
     "base": "non-spec:/p",
@@ -7607,7 +7600,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "..//path",
     "base": "non-spec:/p",
@@ -7622,7 +7614,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "a/..//path",
     "base": "non-spec:/p",
@@ -7637,7 +7628,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "",
     "base": "non-spec:/..//p",
@@ -7652,7 +7642,6 @@
     "search": "",
     "hash": ""
   },
-  "skip next",
   {
     "input": "path",
     "base": "non-spec:/..//p",