Closed Bug 1564208 Opened 1 year ago Closed 1 year ago

Setting `svg.disabled` to `true` causes extension icons to disappear

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr68 --- fixed
firefox70 --- fixed

People

(Reporter: gk, Assigned: dholbert)

References

Details

(Whiteboard: [tor 31015])

Attachments

(3 files)

We got a bug report over in https://trac.torproject.org/projects/tor/ticket/31015 stating that setting svg.disabled to true causes extension UI buttons to disappear (in this case for uBlock Origin and uMatrix).

Optionally disabling SVG is not meant for browser chrome or extension code but rather for SVG content loaded by websites. Thus, I suspect this is a bug in the original patch that landed in bug 1216893.

Summary: Setting `svg.disabled` to `true` cause extension icons to disappear → Setting `svg.disabled` to `true` causes extension icons to disappear
Attached image screenshot of bug

Here's a screenshot of what this bug looks like, in latest Firefox Nightly with svg.disabled = true.

In the upper squarish white area of uBlock Origin's panel in this screenshot, there's supposed to be a power-button icon with four smaller icons below it. And at the very bottom in the gray area with "2" and "18", there are supposed to be 5 more icons.

Yeah, if I force that function to end up with conclusion = true (e.g. in my debugger) then I get the expected rendering. So the conclusion logic probably needs a bit more subtlety to allow SVG in "trusted" web-extension content.

Also, for the record: in this case with uBlock origin, the popup itself is just an HTML document with URI moz-extension://[some-long-random-UUID]/popup.html, and the SVG content is directly included as inline nodes in the HTML of that page.

zombie mentioned on IRC that we can test mPrincipal for webextension-ness by checking this expression for truthiness:

 BasePrincipal::Cast(mPrincipal)->AddonPolicy()

We may want to extend the LoadingPrincipal and TriggeringPrincipal cases as well (might be required in order for images/embeds in WebExtension panels to work properly, particularly if they're not moz-extension:// URIs themselves).

Assignee: nobody → dholbert
Status: NEW → ASSIGNED

Bugbug thinks this bug is a task, but please change it back in case of error.

Type: defect → task
Type: task → defect
Priority: -- → P3

I tested the attached patch locally with uBlock origin, FWIW, and it seems to work.

Do we want just addonPolicy check or isAddonOrExpandedAddonPrincipal ?
See for example what bug 1453916 did.
But perhaps addonPolicy is enough for now.

Attachment #9076890 - Attachment description: Bug 1564208: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug → Bug 1564208 part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug

(In reply to Olli Pettay [:smaug] from comment #8)

Do we want just addonPolicy check or isAddonOrExpandedAddonPrincipal ?
See for example what bug 1453916 did.

I don't know enough about addons or expanded principals to know for sure what sort of scenario would require that more lenient (I think?) check.

I did test Firefox Screenshots (which was the use case in bug 1453916), and I determined that the current patch is fine for that use-case at least. Specifically, Firefox Screenshots does have some SVG icons which indeed do not show up in its UI in current Nightly with svg.disabled=false, and the patch as it stands right now makes those icons start showing up. (So: no need to swap to isAddonOrExpandedAddonPrincipal to make that work.)

But perhaps addonPolicy is enough for now.

Unless you think there's a compelling case where we need a isAddonOrExpandedAddonPrincipal check, then yeah, I'm leaning towards sticking with the addonPolicy check.

Assuming this sticks, we should probably request uplift to the ESR68 (which is being released this week -- I don't imagine this patch will make the first esr68 release but it could make 68.01 or whatever)

This is pretty low-risk from a Firefox perspective, since it's a small patch that just tweaks an off-by-default feature that's only enabled in TorBrowser as far as we know (not exposed in the Firefox UI anywhere).

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9e6dada64af0
part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r=smaug
https://hg.mozilla.org/integration/autoland/rev/dd4671ce4c73
part 2: Use Maybe<bool> to represent lazily-initialized bools for SVG/MathML enabling. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Comment on attachment 9076890 [details]
Bug 1564208 part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a fixing bug in a feature that's used by Tor Browser (which is based off of ESR). The feature isn't exposed in Firefox (users would have to manually go to about:config and toggle the pref in order to activate this feature i.e. to disable SVG content), so this change is highly unlikely to affect many folks beyond TorBrowser users.

  • User impact if declined: Broken extension UI (missing icons) in e.g. uBlock Origin, for TorBrowser users who've dialed up their security settings (which sets this pref svg.disabled = true under the hood). See screenshot in comment 1 for an example.

  • Fix Landed on Version: 70

  • Risk to taking this patch: Low

  • Why is the change risky/not risky? (and alternatives if risky): Not risky because:
    (a) It's a very targeted fix
    (b) ...to allow SVG in one additional "trusted" context (extensions)
    (c) ...and SVG is default-allowed anyway, except when this pref is flipped (mainly in special TorBrowser configurations)
    (d) ...and the pref only intends to turn off SVG in web content, so this makes the setting better-match the pref's intent.

  • String or UUID changes made by this patch: None

Attachment #9076890 - Flags: approval-mozilla-esr68?

Comment on attachment 9076890 [details]
Bug 1564208 part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug

Fixes a bug affecting Tor browser which is non-default for Firefox. Approved for 68.1esr.

Attachment #9076890 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9076903 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Daniel: I might be a bit late with this but still: many thanks from our side to get this issue fixed so quickly and backported to esr68. That's been really impressive and very kind!

You're welcome! Thanks for the bug report. I'm glad it was a straightforward fix. :)

(BTW, I don't think this is actually fixed on the ESR68 branch yet, but it'll be there soon I think; it's got approval and someone will land all the approved bugs before long.)

QA Whiteboard: [qa-70b-p2]
You need to log in before you can comment on or make changes to this bug.