Skip to content
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

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

Closed
CYBAI opened this issue Apr 22, 2021 · 5 comments
Closed

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

CYBAI opened this issue Apr 22, 2021 · 5 comments

Comments

@CYBAI
Copy link
Member

CYBAI commented Apr 22, 2021

While trying to investigate servo/servo#28386, I found this test case for hostname setter will crash in Servo

{
    "href": "non-spec:/.//p",
    "new_value": "",
    "expected": {
        "href": "non-spec:////p",
        "host": "",
        "hostname": "",
        "pathname": "//p"
    }
}

I can reproduce this issue in rust-url with adding this test case to the url/tests/setters_tests.json in the hostname section.

---- setters_tests stdout ----
    test: "non-spec:/.//p".hostname = ""
  failed: invariants checked -> "9 != 11 (self.username_end != self.scheme_end + 3) for URL \"non-spec://p\""
@BurgundyWillow
Copy link

Sure. I see there are no fixes yet. I'll work on the issue.

@BurgundyWillow
Copy link

BurgundyWillow commented May 9, 2021

This would need clarification based on the URL specification. Reason being that,

typically double slashes are not allowed if the URL does not contain an authority component. However, the same on entry is still allowed in browsers. The clarification that is needed is how the rust-url should parse this. Should it remove any double slashes it encounters, effectively making:

non-spec:/.//p => non-spec:/p as it would have done for non-spec:/./p

or should it retain a single slash, like non-spec://p, but ends up changing the format of the URL to one having a domain name and path, thereby changing the structure of the URL.

eg. non-spec:/.//localhost//thispath would become non-spec://localhost/thispath to illustrate the quandary.
If the latter is true, then the test case failing is due to the self parsing tests that will fail.

@qsantos
Copy link
Contributor

qsantos commented Mar 8, 2023

The main issue is that the invariants were not kept. According to the URL Standard, the leading /. is ignored while parsing the path (but indicates to the parser that there is no authority). It is added back during serialization. See section 4.5, §3. So, the expected pathname is indeed //p, but the expected href should actually be non-spec:/.//p.

This is what #817 did, so I think this issue is now solved.

@qsantos
Copy link
Contributor

qsantos commented Mar 8, 2023

To be more explicit, adding the test case below to url/tests/setters_tests.json now passes.

        {
            "href": "non-spec:/.//p",
            "new_value": "",
            "expected": {
                "href": "non-spec:/.//p",
                "host": "",
                "hostname": "",
                "pathname": "//p"
            }
        }

@valenting
Copy link
Collaborator

Fixed by #817

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants