Intent to Ship: Port overflow check in URL setters

[Jiacheng Guo]

Contact emails

g...@google.com

Explainer

This is an implementation of an established standard.

Specification

https://url.spec.whatwg.org/#dom-url-port

Summary

The port value will be checked when setting url.port. All the values that overflows the 16-bit numeric limit will be no longer valid. For instance the following script behave differently after the change: ``` u = new URL("http://test.com"); u.port = 65536; console.log(u.port); ``` Before the change the output is 65536. After the change the output will be 80.

Blink component

Blink>JavaScript>API

TAG review

TAG review status

Not applicable

Risks

Interoperability and Compatibility

Gecko: Shipped/Shipping

WebKit: Shipped/Shipping

Web developers: No signals

Other signals:

WebView application risks

No signals

Debuggability

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes

Is this feature fully tested by web-platform-tests?

Yes

Flag name

URLSetPortCheckOverflow

Requires code in //chrome?

False

Tracking bug

https://crbug.com/1416017

Estimated milestones

No milestones specified

Anticipated spec changes

Open questions about a feature may be a source of future web compat or interop issues. Please list open issues (e.g. links to known github issues in the project for the feature specification) whose resolution may introduce web compat/interop risk (e.g., changing to naming or structure of the API in a non-backward-compatible way).

Link to entry on the Chrome Platform Status

https://chromestatus.com/feature/5097311074516992

This intent message was generated by Chrome Platform Status.

[Jiacheng Guo]

The implementation can be found at https://chromium-review.googlesource.com/c/chromium/src/+/4252309

[Yoav Weiss]

On Wed, Feb 22, 2023 at 2:23 PM 'Jiacheng Guo' via blink-dev <blin...@chromium.org> wrote:

The implementation can be found at https://chromium-review.googlesource.com/c/chromium/src/+/4252309

On Wed, Feb 22, 2023 at 9:39 PM Jiacheng Guo <g...@google.com> wrote:

Contact emails

g...@google.com

Explainer

This is an implementation of an established standard.

An explainer (even inline) helps to understand what change you're trying to ship, regardless of its spec status.

At the same time, the explanation you included in your summary does that.

 

Specification

https://url.spec.whatwg.org/#dom-url-port

Summary

The port value will be checked when setting url.port. All the values that overflows the 16-bit numeric limit will be no longer valid. For instance the following script behave differently after the change: ``` u = new URL("http://test.com"); u.port = 65536; console.log(u.port); ``` Before the change the output is 65536. After the change the output will be 80.

Do we have a usecounter for this?

Do I understand correctly and the current behavior would cause requests that are based on such URL values to fail, given that the port number exceeds what's permitted on the network protocol?

If that's the case, this change could cause such requests to "succeed" even though they are sent to a different origin than what the developer had in mind.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJQw1NyQV7zuDODbvobJK%2BK_WeSX%2B-Bq5S80RAQqQZmy2NZxqw%40mail.gmail.com.

[Jiacheng Guo]

We don't currently have a use counter for it.

Does it make sense to add the port overflow check under a flag and a usecounter as well to record the frequency of setting URL ports with an overflow value.

We can collect data first and then gradually enable the feature based on the data we collected.

[Yoav Weiss]

On Wed, Feb 22, 2023 at 3:32 PM Jiacheng Guo <g...@google.com> wrote:

We don't currently have a use counter for it.

Does it make sense to add the port overflow check under a flag and a usecounter as well to record the frequency of setting URL ports with an overflow value.

I think it would make sense. If there's time pressure, we may not even have to wait till the usecounter hits stable and use the internal UMA equivalent to get a read of what current impacted usage is like. With any luck, it'd be close to 0, and we'd be able to go ahead. At the same time, having those usage numbers would help reassure us that breakage is not significant.

[Rick Byers]

If this only ever caused connections that previously failed to now succeed as they do in other browsers, then the risk of it causing a compat issue is exceedingly low right? Perhaps this is more of a bugfix than a breaking API change?

I see the implementation is already behind a base::Feature. One option would be to add a UseCounter and turn the feature on to 100% via finch. Then we can check the UseCounter data in beta and turn the finch flag off if it's surprisingly high. Thoughts?

Rick

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfXNj3LDqQ0H%3DUE_p3zML%3DTRksMzb%3Dtc25EmtkaaCPNa3w%40mail.gmail.com.

[Yoav Weiss]

Adding the usecounter and optimistically defaulting to enable it (while keeping an eye on the numbers) would also work for me.

[Yoav Weiss]

LGTM1 to optimistically add a usecounter and enable (with a base feature that'd enable to disable this if needed)

[Daniel Bratell]

LGTM2

/Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfUcjZ_9nLbvUX4Dg12d6RYaWd05FjsqokKtBZfe%3DJohsw%40mail.gmail.com.

[slightlyoff via Chromestatus]

LGTM3