Fix GH-19944: validate Uri\WhatWg\Url::withPort() boundaries

[alexandre-daubois]

No description provided.

[TimWolla]

I did not yet read the spec to see if the reported bug is even a bug. But I can say for certain that this this patch is incorrect: Lexbor would be responsible to perform the validation and report the issue. Not PHP.

PS: The NEWS entry is in the wrong location.

[alexandre-daubois]

Should we really do no validation on PHP side about invalid values? Isn't it better to catch errors as early as possible?

Also I'm not sure why this is the wrong location for the news entry?

[kocsismate]

The spec says the following:

If port is not a 16-bit unsigned integer, port-out-of-range validation error, return failure.

Apparently, the wither call somehow misses the validation. I'll have to debug it why, but the fix must go into the lexbor library indeed. PHP is just a dump wrapper, the logic is better suited in the original upstream library.

[alexandre-daubois]

Alright, I wasn't sure how much it should be "just a wrapper". It's clear now 🙂

[kocsismate]

As far as I see, the error is only present for negative values around this line: https://github.com/lexbor/lexbor/blob/master/source/lexbor/url/url.c#L1908

I think a negative value check was left out.

[TimWolla]

Should we really do no validation on PHP side about invalid values?

In the best case, we are just replicating the validation of the upstream library. In the worst case, we are using differing validation, which will lead to security issues. The point of using a library is that it does the heavy lifting for you.

Also I'm not sure why this is the wrong location for the news entry?

Because RC 1 is already tagged.

Apparently, the wither call somehow misses the validation.

There is some validation happening: The port is not actually changed. So the invalid value is not accepted. This might actually be the expected behavior with regard to the specification. See JavaScript:

u = new URL('https://example.com:432');
URL {origin: 'https://example.com:432', protocol: 'https:', username: '', password: '', host: 'example.com:432', …}
u.port
'432'
u.port = '-1';
'-1'
u.port
'432'
u.port = '99999';
'99999'
u.port
'432'
u.port = '99';
'99'
u.port
'99'

[kocsismate]

I filed an issue upstream with a possible patch: lexbor/lexbor#306

Too large values trigger validation errors, while negative values don't. I think it is only due to an omission, but let's hear Alexander's opinion.