Last Comment Bug 686581 - Disable theming in SVG images
: Disable theming in SVG images
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 689224
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 16:15 PDT by Daniel Holbert [:dholbert]
Modified: 2011-09-26 12:06 PDT (History)
7 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (488 bytes, text/html)
2011-09-13 16:49 PDT, Daniel Holbert [:dholbert]
no flags Details
fix (3.10 KB, patch)
2011-09-22 12:07 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 1, Implement -moz-is-resource-document media query (2.86 KB, patch)
2011-09-23 08:59 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 2, Use it to disable theming in SVG images (5.37 KB, patch)
2011-09-23 09:00 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 3, add the missing !important (1.53 KB, patch)
2011-09-26 07:33 PDT, Mats Palmgren (:mats)
longsonr: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-09-13 16:15:26 PDT
roc pointed out that it's probably possible to detect what platform a user is on (and their GTK theme, etc) by sticking a button into an SVG image.

To prevent this data leakage, we should disable theming in SVG images.
Comment 1 Daniel Holbert [:dholbert] 2011-09-13 16:49:45 PDT
Created attachment 560071 [details]
testcase
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-21 17:11:02 PDT
Mats, can you take this? We probably just need to add a "-moz-appearance:none ! important" rule to the style sheets for all SVG images.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-21 20:44:16 PDT
Hmm, another thing we need to look at is whether links in SVG images get colored according to visitedness. If they do, then we're leaking visitedness information.
Comment 4 Robert Longson 2011-09-21 23:29:47 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Hmm, another thing we need to look at is whether links in SVG images get
> colored according to visitedness. If they do, then we're leaking visitedness
> information.

We've got bug 641731 for that.
Comment 5 Mats Palmgren (:mats) 2011-09-22 12:07:04 PDT
Created attachment 561821 [details] [diff] [review]
fix
Comment 6 Mats Palmgren (:mats) 2011-09-22 12:44:40 PDT
Do we restrict theme media queries to chrome?
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGkAtomList.h#1876
Comment 7 Robert Longson 2011-09-22 14:35:59 PDT
The test and code does not seem to be for svg within an image tag. We're OK with SVG having themed as long as it's not <image src="something.svg/>  aren't we? That's what the standalone testcase tests - you have an image tag and the svg is within that.

I think you should have tests that svg not in an image tag is still themed.

It's a bit tricky to read the tests as they seem to be all on one line too.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-22 16:23:14 PDT
It looks like this patch will apply to all SVG, which isn't what we want. We want it to only apply to the content of SVG used in an image context.
Comment 9 Mats Palmgren (:mats) 2011-09-22 20:22:50 PDT
Sorry, I thought that's what you wanted...
As far as I know it's NOT possible to do this using only CSS currently.

Maybe we could introduce a :-moz-root-of-resource-document that would match
the root element of a resource document to differentiate the two cases?
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-22 22:11:49 PDT
A media query feature would probably be slightly better.

Whether a document is a resource document can't change dynamically so it should be quite simple to implement.

One thing that just occurred to me is that it appears an <iframe> inside an SVG image document might not set its child document to return true for IsBeingUsedAsImage ... but it should.
Comment 11 Mats Palmgren (:mats) 2011-09-23 08:57:41 PDT
I can't get an <iframe> inside a SVG image to render at all.  It works when I load the
SVG directly, but when using it in a <img src=...> all I see is the <iframe>
borders, nothing inside.
Comment 12 Mats Palmgren (:mats) 2011-09-23 08:59:00 PDT
Created attachment 562051 [details] [diff] [review]
part 1, Implement -moz-is-resource-document media query
Comment 13 Mats Palmgren (:mats) 2011-09-23 09:00:37 PDT
Created attachment 562052 [details] [diff] [review]
part 2, Use it to disable theming in SVG images
Comment 15 Dão Gottwald [:dao] 2011-09-24 03:48:25 PDT
Comment on attachment 562052 [details] [diff] [review]
part 2, Use it to disable theming in SVG images

>+@media all and (-moz-is-resource-document) {
>+ foreignObject *|* {
>+   -moz-appearance: none;
>+ }
>+}

Does this prevent people from explicitly setting -moz-appearance:button? It looks like it doesn't.

Besides, CSS platform colors also still work and expose pretty much the same information, right? I don't think this is worth fighting against...
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-24 04:37:55 PDT
That's a good point about CSS platform colors, although I think those are already leaked through canvas drawing. I still think this is worth doing.

(In reply to Dão Gottwald [:dao] from comment #15)
> >+@media all and (-moz-is-resource-document) {
> >+ foreignObject *|* {
> >+   -moz-appearance: none;
> >+ }
> >+}
> 
> Does this prevent people from explicitly setting -moz-appearance:button? It
> looks like it doesn't.

Sigh, I missed that. It needs !important.
Comment 17 Dão Gottwald [:dao] 2011-09-24 04:42:56 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> That's a good point about CSS platform colors, although I think those are
> already leaked through canvas drawing. I still think this is worth doing.

You need neither canvas nor SVG for it, just CSS and getComputedStyle.
Comment 18 Matt Brubeck (:mbrubeck) 2011-09-24 08:26:17 PDT
https://hg.mozilla.org/mozilla-central/rev/ec384fc33769
Comment 19 Matt Brubeck (:mbrubeck) 2011-09-24 08:28:31 PDT
I pasted the wrong changeset above.
https://hg.mozilla.org/mozilla-central/rev/c764918036ab
https://hg.mozilla.org/mozilla-central/rev/8068ef5b4dcb
Comment 20 Mats Palmgren (:mats) 2011-09-26 07:33:07 PDT
(In reply to Dão Gottwald [:dao] from comment #15)
> Does this prevent people from explicitly setting -moz-appearance:button? 

Good catch, thanks!
Comment 21 Mats Palmgren (:mats) 2011-09-26 07:33:46 PDT
Created attachment 562423 [details] [diff] [review]
part 3, add the missing !important
Comment 22 Dão Gottwald [:dao] 2011-09-26 07:35:05 PDT
I still think this bug is pointless...
Comment 23 Robert Longson 2011-09-26 09:00:28 PDT
Comment on attachment 562423 [details] [diff] [review]
part 3, add the missing !important

Stealing review so it can land before the aurora merge if necessary.
Comment 24 Mats Palmgren (:mats) 2011-09-26 09:30:27 PDT
Thanks.
https://hg.mozilla.org/mozilla-central/rev/368b4a870769

Note You need to log in before you can comment on or make changes to this bug.