Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 52

Status

()

Core
Networking
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mkaply, Assigned: junior)

Tracking

({regression})

Trunk
mozilla54
regression
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52+ fixed, firefox53 fixed, firefox54 fixed, firefox-esr52 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 months ago
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
Please take a look.
Flags: needinfo?(juhsu)
Whiteboard: [necko-active]

Comment 2

6 months ago
[Tracking Requested - why for this release]:
Regression affecting add-ons and/or partners.
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
Summary: Regression: Resource URLs containing numbers no longer work → Regression: Resource URLs whose first path ("host") component consists entirely of numbers no longer work
(Assignee)

Comment 3

6 months ago
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.
Flags: needinfo?(juhsu)
(Assignee)

Comment 4

5 months ago
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?
Flags: needinfo?(valentin.gosu)
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
Flags: needinfo?(valentin.gosu)

Comment 6

5 months ago
(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?
(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.
(Assignee)

Comment 8

5 months ago
Created attachment 8836649 [details] [diff] [review]
preventCanonicalResIPv4
Assignee: nobody → juhsu
Attachment #8836649 - Flags: review?(valentin.gosu)
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
Attachment #8836649 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 10

5 months ago
Created attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

modified by last comment and carry r+

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=872ef842c3e1364e62f4baeb39fa0f25f8ae1a0e
Attachment #8836649 - Attachment is obsolete: true
Attachment #8836654 - Flags: review+
Tracking for 52 as recent regression.
status-firefox51: affected → wontfix
tracking-firefox52: ? → +
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 12

5 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17853e696113
not normalize ipv4 for resource and chrome uri. r=valentin
Keywords: checkin-needed

Comment 13

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17853e696113
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 14

5 months ago
Do you think if we need an uplift to beta/aurora?
Flags: needinfo?(mozilla)

Comment 15

5 months ago
(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.
(Reporter)

Comment 16

5 months ago
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.
Flags: needinfo?(mozilla)
(Assignee)

Comment 17

5 months ago
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
Attachment #8836654 - Flags: approval-mozilla-beta?
Attachment #8836654 - Flags: approval-mozilla-aurora?
Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

Fix a regression using pure numbers alias of resource URI from the extension. Aurora53+.
Attachment #8836654 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

fix resource/chrome URLs with numeric "host", beta52+
Attachment #8836654 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 20

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6db1cdc5eedd
status-firefox53: affected → fixed

Comment 21

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/7703bdf63084
status-firefox52: affected → fixed
(Assignee)

Comment 22

5 months ago
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.
Attachment #8836654 - Flags: approval-mozilla-esr52?
Setting qe-verify- based on Junior's assessment on manual testing needs (Comment 17) and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8836654 [details] [diff] [review]
preventCanonicalResIPv4 - V2

esr52 gets synced from beta, no need for separate approval process.
Attachment #8836654 - Flags: approval-mozilla-esr52?
(Assignee)

Comment 25

5 months ago
(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

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/7703bdf63084
status-firefox-esr52: --- → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.