Closed
Bug 1269238
Opened 8 years ago
Closed 8 years ago
Some links can’t be clicked in About dialog
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
People
(Reporter: theo, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
The following links can’t be clicked anymore in About dialog: Licensing information End-User Rights global community Regression window: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae7413abfa4d3954a6a4ce7c1613a7100f367f9a&tochange=973dfa15822126c39ea2d98d34ac16643b040243 (Is there a way to narrow it that I missed with mozregression?)
Comment 1•8 years ago
|
||
Regression window: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=9a8ff2f4978493c3fd9982652a09ffcdf2333a75&tochange=d4b74afcc60b937f25d54f365be46a8133980209 Regressed by: Bug 1253673
Flags: needinfo?(gijskruitbosch+bugs)
Updated•8 years ago
|
Blocks: CVE-2016-5268
Assignee | ||
Comment 2•8 years ago
|
||
Delightful. This is because the text.xml text-link binding does this: var uri = null; try { ... uri = ioService.newURI(href, null, null); var nullPrincipal = secMan.createNullPrincipal({}); try { secMan.checkLoadURIWithPrincipal(nullPrincipal, uri, nsISSM.DISALLOW_INHERIT_PRINCIPAL) } catch (ex) { ... return; } The null principal can no longer link to about: URIs so this fails. This code has been here since bug 315166, which ~= forever (OK, OK, 10 years... net effect is the same). Looking at the description of that bug, it seems at the time we didn't have origin principals. The world has moved on since then... and from looking at that bug the correct thing is for this code to use the node's own principal. However, from looking at bug 315004, which is cited from bug 315166 comment #0, it seems some consumers expect to be able to put 'malicious' URIs in text-links (like things provided by the user and/or untrusted code), and have them be blocked by the binding. So there are a couple of things we could do: 1) always use the document principal for text-link because that assumption is fairly weird anyway. We can keep the inherit_principal stuff so JS and data: URIs would still break; 2) add an attribute to opt-in to using the document principal (hinted at in bug 315166 comment #0) instead of the null principal, and sprinkle that around appropriately; 3) work around this in the about dialog, which is chrome-privileged anyway; 4) mark all the pages we link to from the about:dialog as content-linkable; I'm tempted to go for 2, but I could be convinced to go for any of the other ones. There may be even more options that I haven't thought of. Either way, we're loosening security restrictions so I'd like to cast a wide net for opinions on how to fix this. Boris/Bobby/Neil, thoughts?
Component: General → XUL Widgets
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Product: Firefox → Toolkit
Assignee | ||
Comment 3•8 years ago
|
||
(I audited 'text-link' in non-test non-objdir non-CSS code in our codebase, and AFAICT the about dialog is the only case where we stick about: URIs in them - apart, I suppose, from perhaps add-ons that use them for their links which we then show in the add-ons manager, which is fun because that's exactly the kind of stuff bug 315166 / bug 315004 was trying to deal with...)
Updated•8 years ago
|
Flags: needinfo?(enndeakin)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51045/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51045/
Attachment #8749572 -
Flags: review?(mdeboer)
Comment 7•8 years ago
|
||
Comment on attachment 8749572 [details] MozReview Request: Bug 1269238 - allow text-links to use origin principals for link opening checks, r?mikedeboer https://reviewboard.mozilla.org/r/51045/#review47727 LGTM! ::: toolkit/content/widgets/text.xml:317 (Diff revision 1) > Components.classes["@mozilla.org/network/io-service;1"] > .getService(Components.interfaces.nsIIOService); > > uri = ioService.newURI(href, null, null); > > - var nullPrincipal = secMan.createNullPrincipal({}); > + var principal; nit: you might as well change this to `let`
Attachment #8749572 -
Flags: review?(mdeboer) → review+
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8deb0d7312e5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8749572 [details] MozReview Request: Bug 1269238 - allow text-links to use origin principals for link opening checks, r?mikedeboer Approval Request Comment [Feature/regressing bug #]: bug 1253673 [User impact if declined]: some links in about dialog don't work [Describe test coverage new/current, TreeHerder]: limited automated testing [Risks and why]: reasonably low, has baked for a significant amount of time by now without known regressions, impact is pretty much limited to the about dialog. [String/UUID change made/needed]: nope
Attachment #8749572 -
Flags: approval-mozilla-beta?
Comment 11•8 years ago
|
||
Comment on attachment 8749572 [details] MozReview Request: Bug 1269238 - allow text-links to use origin principals for link opening checks, r?mikedeboer User facing regression, taking it in beta. Should be in 48 beta 3.
Attachment #8749572 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•8 years ago
|
||
has problems uplifting to beta: grafting 343873:8deb0d7312e5 "Bug 1269238 - allow text-links to use origin principals for link opening checks, r=mikedeboer" merging browser/base/content/aboutDialog.xul merging toolkit/content/widgets/text.xml warning: conflicts while merging toolkit/content/widgets/text.xml! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 13•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/fb8483f7bc37
Flags: needinfo?(gijskruitbosch+bugs)
Updated•8 years ago
|
Version: unspecified → 48 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•