Fix anarchist URL where path starts with // #817

qsantos · 2023-02-28T11:08:40Z

This PR closes #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 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, 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, 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 from #654 must be amended. However, we can cover the m:/.// case in a dedicated unit test.

valenting · 2023-02-28T21:28:27Z

You reference an RFC. This crate explicitly only implements the URL Standard.
According to the reference URL parser m:/.//\\ should parse successfully

qsantos · 2023-03-01T20:51:40Z

My bad. Thanks for the proper reference. Then, it looks like this comes from the serialization part, more specifically, from section 4.5, item 3, whose note explicitly mentions the case we are discussing. I have correspondingly moved the correction to the serialization.

qsantos · 2023-03-02T10:27:40Z

Rebased to take into account the new tests

codecov · 2023-03-02T11:32:35Z

Codecov Report

Patch coverage: 83.87% and project coverage change: -0.02 ⚠️

Comparison is base (edeaea7) 82.74% compared to head (fd29f5f) 82.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #817      +/-   ##
==========================================
- Coverage   82.74%   82.73%   -0.02%     
==========================================
  Files          20       20              
  Lines        3303     3330      +27     
==========================================
+ Hits         2733     2755      +22     
- Misses        570      575       +5

Impacted Files	Coverage Δ
url/src/slicing.rs	`91.93% <80.00%> (-1.17%)`	⬇️
url/src/parser.rs	`81.38% <80.95%> (+0.06%)`	⬆️
url/src/lib.rs	`76.47% <100.00%> (+0.12%)`	⬆️
data-url/tests/wpt.rs	`84.41% <0.00%> (-1.30%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

valenting · 2023-03-02T11:53:58Z

url/tests/urltestdata.json

@@ -7668,6 +7660,7 @@
    "hash": ""
  },
  "Do not serialize /. in path",
+  "skip next",


I think we need to also explicitly handle this part here

qsantos · 2023-03-04T14:45:24Z

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.

valenting

Great progress, thank for working on this. I only have a few small issues.
Also, it would be nice to add some unit tests to improve code coverage.

valenting · 2023-03-04T22:12:22Z

url/src/parser.rs

+        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 fragment, but the resulting one does


fragment -> something else.

Ah yes, thanks, I meant “path segment”. I have rebased for a less confusing history with the proper terminology.

valenting · 2023-03-04T22:15:18Z

url/src/parser.rs

+                    .as_bytes()
+                    .get(path_start_as_usize)
+                    .copied(),
+                Some(b'/')


This seems overly verbose. self.serialization.as_bytes()[path_start_as_usize] == b'/' should work?

Absolutely! I edited this code quite a bit, and did not notice that the .get() was not necessary anymore here. Thanks!

valenting · 2023-03-04T22:17:41Z

url/src/parser.rs

        remaining: Input<'_>,
    ) -> ParseResult<Url> {
+        // Special case for anarchist URL's with a leading empty fragment


fragment has a special meaning in the context of a URL - it's the bit after the # sign. Let's use another word here instead.

valenting · 2023-03-04T22:18:46Z

url/src/parser.rs

+        } 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 fragment


fragment -> something else.

valenting · 2023-03-04T22:18:56Z

url/src/parser.rs

+                .copied()
+                != Some(b'/')
+            {
+                // Case 2: The base URL had an empty fragment, but the resulting one does not


fragment -> something else.

valenting

Great stuff. Thank you!

qsantos · 2023-03-05T15:53:53Z

Thanks for the reactivity and the detailed comments!

klensy · 2023-03-05T23:50:57Z

url/src/slicing.rs

+            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()


Oww, this can be done without allocating string.

qsantos mentioned this pull request Feb 28, 2023

Unable to encode + decode URL #799

Closed

qsantos force-pushed the master branch 2 times, most recently from 1479c09 to 4b202b1 Compare March 2, 2023 10:27

qsantos force-pushed the master branch 2 times, most recently from 724bdf1 to b0ba159 Compare March 2, 2023 10:52

valenting reviewed Mar 2, 2023

View reviewed changes

valenting requested changes Mar 4, 2023

View reviewed changes

qsantos added 8 commits March 5, 2023 09:17

Handle null host with leading empty path segment

bb1e1b8

Disable failing test

607cdfc

Fix invariants of anarchist URLs + empty segment

9fe1375

Handle empty leading segment in relative paths

f95323c

Handle case where input starts with a slash

ed22de8

Unify handling of leading empty path segment

4f2143a

Avoid casting back and forth between u32 and usize

5a1f7fc

Fix use of .get() that should never fail

fd29f5f

qsantos force-pushed the master branch from 0f80937 to fd29f5f Compare March 5, 2023 08:52

valenting approved these changes Mar 5, 2023

View reviewed changes

valenting merged commit 74b8694 into servo:master Mar 5, 2023

klensy reviewed Mar 5, 2023

View reviewed changes

This was referenced Mar 6, 2023

Avoid string allocation to get length of port #823

Merged

Panic on setting hostname for non-spec:/.//p URL #700

Closed

Url::as_str() cannot be parsed back into Url #661

Closed

This was referenced Mar 19, 2023

Round-trip parsing for a URL string results in different outcomes #391

Closed

URLs containing no host and a path starting with ./ or ../ are not serialized idempotently #601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix anarchist URL where path starts with // #817

Fix anarchist URL where path starts with // #817

qsantos commented Feb 28, 2023

valenting commented Feb 28, 2023

qsantos commented Mar 1, 2023

qsantos commented Mar 2, 2023

codecov bot commented Mar 2, 2023 •

edited

Loading

valenting Mar 2, 2023

qsantos commented Mar 4, 2023

valenting left a comment

valenting Mar 4, 2023

qsantos Mar 5, 2023

valenting Mar 4, 2023

qsantos Mar 5, 2023

valenting Mar 4, 2023

qsantos Mar 5, 2023

valenting Mar 4, 2023

qsantos Mar 5, 2023

valenting Mar 4, 2023

valenting left a comment

qsantos commented Mar 5, 2023

klensy Mar 5, 2023

qsantos Mar 6, 2023

Fix anarchist URL where path starts with // #817

Fix anarchist URL where path starts with // #817

Conversation

qsantos commented Feb 28, 2023

valenting commented Feb 28, 2023

qsantos commented Mar 1, 2023

qsantos commented Mar 2, 2023

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

qsantos commented Mar 4, 2023

valenting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valenting left a comment

Choose a reason for hiding this comment

qsantos commented Mar 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 2, 2023 •

edited

Loading