Loose base URL check can crash URL parser

[rubycon]

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 https://github.com/whatwg/urlpattern/issues/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. https://github.com/whatwg/urlpattern/issues/200).

[sisidovski]

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

[domenic]

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]

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]

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]

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).