Closed Bug 1337629 Opened 3 years ago Closed 3 years ago

Add more restrictions to the host parser

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: jhao, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

In order to fix the crash in bug 1334468, we'd like URI parser to at least reject hostnames having the following characters: http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/dom/quota/ActorsParent.cpp#113

Valentin said in https://bugzilla.mozilla.org/show_bug.cgi?id=1334468#c23 that he'd like to take this bug.

[1] https://github.com/whatwg/url/issues/159#issuecomment-270400298
Whiteboard: [necko-active]
Hi, Valentin.  I looked at your patch on try, it seems that you didn't include the character '"', did you?
MozReview-Commit-ID: H8u2C5oSiT9
Attachment #8834945 - Flags: review?(mcmanus)
Attachment #8834945 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/9256346ca5ad
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Verified!

Firefox Nightly 54.0a1 (2017-02-09)(64bit)

Ubuntu 14.04  Passed
Mac 10.11.6   Passed
Windows 10    Passed
Hi Valentin.  Thanks for fixing this so quickly.  Could you uplift this to Firefox 53 and 52?  It can solve the crash in bug 1334468.  We need this in 52 because Tor browser will uses ESR52.  Thanks.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8834945 [details] [diff] [review]
Restrict allowed hostname characters

Approval Request Comment
[Feature/Bug causing the regression]: Preexisting issue in the codebase
[User impact if declined]: Required fix for bug 1334468.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: Slightly.
[Why is the change risky/not risky?]: Small risk of web-compat issues because we are now forbidding characters which were previously allowed.
[String changes made/needed]: -
Flags: needinfo?(valentin.gosu)
Attachment #8834945 - Flags: approval-mozilla-beta?
Attachment #8834945 - Flags: approval-mozilla-aurora?
Hi :jhao, 
According to comment #9, is that expected?
Flags: needinfo?(jhao)
I'll pass this question to Valentin.
Flags: needinfo?(jhao) → needinfo?(valentin.gosu)
(In reply to Gerry Chang [:gchang] from comment #10)
> Hi :jhao, 
> According to comment #9, is that expected?

Well, kind of. What the addons were doing is a bit hacky, and there are other workarounds. At this point it's difficult to say if there are other addons doing the same thing, and if we should back it out or not.
From a webcompat point of view, Safari is also rejecting URLs containing these same characters (*[]<>) , so that shouldn't be a big issue.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8834945 [details] [diff] [review]
Restrict allowed hostname characters

Required fix for bug 1334468. Aurora53+.
Attachment #8834945 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Not convinced it makes sense to knowingly break extensions this late in the 52 cycle...
Comment on attachment 8834945 [details] [diff] [review]
Restrict allowed hostname characters

beta52- per comment 15.
Attachment #8834945 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Julien Cristau [:jcristau] from comment #15)
> Not convinced it makes sense to knowingly break extensions this late in the
> 52 cycle...

Hi Julien,

Could you reconsider the request to uplift this patch to 52, please?

The reasons are:
1. The next version of the Tor Browser will be based on Firefox ESR 52.
   This fix can avoid a potential horrible crash for them.
2. The code change of this patch is small (only one line), and the risk is low.
3. Although we knew the patch breaks the addon greasemonkey, according to the ticket 
   https://github.com/greasemonkey/greasemonkey/issues/2480, greasemonkey already fixed 
   the issue on their own.  The patch won't bring breakage to them anymore.

I think the only concern is, we might break other addons which are using the newURI() API
in a hacky way as greasemonkey did.
To be honest, we don't know how many of such addons exist.
However, I believe making host parser more restrictive is the right thing.
(comment 12: Safari is also rejecting URLs containing these same characters (*[]<>))

Furthermore, if an addon is broken by this patch, they could fix the issue as greasemonkey did.

What do you think?
Flags: needinfo?(jcristau)
(In reply to Ethan Tseng [:ethan] from comment #17)
> 1. The next version of the Tor Browser will be based on Firefox ESR 52.
>    This fix can avoid a potential horrible crash for them.

Oh, actually not just avoiding crashes for the Tor Browser.
It can avoid crashes for any Firefox ESR 52 user who is trying to use the First Party Isolation
feature by flipping on the pref "privacy.firstparty.isolate".
I was assuming the Tor Browser would keep carrying patches on top of ESR 52 anyway, is that not the case?
(In reply to Julien Cristau [:jcristau] from comment #19)
> I was assuming the Tor Browser would keep carrying patches on top of ESR 52
> anyway, is that not the case?

Uplifting won't be too big a problem for Tor, but I think it's probably better for 
Firefox ESR 52 to avoid crashes anyhow.
Please allow me to quote Jonathan's comment in bug 1334468 [1] here:
"From my point of view, the fix in Bug 1337629 is not only more straightforward but also less risky because it has been in Nightly for 13 days and in Aurora for 7 days.  There was only one addon breakage reported, but the owner of that addon quickly fixed that."

This bug can prevent crashes for at least millions of ESR users.
Shouldn't we uplift it to beta?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1334468#c42
Comment on attachment 8834945 [details] [diff] [review]
Restrict allowed hostname characters

Alright, I think you've convinced me.  Let's get this one in 52.0b9.
Flags: needinfo?(jcristau)
Attachment #8834945 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
(In reply to Julien Cristau [:jcristau] from comment #22)
> Alright, I think you've convinced me.  Let's get this one in 52.0b9.

Thank you, Julien and Ryan!
Depends on: 1355487
You need to log in before you can comment on or make changes to this bug.