799600 - allow data urls for panels

Status: RESOLVED WONTFIX

(Firefox Graveyard :: SocialAPI, enhancement)

Description

[Shane Caraveo (:mixedpuppy)]

The use case is to use them to show data that is already loaded in the browser via the sidebar.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Allow data urls in panels

This patch allows them and does basic testing.  It was necessary to relax the same origin check in mozSocialAPI or the content couldn't get access to window.mozSocial.

Shane, can you think of any other checks the test should perform, or anything else necessary?

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 673102 [details] [diff] [review]
Allow data urls in panels

it looks good.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 673102 [details] [diff] [review]
Allow data urls in panels

Doesn't seem a priority, but still might as well knock it on the head...

Comment 4

[Mark Hammond [:markh] [:mhammond]]

FWIW, I noticed via another bug that cliq (sp?) would also like to do this

Comment 5

[Shane Caraveo (:mixedpuppy)]

IMO 19 definitely, 18 would be great since we have a use case for it (but I'd really like to end up on a regularly scheduled train sometime)

Comment 6

[edA-qa mort-ora-y]

We (cliqz) definitely have use-cases for this. Generally it'd be used to improve user experience. We can preload certain data so panels would display instantly. In some cases we can display a quick info, and then replace as the rest of the data comes in.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/689834383fbb

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Was backed out in ed0f26ebbcdb (but the backout comment had the wrong bug number)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Pushed with test-only tweaks to https://hg.mozilla.org/integration/mozilla-inbound/rev/9c064c78d86a

Green try at https://tbpl.mozilla.org/?tree=Try&rev=c9574583296d

Comment 10

[Daniel Holbert [:dholbert]]

Is it possible to do this by checking for protocol flags (e.g. URI_IS_LOCAL_RESOURCE) instead of checking for the specific "data" scheme?

At least in C++, when I've wanted to whitelist "data:" URIs, I've been encouraged to instead pick out the qualities of data URIs that I'm interested in and whitelist *those*, so that other similar protocols (e.g. "blob:") will be allowed as well.

See e.g. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDataDocumentContentPolicy.cpp?rev=84317c2f199c?mark=73-85#73 , where we implement this restriction for SVG-as-an-image. (which we only allow to reference data URIs and things like data URIs, without ever explicitly checking for the "data" scheme)

Comment 11

[Daniel Holbert [:dholbert]]

(In JS, I believe you'd want to do that by calling nsINetUtil's URIChainHasFlags() method, like we do here:
 http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#497
)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fwiw, there are other problems with the code involved.  For example, comparing an "origin" to a URI prepath means you'll fail to match if the URI contains a username....

In general, if you want to do security checks you should really be using the APIs we have for that sort of thing instead of rolling your own via string manipulation.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

And note, this is why we generally require sr for security-related changes....

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Looks like the push is bouncing anyway - but I opened bug 806037 to try and address the comments about the security sensitive code (which isn't all related to this bug - the "bad" origin checks already existed and we already had other explicit checks for "data"...)

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9fb2ed910c for a "browser_social_toolbar.js | data: panel has mozSocial object" failure (and the way we're coalescing WinXP, hard to say whether it's 50% failing or just horrible luck that a .01% failure hit on the push itself).

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Fwiw, there are other problems with the code involved.  For example,
> comparing an "origin" to a URI prepath means you'll fail to match if the URI
> contains a username....

This isn't really a problem for this particular code, since failing to match is the "safe" branch, and the check only occurs for social provider-specified URLs, which are quite unlikely to contain usernames in practice.

> In general, if you want to do security checks you should really be using the
> APIs we have for that sort of thing instead of rolling your own via string
> manipulation.  :(

These aren't really traditional "security checks". We have a built-in list of "accepted social providers", and want to confirm that the pages we're loading comes from their origin. Which APIs would you suggest we use instead of URI comparisons?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, for example you could create an nsIPrincipal that represents the origin and then use checkMayLoad (which incidentally has a flag for allowing things like data:).

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #17)
> Well, for example you could create an nsIPrincipal that represents the
> origin and then use checkMayLoad (which incidentally has a flag for allowing
> things like data:).

Ah, yeah, that sounds like a good idea. Thanks for the tip!

Comment 19

[Mark Hammond [:markh] [:mhammond]]

This has a better fix via bug 806037

Comment 20

[Mark Hammond [:markh] [:mhammond]]

FTR, bug 806037 landed but did not enable data: URLs anywhere (although it did lay the framework for doing that sanely)

Comment 21

[Mark Hammond [:markh] [:mhammond]]

I'm inclined to WONTFIX this as I don't think it fits with other plans Shane has for social.  Shane, what do you think?

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Chatting with Shane in IRC, he agreed we should just WONTFIX this.