Some links can’t be clicked in About dialog

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tchevalier, Assigned: Gijs)

Tracking

({regression})

48 Branch
mozilla49
regression
Points:
---

Firefox Tracking Flags

(firefox47 unaffected, firefox48 fixed, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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?)
Blocks: 1253673
(Assignee)

Comment 2

2 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

2 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...)
I think option 2 sounds reasonable....
Flags: needinfo?(bzbarsky)
agreed,
Flags: needinfo?(bobbyholley)

Updated

2 years ago
Flags: needinfo?(enndeakin)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
Created attachment 8749572 [details]
MozReview Request: Bug 1269238 - allow text-links to use origin principals for link opening checks, r?mikedeboer

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 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8deb0d7312e5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1273936
No longer blocks: 1273936
(Assignee)

Comment 10

2 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 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+
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

2 years ago
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/fb8483f7bc37
status-firefox48: affected → fixed
Flags: needinfo?(gijskruitbosch+bugs)
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.