Normalize URL paths: convert /.//p, /..//p, and //p to p #943
Conversation
theskim
force-pushed
the
main
branch
2 times, most recently
from
8497f7e
to
c5c13be
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #943 +/- ##
=======================================
Coverage ? 81.82%
=======================================
Files ? 21
Lines ? 3559
Branches ? 0
=======================================
Hits ? 2912
Misses ? 647
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| @@ -44,7 +44,4 @@ | |||
| <file://monkey/> set pathname to <\\\\> | |||
| <file:///unicorn> set pathname to <//\\/> | |||
| <file:///unicorn> set pathname to <//monkey/..//> | |||
| <non-spec:/> set pathname to </.//p> | |||
| <non-spec:/> set pathname to </..//p> | |||
| <non-spec:/> set pathname to <//p> | |||
| <non-spec:/.//> set pathname to <p> | |||
There was a problem hiding this comment.
I would have expected us to fix this one too.
| // To handle cases like <non-spec:/> set pathname to </..//p> | ||
| // For instance, //p should be converted to /..//p here | ||
| // At this point, we would get "non-spec://p" for serialization | ||
| // and "/..//p" for path |
There was a problem hiding this comment.
Could you rework this comment a bit, to make it clear how this should work
Reference https://url.spec.whatwg.org/#url-serializing step 3 if possible.
| const PATH_INCREMENT: usize = 2; // length of "/." | ||
|
|
||
| if path_pos + PATH_INCREMENT <= path.len() | ||
| && serialization_pos + PATH_INCREMENT <= self.serialization.len() |
There was a problem hiding this comment.
I'm not sure about these conditions.
Wouldn't they always be true?
Moreover, I don't know if path.rfind("//") is the right check.
Step 3 of the serialization says:
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.
Url's path size is greater that 1 and path[0] is empty string basically means path begins with //.
I don't think a rfind should be needed - if you think it is make sure to document it really well.
There was a problem hiding this comment.
This bug might need to be fixed after has_host is implemented, considering the case of:
"postgres://postgres@localhost"
"postgres:/.//postgres@localhost"
https://bugzilla.mozilla.org/show_bug.cgi?id=1874119