1338154 - Regression: Resource URLs whose first path ("host") component consists entirely of numbers no longer work

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mike Kaply [:mkaply]]

Regression from bug 1288049.

In the scratch pad enter:

new URL("resource://123/").href

URL is:

resource://0.0.0.123/

It should have stayed as resource://123

This is breaking extensions that happen to use numbers only in their chrome.manfiest identifiers

Comment 1

[Dragana Damjanovic [:dragana]]

Please take a look.

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression affecting add-ons and/or partners.

Comment 3

[Junior [inactive]]

I don't have much knowledge about |resource| protocol.
If I understand correctly, for the case |resource://123|,
the 123 is more like a path or a filename instead of a host.

Maybe we should not use a standard url parser, but I'm not sure.

Stay tuned. I'll keep investigate.

Comment 4

[Junior [inactive]]

resource and chrome protocols are similar to file protocol.
They are not supposed to have a host.

Therefore, we usually use three slashes to indicate no host for a file url,
e.g., |file:///| indicates the root directory of localhost.

Moreover, for resource://path/to/file case, we will see "path" as a host instead of path,
which satisfies the spec. But the behavior is not what we want.

Hello Valentin,
Do you have any suggestion for fixing this?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Do we have an idea how many addons are affected?
I'd say this is probably not the only case where something like this happens. We also normalize IDNA domain names, so might also change.

There are a few solutions we might consider here:
1. Don't normalize all-number-hostnames if scheme is "resource". Hacky, but in the end would work.
2. Use SimpleURI for parsing resource URLs. Doesn't have the concept of Host, so there is a possibility of more regressions.
3. Enforce that addons don't use all-number hostnames

Comment 6

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #5)
> Do we have an idea how many addons are affected?
> I'd say this is probably not the only case where something like this
> happens. We also normalize IDNA domain names, so might also change.
> 
> There are a few solutions we might consider here:
> 1. Don't normalize all-number-hostnames if scheme is "resource". Hacky, but
> in the end would work.
> 2. Use SimpleURI for parsing resource URLs. Doesn't have the concept of
> Host, so there is a possibility of more regressions.

When I looked at the implementation here, I thought that the resource URI implementation already subclasses nsStandardURL - is there no way to subclass SimpleURI instead and still provide a 'host' concept, just without the implied semantics (for IP addresses / IDNA) that nsStandardURL uses, and would that reduce the risk of other regressions?

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs from comment #6)
> (In reply to Valentin Gosu [:valentin] from comment #5)
> > Do we have an idea how many addons are affected?
> > I'd say this is probably not the only case where something like this
> > happens. We also normalize IDNA domain names, so might also change.
> > 
> > There are a few solutions we might consider here:
> > 1. Don't normalize all-number-hostnames if scheme is "resource". Hacky, but
> > in the end would work.
> > 2. Use SimpleURI for parsing resource URLs. Doesn't have the concept of
> > Host, so there is a possibility of more regressions.
> 
> When I looked at the implementation here, I thought that the resource URI
> implementation already subclasses nsStandardURL - is there no way to
> subclass SimpleURI instead and still provide a 'host' concept, just without
> the implied semantics (for IP addresses / IDNA) that nsStandardURL uses, and
> would that reduce the risk of other regressions?

Both of these are basically hacks. Changing nsStandardURL to do this is way simpler than extending nsSimpleURI and changing its behaviour. I favor the first one.

Comment 8

[Junior [inactive]]

Attachment: preventCanonicalResIPv4

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8836649 [details] [diff] [review]
preventCanonicalResIPv4

Review of attachment 8836649 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.
Please also update nsStandardURL::SetHost to do the same, and add a test for that too. Thanks!

::: netwerk/base/nsStandardURL.cpp
@@ +2476,5 @@
>          }  
>      } else {
>          // add some flags to coalesceFlag if it is an ftp-url
>          // need this later on when coalescing the resulting URL
> +        if (SegmentIs(mScheme, "ftp")) {

nit: whitespace only change

Comment 10

[Junior [inactive]]

Attachment: preventCanonicalResIPv4 - V2

modified by last comment and carry r+

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=872ef842c3e1364e62f4baeb39fa0f25f8ae1a0e

Comment 11

[Julien Cristau [:jcristau]]

Tracking for 52 as recent regression.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17853e696113
not normalize ipv4 for resource and chrome uri. r=valentin

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/17853e696113

Comment 14

[Junior [inactive]]

Do you think if we need an uplift to beta/aurora?

Comment 15

[:Gijs (he/him)]

(In reply to Junior[:junior] from comment #14)
> Do you think if we need an uplift to beta/aurora?

Beta is 52 which is also going to be next esr. Given Mike reported this I suspect it affects enterprise users. If the patch is simple enough (which from a quick look it looks like it probably is) and has decent test coverage (which it does), uplift might not be a bad idea. Leaving ni for mkaply to confirm.

Comment 16

[Mike Kaply [:mkaply]]

Yes, I would like this uplifted please. As Gijs pointed out, this did affect enterprise so I am concerned more people would hit it on 52. Thanks.

Comment 17

[Junior [inactive]]

Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

Approval Request Comment
[Feature/Bug causing the regression]: bug 1288049
[User impact if declined]: pure numbers alias of resource URI from extension don't work. affect on enterprise users.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: verified in unit test and mochitest
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: small changes for not normalizing IPv4 for resource uri, which isn't with concept of IPv4/host
[String changes made/needed]: no

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

Fix a regression using pure numbers alias of resource URI from the extension. Aurora53+.

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

fix resource/chrome URLs with numeric "host", beta52+

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6db1cdc5eedd

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7703bdf63084

Comment 22

[Junior [inactive]]

Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: want the enterprise users not suffering from non-working extensions
User impact if declined: pure numbers alias of resource URI from extension don't work. affect on enterprise users.
Fix Landed on Version: 54, also uplift to 52, 53
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: no

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 23

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Junior's assessment on manual testing needs (Comment 17) and the fact that this fix has automated coverage.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

esr52 gets synced from beta, no need for separate approval process.

Comment 25

[Junior [inactive]]

(In reply to Julien Cristau [:jcristau] from comment #24)
> Comment on attachment 8836654 [details] [diff] [review]
> preventCanonicalResIPv4 - V2
> 
> esr52 gets synced from beta, no need for separate approval process.

Got it. Thanks.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7703bdf63084