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)

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: theo, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

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?)
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
(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)
Flags: needinfo?(enndeakin)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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+
https://hg.mozilla.org/mozilla-central/rev/8deb0d7312e5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1273936
No longer blocks: 1273936
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)
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/fb8483f7bc37
Flags: needinfo?(gijskruitbosch+bugs)
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: