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

Loose base URL check can crash URL parser #204

Closed
rubycon opened this issue Jan 14, 2024 · 5 comments · Fixed by #213
Closed

Loose base URL check can crash URL parser #204

rubycon opened this issue Jan 14, 2024 · 5 comments · Fixed by #213

Comments

@rubycon
Copy link
Contributor

rubycon commented Jan 14, 2024

What is the issue with the URL Pattern Standard?

According to the current spec, when initialize is called with a string input and a null baseURL, the URL parser behavior can be unexpected.

new URLPattern("https://example.com:8080/foo?bar#baz");

With a string input, init["baseURL"] is always set to the value of baseURL (null in our case). Cf. 3. Set init["baseURL"] to baseURL.

The init object is then passed to process a URLPatternInit and the steps will branch to 11. If init["baseURL"] exists: with a null value. URL spec's internal parser is then called with an unexpected null value (string expected) and will likely fail in someway depending on the implementation.

This issue is similar to #202 as both issues come from calling the URL spec's internal parser without properly checking the input arguments or the return value.

I also think that using map exists to check if values are properly set is maybe not the most robust practice as map exists can be true even if the value associated with the key is null or undefined.

Maybe the base URL contains/exists checks in process a URLPatternInit could be replace with more explicit type check (non empty string, integer or path list). It could avoid unnecessary processing, unfiltered call to the URL parser and maybe fix some non-intuitive behaviors (cf. #200).

@sisidovski
Copy link
Collaborator

I agree, 11. If init["baseURL"] exists: in process a URLPatternInit should also check if it's a non-empty string.

@domenic
Copy link
Member

domenic commented Jan 15, 2024

I think the bug here is a bit more subtle. It should not be possible to set init["baseURL"] to null because its type is USVString, not USVString?. But I guess the spec does that in "initialize" step 2.3.

So we should add a guard to step 2.3 at least.

We may also need an empty string guard. I am less sure about that.

I also think that using map exists to check if values are properly set is maybe not the most robust practice as map exists can be true even if the value associated with the key is null or undefined.

This is not quite right in our case. Because the dictionary member is defined to be of type USVString, it cannot hold undefined or null values. So "map exists" is good enough and no more checks should be necessary.

@sisidovski
Copy link
Collaborator

I think the bug here is a bit more subtle. It should not be possible to set init["baseURL"] to null because its type is USVString, not USVString?. But I guess the spec does that in "initialize" step 2.3.

I overlooked the type. Yes, it's USVString so it can't be null. Thanks.

How about adding an empty string guard to between step 2.2 and 2.3 and throwing a TypeError?

@rubycon
Copy link
Contributor Author

rubycon commented Jan 15, 2024

You're right @domenic, type already should be insured. However there's other places where null values are explicitely set URLPatternInit entries.

During initialization process a URLPatternInit is called with all url parts argument to null ! The function allow null to be passed for these arguments but they are only use to fill an URLPatternInit struct.

@jeremyroman
Copy link
Collaborator

I don't follow the argument for a non-empty string guard. Of course this is going to fail ultimately, but it seems fine for it to do so in "process a URLPatternInit" 11.2 (after we fail to parse the empty string as a URL).

jeremyroman added a commit to jeremyroman/urlpattern that referenced this issue Jan 26, 2024
…LPattern"

It is not valid for the baseURL dictionary member to be null, only
either absent or a USVString. Instead, this should be omitted from the
dictionary altogether if no string was provided to this algorithm.

Any string which is invalid will fail later on, when it is to be parsed
as a URL.

Fixes whatwg#204.
jeremyroman added a commit that referenced this issue Jan 29, 2024
…LPattern"

It is not valid for the baseURL dictionary member to be null, only
either absent or a USVString. Instead, this should be omitted from the
dictionary altogether if no string was provided to this algorithm.

Any string which is invalid will fail later on, when it is to be parsed
as a URL.

Fixes #204.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants