-
Notifications
You must be signed in to change notification settings - Fork 325
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
Normalize URL paths: convert /.//p, /..//p, and //p to p #943
base: main
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix.
I think you're on the right track, but I'd like the fix to be more rooted in the spec.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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