Status

enhancement
RESOLVED WONTFIX
7 years ago
5 months ago

People

(Reporter: mixedpuppy, Assigned: markh)

Tracking

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The use case is to use them to show data that is already loaded in the browser via the sidebar.
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?
Attachment #673102 - Flags: feedback?(mixedpuppy)
Comment on attachment 673102 [details] [diff] [review]
Allow data urls in panels

it looks good.
Attachment #673102 - Flags: feedback?(mixedpuppy) → feedback+
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...
Attachment #673102 - Flags: review?(jaws)
Attachment #673102 - Attachment is patch: true
FWIW, I noticed via another bug that cliq (sp?) would also like to do this
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)
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.
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Was backed out in ed0f26ebbcdb (but the backout comment had the wrong bug number)
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)
(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
)
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.  :(
And note, this is why we generally require sr for security-related changes....
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"...)
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).
(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?
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:).
(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!
This has a better fix via bug 806037
Depends on: 806037
FTR, bug 806037 landed but did not enable data: URLs anywhere (although it did lay the framework for doing that sanely)
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?
Flags: needinfo?(mixedpuppy)
Chatting with Shane in IRC, he agreed we should just WONTFIX this.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(mixedpuppy)
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.