Closed Bug 1390917 Opened 7 years ago Closed 7 years ago

Accept data:image/* as a theme header

Categories

(WebExtensions :: Untriaged, enhancement, P2)

Product:
WebExtensions
Component:
Untriaged
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: TheOne, Assigned: TheOne)

Details

Attachments

(1 file)

Bug 1390917 - Accept data:image/png and data:image/jpeg as theme background;
59 bytes, text/x-review-board-request
aswan
: review+
Details 
One of the main use cases for Personas Plus is to apply a custom local header image.
As of now, this only works if the the image is actually inside the extension (since "headerURL" must be a relative path to the file), which doesn't work for general users.

Can we make "headerURL" accept a data:image/* blob as well? Are there any restrictions on which image formats we can handle?
Adam, is it safe (enough) to allow 'data:image/*' as a pattern to accept as value for the CSS 'background-image' property on the browser window?
Flags: needinfo?(adam)
On a quick ponder, I can't think of issues here. We allow data: URIs in CSS background properties in other contexts, so the only real difference here is evaluating them in a context that has whatever permissions have been granted to the Extension itself, right?

SVG is kind of interesting, in that it can contain <script> elements, but they're running in their own context (and data: URIs have an isolated context to boot). I don't *think* they'll be a problem.

All of that said, there are implementation subtleties here that I'm almost certainly not familiar with, so I'd like to get a quick "that looks good" from someone who breathes Gecko before moving forward. Peter? Any concerns?
Flags: needinfo?(adam) → needinfo?(peterv)
Assignee: nobody → awagner
I'm not sure whether Peter is available. Is there anyone else we could ask? This is critical for PersonasPlus to work, since we WONTFIXED bug 1369209.
Flags: needinfo?(mdeboer)
On bholley's recommendation, I'm changing the N-I to Chrisoph to see if he has the cycles to think about this. If he doesn't answer quickly, I'd suggest you check with any of the DOM or Content Security peers.
Flags: needinfo?(peterv) → needinfo?(ckerschb)
(In reply to Adam Roach [:abr] from comment #2)
> On a quick ponder, I can't think of issues here. We allow data: URIs in CSS
> background properties in other contexts, so the only real difference here is
> evaluating them in a context that has whatever permissions have been granted
> to the Extension itself, right?

Right, so the data: image will load using the security context of the including document. So the security context is not any different than loading the headerURL from a relative path inside the extension. In both worlds, the image will load using the same security context.

> SVG is kind of interesting, in that it can contain <script> elements, but
> they're running in their own context (and data: URIs have an isolated
> context to boot). I don't *think* they'll be a problem.

Please note that data: URIs loaded in an iframe or object tag will become cross origin with the including context once we flip the pref |security.data_uri.unique_opaque_origin|, which we are about to flip in the upcoming days, but that change in inheriting the security context does not apply to images. Just to be clear, data: URIs only have an isolated context to boot once we flip that pref and only if loaded within an iframe or object tag. 

SVGs itself always scare me, because as Adam mentioned, they can execute script as a payload. Would it be a big downside to exclude data:image/svg from your approach? E.g. only explicitly allow data:image/png and friends within imageDataOrStrictRelativeUrl()?
Flags: needinfo?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> SVGs itself always scare me, because as Adam mentioned, they can execute
> script as a payload. Would it be a big downside to exclude data:image/svg
> from your approach? E.g. only explicitly allow data:image/png and friends
> within imageDataOrStrictRelativeUrl()?

I don't believe so. I'll update the patch and allow PNG and JPG.
Mike, I updated the patch. Could you please review it?
Aww, just when I thought that SVGs would be the most awesome to have as a header image... Christoph, who might be able to answer the security aspect question for scripts in SVGs? Might that be :jfkthame ?
Flags: needinfo?(mdeboer) → needinfo?(ckerschb)
Flags: needinfo?(jfkthame)
Comment on attachment 8898678 [details]
Bug 1390917 - Accept data:image/png and data:image/jpeg as theme background;

https://reviewboard.mozilla.org/r/170066/#review176836

Cool stuff! Especially for your first patch here ;-)

r=me with the small change below, but that unfortunately doesn't mean you can land it just yet; you'll need review from a WebExtension peer, which I am not.

::: toolkit/components/extensions/schemas/theme.json:32
(Diff revision 3)
>                  "type": "array",
>                  "items": { "$ref": "ExtensionURL" },
>                  "optional": true
>                },
>                "headerURL": {
> -                "$ref": "ExtensionURL",
> +                "$ref": "ImageDataOrExtensionURL",

Can you make this change to the other properties as well? IOW: make it so that `additional_backgrounds` and `theme_frame` also start accepting 'ImageDataOrExtensionURL'.
Attachment #8898678 - Flags: review?(mdeboer) → review+
Attachment #8898678 - Flags: review?(aswan)
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Aww, just when I thought that SVGs would be the most awesome to have as a
> header image... Christoph, who might be able to answer the security aspect
> question for scripts in SVGs? Might that be :jfkthame ?

Try another Jonathan... :) I think :jwatt would probably be a better candidate, or would know who to ask.
Flags: needinfo?(jfkthame) → needinfo?(jwatt)
Mike, thank you for the r+! I updated the patch and will wait for review from :aswan.
Attachment #8898678 - Flags: review?(aswan)
Comment on attachment 8898678 [details]
Bug 1390917 - Accept data:image/png and data:image/jpeg as theme background;

https://reviewboard.mozilla.org/r/170066/#review177290

To be clear, this allows (some types of) data: urls both in the manifest for a webextension-theme and as parameters to `browser.theme.update()`.  Is that what is intended?
Also, the schema handling is well-tested here, but what about testing actually applying a theme that includes a data: url?  Ideally we would test that the theme elements are actually applied but at a minimum, I think we should test that nothing blows up when we do that.  (I'm mostly worried about some future change elsewhere causing a regression here that goes unnoticed)
Attachment #8898678 - Flags: review?(aswan) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Aww, just when I thought that SVGs would be the most awesome to have as a
> header image... Christoph, who might be able to answer the security aspect
> question for scripts in SVGs? Might that be :jfkthame ?

SVGs might contain script tags which might lead to an XSS vulnerability. I think we are better off not even going down that route and just allow data:image/png and data:image/jpeg. Just like the current patch does.
Flags: needinfo?(ckerschb)
Priority: -- → P2
It's unclear exactly what I'm being asked here. What I can say is that SVG when loaded as-a-document (in iframe, etc.) can run script and load external resources. When it's loaded as-an-image (<img>, 'background-image', etc.) it is blocked from running script or loading external resources.
Flags: needinfo?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #18)
> It's unclear exactly what I'm being asked here. What I can say is that SVG
> when loaded as-a-document (in iframe, etc.) can run script and load external
> resources. When it's loaded as-an-image (<img>, 'background-image', etc.) it
> is blocked from running script or loading external resources.

\o/ That's exactly what I was asking for. Cool, means we can include SVG in the list here ;-)
aswan and I talked on IRC and agreed that we would like to land this without SVG support to unblock Personas Plus.

We can add SVG in a follow-up, but it might also require changes on AMO side if we want to support developers creating themes using SVGs.
*thumb-up* from me!
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a67b6b7fef24
Accept data:image/png and data:image/jpeg as theme background; r=aswan,mikedeboer
https://hg.mozilla.org/mozilla-central/rev/a67b6b7fef24
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: