Closed Bug 692677 Opened 13 years ago Closed 12 years ago

Relax same-origin XHR restrictions for privileged applications

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
blocking-b2g 2.2?
blocking-basecamp ?
tracking-b2g backlog

People

(Reporter: cjones, Assigned: philikon)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

Let's bring back |netscape.security.PrivilegeManager.enablePrivilege('UniversalBrowserRead');| hahaha!

But seriously, we need a way to enable this.  There are many use cases.  Probably the most compelling case for the purposes of this bug is a client-side-only, Google Reader-style news aggregator.  Yes, CORS is a partial solution for XHR, but it's naive to assume all aggregated servers will support it.  The economics of the situation are wrong too: only the aggregator app suffers by not being able to compete with native apps.  The app can't pressure servers into better CORS support unless it has a lot of users, but the app can't get users unless news servers support CORS.  A better market is setting up a privilege barrier for cross-origin access in the news aggregator.  Then the happy little world of CORS-compliant servers and apps that don't need explicit cross-origin privileges would theoretically have a better UX than the crufty old world, and possibly become more popular.

This is a pretty interesting design problem.  We certainly want sites to explicitly request this privilege.  (Popping up a doorhanger saying "Do you want to allow cross-origin access?" whenever a site generates a same-origin exception is a UX and security nightmare.  Looking at you, IE ...)  There's not really a clean way to have users implicitly grant this privilege in context, in general: the news aggregator wants to show a feed when it first loads, built by script, not in response to a user action.

At the same time, "do you want to relax same-origin?" is a very difficult question to ask users.  I can't think of a simple way to phrase it, off the top of my head.

This /could/ be a purely UA-side feature, but since we want sites to explicitly request this privilege I think we need API for it.  I think we can focus on XHR for the time being.  There are use cases for relaxed same-origin with canvas.getImageData(), e.g., but I can't think of one that couldn't be worked around easily enough with a cross-origin binary XHR.

Anyone have clever ideas for API?  Here's a strawman proposal: add an attribute "crossOrigin" to XMLHttpRequest.  If |req.send()| is called on an XMLHttpRequest |req| with |req.crossOrigin = true|, then it's up to UAs to allow or deny the request.  If the request is denied, the UA can pretend like the request timed out.  (This obviously doesn't address the UI problem.)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> But seriously, we need a way to enable this.  There are many use cases. 
> Probably the most compelling case for the purposes of this bug is a
> client-side-only, Google Reader-style news aggregator.

How about allowing apps to register interest in RSS feeds (using Web Intents or registerProtocolHandler or something new) and then have browser UI for RSS feeds that lets the user choose to view a feed in a registered Web app?

Question: what access to cross-origin content is actually needed here? An app can load a cross-origin page into an IFRAME. What else is needed?

And what's wrong with Google Reader that makes you want to do it "client-side-only"?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> > But seriously, we need a way to enable this.  There are many use cases. 
> > Probably the most compelling case for the purposes of this bug is a
> > client-side-only, Google Reader-style news aggregator.
> 
> How about allowing apps to register interest in RSS feeds (using Web Intents
> or registerProtocolHandler or something new) and then have browser UI for
> RSS feeds that lets the user choose to view a feed in a registered Web app?
> 

What manages the RSS feed subscriptions in your example?  

> Question: what access to cross-origin content is actually needed here? An
> app can load a cross-origin page into an IFRAME. What else is needed?
> 

Here are some more use cases that would need cross-origin permissions
 - RSS feed
 - screen-scraping in order to automatically summarize articles
 - fetch images from a flickr feed, say, allow editing / mashing up

> And what's wrong with Google Reader that makes you want to do it
> "client-side-only"?

Privacy, scalability, ...?  Doesn't really matter though.
I do think that we'll want to allow privileged applications raw TCP socket access. There are lots of use cases out there for communicating with network connected hardware out there.

Raw TCP socket access is certainly more scary than XHR access in many ways (can reach hardware behind firewalls, etc), but it's actually safer than cross-origin XHR in *one* way. With raw TCP socket access the browser won't inject the users credentials (cookis, auth:-headers) into the request. So you can't really do anything that you couldn't do directly from the server. Except accessing things behind firewalls of course :-)

Another data point is that one of the most requested privileges for android apps is internet access. Which I *think* is equivalent to raw TCP socket access. Apparently this is something that almost every android app asks for. Though I do think we can get that down significantly by offering the normal same-origin-plus-CORS-plus-websocket security model of the web.

And in the cases where we grant raw TCP socket access, we can also grant "anonymous" XHR access. I.e. the ability to make XHR requests which doesn't include the users credentials.

In either case, I definitely think that the app should need to ask for elevated privileges to get this access though.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> > > But seriously, we need a way to enable this.  There are many use cases. 
> > > Probably the most compelling case for the purposes of this bug is a
> > > client-side-only, Google Reader-style news aggregator.
> > 
> > How about allowing apps to register interest in RSS feeds (using Web Intents
> > or registerProtocolHandler or something new) and then have browser UI for
> > RSS feeds that lets the user choose to view a feed in a registered Web app?
> 
> What manages the RSS feed subscriptions in your example?

The Web application. Browser UI would only be required for the initial access grant.

>  - fetch images from a flickr feed, say, allow editing / mashing up

Flickr already allows this via JSON-P.

> > And what's wrong with Google Reader that makes you want to do it
> > "client-side-only"?
> 
> Privacy, scalability, ...?  Doesn't really matter though.

Actually it does matter, because it affects what you'd be willing to do on a server.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> >  - fetch images from a flickr feed, say, allow editing / mashing up
> 
> Flickr already allows this via JSON-P.

Hmm, maybe not the actual image data.
Flickr does support feeds though, so if you use browser UI to establish a connection between a Flickr feed and a Web app, then we could grant the app read access to the contents of <link rel="enclosure"> resources.

Another idea for the API could be <input type="feed">, where the browser UI pops up a list of the feeds exposed by your open tabs or frequently visited sites or something.
Flickr could easily make image data available now by using CORS. This would work great with the new crossorigin attribute added to <img> in combination with <canvas>. (though I think we should add <img>.getImageData() and <video>.getImageData() as well)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> > > > But seriously, we need a way to enable this.  There are many use cases. 
> > > > Probably the most compelling case for the purposes of this bug is a
> > > > client-side-only, Google Reader-style news aggregator.
> > > 
> > > How about allowing apps to register interest in RSS feeds (using Web Intents
> > > or registerProtocolHandler or something new) and then have browser UI for
> > > RSS feeds that lets the user choose to view a feed in a registered Web app?
> > 
> > What manages the RSS feed subscriptions in your example?
> 
> The Web application. Browser UI would only be required for the initial
> access grant.
> 

OK, I understand now.  Some issues
 - what happens if two installed apps want to read a different set of RSS feeds for different reasons?
 - what happens if one application wants to read an RSS feed from its own domain, for its own use?
 - privacy problem when switching from default RSS reader X to reader Y: if X still maintains a set of subscriptions and tries to access them, Y will be used as the handler, which leaks info from X to Y without the user necessarily knowing

I think this approach would reduce to granting particular applications "RSS privileges" for their own use, which seems overly specific.

> >  - fetch images from a flickr feed, say, allow editing / mashing up
> 
> Flickr already allows this via JSON-P.
> 

JSON-P is horrible :P.  But more importantly, it has the same economic problem as CORS: JSON-P requires server-side changes for web apps that aren't needed for native apps, which leaves web apps at an inherent disadvantage with no leverage to change the situation.

> > > And what's wrong with Google Reader that makes you want to do it
> > > "client-side-only"?
> > 
> > Privacy, scalability, ...?  Doesn't really matter though.
> 
> Actually it does matter, because it affects what you'd be willing to do on a
> server.

Requiring a Google-sized server infrastructure is tremendously unfair economically to Joe Webauthor, and in the grand scheme of things, it's worse for users because maintaining a secure and privacy-respecting server infrastructure is tremendously hard.  I don't think this example matters because it doesn't really bear on whether cross-origin privileges are fundamentally too insecure or difficult.  For example, we don't let web apps run machine code (well, unless they call themselves "extensions") not because computationally-intensive work can be offloaded to servers, but because JS is fast enough to outweigh the fundamental problems of machine code.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Another idea for the API could be <input type="feed">, where the browser UI
> pops up a list of the feeds exposed by your open tabs or frequently visited
> sites or something.

But then you have to specify not only RSS but the format of the feeds, so you know what data can be loaded by the app.  Right?  Do we really want to do that?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> OK, I understand now.  Some issues
>  - what happens if two installed apps want to read a different set of RSS
> feeds for different reasons?

The user could direct different feeds to different apps. I'm not sure what the problem is.

>  - what happens if one application wants to read an RSS feed from its own
> domain, for its own use?

They can just do that using XHR I guess. What's the problem?

>  - privacy problem when switching from default RSS reader X to reader Y: if
> X still maintains a set of subscriptions and tries to access them, Y will be
> used as the handler, which leaks info from X to Y without the user
> necessarily knowing

I imagine we'd simply give apps permission to read particular feeds. Switching default readers would have to be some kind of mass permission copy. If that's a one-time thing, no additional information would be leaked.

> JSON-P is horrible :P.  But more importantly, it has the same economic
> problem as CORS: JSON-P requires server-side changes for web apps that
> aren't needed for native apps, which leaves web apps at an inherent
> disadvantage with no leverage to change the situation.

"No leverage" overstates the case. There are plenty of popular Web apps out there that would benefit from CORS access. There are existing incentives to provide it.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > Another idea for the API could be <input type="feed">, where the browser UI
> > pops up a list of the feeds exposed by your open tabs or frequently visited
> > sites or something.
> 
> But then you have to specify not only RSS but the format of the feeds, so
> you know what data can be loaded by the app.  Right?  Do we really want to
> do that?

You could give the app XHR access to the feed URL and, possibly, to any <link rel="enclosure"> URIs found as the feed URL is parsed. The constraints on the feed format would be light. We'd have to require the feed be parsed via HTML or XML parsing, that's about it.

(I'm not really sure about <link rel="enclosure"> ... probably the editing use-case can be addressed by just having the user drag-and-drop or copy/paste the image they want to edit into the editor.)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> > OK, I understand now.  Some issues
> >  - what happens if two installed apps want to read a different set of RSS
> > feeds for different reasons?
> 
> The user could direct different feeds to different apps. I'm not sure what
> the problem is.
> 

Then registerProtocolHandler() doesn't work, and using an intent-like system would be weird because the browser would have to handle the intent and remember each individual feed permission granted.  It promotes the feed to a first-class web citizen.

> >  - what happens if one application wants to read an RSS feed from its own
> > domain, for its own use?
> 
> They can just do that using XHR I guess. What's the problem?
> 

I was thinking this would interact badly with registerProtocolHandler(), but it wouldn't.  Never mind.

> > JSON-P is horrible :P.  But more importantly, it has the same economic
> > problem as CORS: JSON-P requires server-side changes for web apps that
> > aren't needed for native apps, which leaves web apps at an inherent
> > disadvantage with no leverage to change the situation.
> 
> "No leverage" overstates the case. There are plenty of popular Web apps out
> there that would benefit from CORS access. There are existing incentives to
> provide it.
> 

OK, desktop web apps can apply leverage, which indirectly helps web apps on phones better compete with native apps.

> (In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > > Another idea for the API could be <input type="feed">, where the browser UI
> > > pops up a list of the feeds exposed by your open tabs or frequently visited
> > > sites or something.
> > 
> > But then you have to specify not only RSS but the format of the feeds, so
> > you know what data can be loaded by the app.  Right?  Do we really want to
> > do that?
> 
> You could give the app XHR access to the feed URL and, possibly, to any
> <link rel="enclosure"> URIs found as the feed URL is parsed. The constraints
> on the feed format would be light. We'd have to require the feed be parsed
> via HTML or XML parsing, that's about it.
> 

That doesn't scale to choosing an image from a flickr feed, e.g., but the drag-n-drop idea sounds plausible (if we had a way to do that on touch screens ...).  How would this work for choosing an audio clip to edit, say?  How would a user choose an article for a reader to screen-scrape and reformat?

What it sounds like all this boils down to is granting generic cross-origin request privileges to particular apps for particular URLs.  Then if the fetched data is HTML/XML-y, users can grant the requesting app access to embedded resources without a second explicit request from the app.  Those sound like orthogonal things to me.  I'm not so keen on the second part because (i) problem above of presenting UI for non-visual or really large items; (ii) practical issue of standardizing around HTML/XML-ish format for items ("Why not JSON?" is the first thing they'll say).  I could see something interesting being made of this though; I wonder if this could be implemented for simple cases as a "src" for <input>, say something like <input type="file" accept="image/*" src="[URI]">.  Apps could set the src to a data: URI for the flickr editor case, after they've parsed the stream.

What I *am* keen on is allowing the grant of particular cross-origin request privileges.  If this all makes sense, how does the proposal in comment 0 suit?

Hm, I just realized that there's a security issue with granting persistent cross-origin privileges to a particular URL: if say the owner of say ocallahan.org loses his domain registration, and another site takes it, then the app will still keep permissions to content that the user didn't explicitly grant.  https is a solution, but yuck, that would severely limit the usefulness of this.  Hmm...  Maybe not worth losing sleep over.
I really hope we could have way to grant cross-domain priviliges for any domain. In my case, it's needed because i'm streaming live video from server A (localhost, using VLC), but loading HTML from server B (internet). I'm trying to do some realtime pixel manipulation for video canvas and getImageData() fails (Security error). My application is kiosk-style environment, so disabling security checks is not a issue and there should be some way to do this. It could be against spec, i don't know about that, but it should be noted that browsers are used more and more for specific applications that have only single purpose and normal security methods are not always needed. 

AFAIK, there is already solution for this, it's 'Access-Control-Allow-Origin' HTTP header that server sends when requested content is allowed to be used in cross-domain environment. However, i'm streaming video content from VLC, and it's not possible to modify HTTP-headers it generates, it's not possible to request cross-domain priviliges this way.

For me, it would be enough to have user setting that would toggle cross-domain checks on/off completely, but more elegant solution would be list of domains somewhere in settings UI.
The right thing to do there is to modify your video server to send Access-Control-Allow-Origin. VLC is open-source even.
Yes, this bug covers cases like client-side news aggregators in which it's not practical to guarantee Access-Control-Allow-Origin for all source servers.  When you control the server, that's not an issue.
My needs were just an example, proposed solution would fix this issue for all cases. And since this functionality is already done (Access-Control-Allow-Origin) in Firefox, it only needs to be exposed to config file/settings UI. 

I will, however, see if patching VLC would be solution for my problem.
I compiled latest version of VLC with modifications to send mentioned header to browser. My modifications seem to work as they should, since Firebug shows correct headers (meaning: Access-Control-Allow-Origin: *). However, i still get Security Error. Look's like it completely ignores this header.

Javascript code looks like this (just the frame processing loop, without any code that modified image data):

	copy.drawImage(video, 0, 0);
	output.clearRect(PAINTRECT.x, PAINTRECT.y, WinW, WinH);
	var imageData = copy.getImageData(0,0,VidW,VidH);
	/* do something with data here */
	output.drawImage(copycanvas, 0, 0, WinW, WinH);

Commenting out getImageData result video to be shown, with getImageData, it causes Security Error and script stops there. 

Am i missing something?
Yeah, we probably need to implement http://www.whatwg.org/specs/web-apps/current-work/#attr-media-crossorigin and you need to use it. Please file a new bug for this.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Yeah, we probably need to implement
> http://www.whatwg.org/specs/web-apps/current-work/#attr-media-crossorigin
> and you need to use it. Please file a new bug for this.

Hmm, that's intresting, but i don't really see how it has much to do with this problem. This situation and problem is well documented in here, and it's already in spec :

http://www.w3.org/TR/cors/ (search for "Not tainting the canvas element")

Like i mentioned before, Firefox ignores mentioned headers when dealing with media files. I don't know, if those headers are used when using functions like XMLHttpRequest() but for getImageData() they are ignored.
And i would like to add, spec only mentions toDataURL(), but it should be the same for getImageData()?
> http://www.w3.org/TR/cors/ (search for "Not tainting the canvas element")

Yes, but the point is that CORS is not applied to media by default because that would break web sites.  It needs to be opt-in on the part of the HTML embedding the media, and we don't implement that opt-in yet.

Now can you please stop adding noise to this bug and do what Robert asked you to do in comment 17, since it looks like what I asked you to do in bug 604496 comment 8 is already handled by the spec?  So all we need now is to add an implementation of that spec.  But that's not _this_ bug; it needs a separate bug on the Video/Audio component to track it.
Why does this block bug 715782, b2g demo phone?  This seems like a somewhat experimental feature to me.
What seems experimental about it?  How would I write a client-side news aggregator without cross-origin XHR?

But, orthogonally, no this isn't hard-blocking anything Mozilla is planning for demo-phone.
Blocks: b2g-product-phone
No longer blocks: b2g-demo-phone
> What seems experimental about it?  How would I write a client-side news aggregator without cross-
> origin XHR?

Just the idea that we should design an API for implementing entirely client-side RSS readers, I guess.  Surely this API is necessary if you want an entirely client-side news aggregator.  But I think the question is whether or not we want apps like that, or whether the client-server approach taken by Google Reader is sufficient.
This has been hashed out in excruciating detail above.  If you have new information to add, please do.

If your opinion is that "no, this use case doesn't seem valid", I'll chalk that up if the decision comes down to a vote.
(In reply to Justin Lebar [:jlebar] from comment #23)
> > What seems experimental about it?
> 
> Just the idea that we should design an API for implementing entirely
> client-side RSS readers, I guess.

UserScripts (GreaseMonkey and equivalents) support such loosening for XHR and it's been a popular and well-used feature. Can't think of a single one that's a client-side RSS reader. For "privileged applications" some measured amount of "loosening" could be useful.

The amount of relaxation and the mechanism needs a security review. Such apps could be poking holes in the security of the other-origin sites (maliciously or not) and we need to put the user in control. First step is to make them aware. Blanket cross-site access like Firefox chrome is a terrible idea. Could the app manifest declare a whitelist of 3rd-party sites it needs to access, probably via limited particular APIs like XHR, sockets, WebGL?
Sorry, the discussion refocused this bug around XHR (IMHO), but that wasn't reflected in the bug title.  Would prefer to keep this feature separate from the raw socket discussion, but the secreviews will be similar I suspect.

Yes, we can have manifests declare a whitelist.  I suspect that's at least an 80% solution to this use case.  (Doesn't work for a full Reader-style aggregator though, obviously.)
Summary: Allow loosening same-origin checks for privileged applications → Relax same-origin XHR restrictions for privileged applications
need to sched the sec-review on this one please work with me to do so
Whiteboard: [secr:curtisk]
Whiteboard: [secr:curtisk] → [sec-assigned:curtisk:749372]
This is a dep of P1 apps calendar, email.
I thought e-mail was using raw TCP sockets, not XHR.

Why does calendar care about cross-origin XHR?
ActiveSync (Msft Exchange) uses HTTP.

Calendar, Email and maybe Contacts will sync with it as part of P1 requirements as an additional backend.

Calendar also will use CalDav (WebDav/HTTP based protocol) as a backend (think calendar.[google|yahoo|etc..].com).

This is all in addition to the raw socket access Email needs for imap/smtp (and eventually pop).
Sync with Exchange is a P1?  That does not make sense to me.  iPhone 1 shipped without Exchange support, didn't it?

But I guess that's besides the point if we need this for CalDAV.  Thanks for clarifying.
(In reply to Justin Lebar [:jlebar] from comment #32)
> Sync with Exchange is a P1?  That does not make sense to me.  iPhone 1
> shipped without Exchange support, didn't it?

Hotmail is #1 in Brazil.  Hotmail's supported sync mechanisms are POP (sucky experience), or ActiveSync.  Exchange is also very important, too.  But of course, any discussions of e-mail prioritization should be discussed elsewhere.
Assuming that each app still gets its own cookie and password store, I don't see any risk for relaxing XHR same-origin restrictions for trusted and certified apps.  That's the current spec'ed behavior for the apps security model.
> I don't see any risk for relaxing XHR same-origin restrictions for trusted and certified 
> apps.

You mean, all trusted apps automagically get this privilege by virtue of being trusted, or any trusted app may apply for this privilege?

In any case, it would be a shame if my RSS reader had to be trusted; we should think about whether we can do this for untrusted apps too.
Yes, but to be clear, only privileged assets in a trusted app can bypass same-origin.  Non-app content still has to obey it as its bound by the browser security model.  

I don't think you can relax this for any untrusted content; you'd punch a big hole in CSRF defenses (i.e. an ad or other 3rd-party content in an untrusted Facebook app could CSRF Facebook).
> Yes, but to be clear, only privileged assets in a trusted app can bypass same-origin.

Yes to what?

> I don't think you can relax this for any untrusted content; you'd punch a big hole in 
> CSRF defenses

I see.  That's a lot more concerning than the fact that you could find your way into intranets.
I think we should do this by always making requests with the "anonymous" flag set. That means that we'll never set any cookies automatically (The site can still do that by using setRequestHeader, but it have to provide the actual cookie values).

That should make it no more possible to CSRF a site than you can from the server.

However that still means that it's possible to read data from behind firewalls. Until we figure out how to handle that I think that means that we can only expose this to trusted apps.
Whiteboard: [sec-assigned:curtisk:749372] → [sec-assigned:curtisk:749372][b2g:blocking+]
Here's what I think we should do in this bug:

Add a argument to the XMLHttpRequest constructor, so something like:

new XMLHttpRequest({ anon: true, system: true });

When the 'system' is set to true the XHR object do not do any cross-origin security checks. Including for things like reading arbitrary response headers, and sending arbitrary request headers.

When the 'anon' is set to true the XHR object makes the request with the LOAD_ANONYMOUS flag set to true which causes no cookie headers or other authentication data to be added to the request by gecko.


For now we would only let trusted apps make requests with the 'system' flag set to true, and only when the 'anon' flag is also set to true.
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [sec-assigned:curtisk:749372][b2g:blocking+] → [sec-assigned:curtisk:749372]
I think I came across another use case for this today in the v1 B2G browser. I need to fetch favicons from different domains to store in global history in IndexedDB. The browser API tells the browser about favicon URLs, but doesn't return the actual favicon file. We could potentially change the API to return the file, but that's not ideal and may not work in all cases. Requiring all web sites to set Access-Control-Allow-Origin HTTP headers for favicons isn't really feasible!

BTW. on the subject of the calendar use case, CalDAV is a superset of WebDAV which is a superset of HTTP so cross-origin HTTP alone may not give you all the verbs etc. you need, you might need TCP socket access for that? It would enable you to subscribe to read-only iCalendar feeds over HTTP though, which is a start.
Gonna take a stab at this.
Assignee: nobody → philipp
Attached patch wip 1 (obsolete) — Splinter Review
I think this is how Jonas wants this to work. It's missing any kind of permission checking, but that's not the biggest problem. nsXMLHttpRequest::Initialize is no longer used from content, since XMLHttpRequest has the new Paris bindings now (see dom/bindings).

Need to find out whether there's a way to

(a) do dictionaries in WebIDL (might be easy to implement or work around if missing)
(b) define a custom constructor/constructor args with Paris bindings

bz, any guidance would be appreciated!
Attachment #629403 - Flags: feedback?(jonas)
Huh, I had no idea we had dictionaries in XPIDL now!

For WebIDL, we currently don't have dictionary support at all.  As in, it needs parser support and then codegen.  I _think_ it shouldn't be too bad in the codegen; I'm not sure about how much work the parser end would be.

You could do a poor-man's dictionary for now with an optional "object" argument and explicit JSAPI property gets in the callee....  But if you have the time to do dictionaries, even just the parser part, that would rock.  I can definitely help with the codegen end, but I don't know the parser that well yet.
What we've done (or at least attempted to do) for dictionaries in at least one other case is to define the dictionary in XPIDL, and make the type of the dictionary argument in the WebIDL be "any" (with implicitJSContext set in Bindings.conf). That translates into a jsval and JSContext* argument in the called C++ code, and then the called C++ code can use the dictionary helper stuff that smaug wrote.

See http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dictionary_helper_gen.conf and http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEvent.cpp#388 for ideas of how the dictionary stuff works in XPIDL.

That's probably the path of least resistance until we have proper dictionary support in WebIDL.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #44)
> What we've done (or at least attempted to do) for dictionaries in at least
> one other case is to define the dictionary in XPIDL, and make the type of
> the dictionary argument in the WebIDL be "any" (with implicitJSContext set
> in Bindings.conf). That translates into a jsval and JSContext* argument in
> the called C++ code, and then the called C++ code can use the dictionary
> helper stuff that smaug wrote.

Sounds great! Do you know how I can get the Paris bindings to to do that on my constructor/initializer? I guess I'm not seeing how nsDOMEvent::InitFromCtor/Initialize are called. nsXMLHttpRequest::Initialize definitely isn't called anymore.
nsXMLHttpRequest::Constructor() (inline in nsXMLHttpRequest.h) is called from the new bindings. The current constructor doesn't take any arguments, but see http://www.w3.org/TR/WebIDL/#Constructor for more. Also, I don't know if we have codegen that deals with constructors with arguments yet, or that we've ever tested that code if we have it.
Comment on attachment 629403 [details] [diff] [review]
wip 1

Review of attachment 629403 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good other than that it needs tests and security bits.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1719,5 @@
>  
>  bool
>  nsXMLHttpRequest::IsSystemXHR()
>  {
> +  return mIsSystem || !!nsContentUtils::IsSystemPrincipal(mPrincipal);

You can remove the !!. Not sure why those were there in the first place.
Attachment #629403 - Flags: feedback?(jonas) → feedback+
> Also, I don't know if we have codegen that deals with constructors with arguments yet

We do.

> or that we've ever tested that code if we have it.

We haven't.  Though I just added a basic test (that the right types of arguments at least are passed); see http://hg.mozilla.org/integration/mozilla-inbound/rev/fdb686511fc4
Yeah I just looked through the tests [1] and traced the way that the constructor hook (which ends up calling MyClass::Constructor) is created in Codegen.py [2]. I might I have everything to make this work the way Johnny suggested.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/bindings/parser/tests/test_constructor.py#44
[2] https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#560
Note that you'll probably need bug 760749 fixed first, but I'm hoping that will happen mid-next-week (Kyle gets back from vacation Monday, give him two days to catch up on e-mail, then hopefully he'll review).
(In reply to Boris Zbarsky (:bz) from comment #50)
> Note that you'll probably need bug 760749 fixed first

Even for "any" constructor argument, as Johnny suggested? But in any case, I'm happy to work on top of your patch.
> Even for "any" constructor argument, as Johnny suggested?

Oh, no.  "any" should just work.
Attached patch wip 2 (obsolete) — Splinter Review
Gallia est pacata.

Still lacking permission checks and tests, but this now properly integrates into the new bindings. Tested manually and it works. I should point out, however, that I haven't done anything about workers yet apart from making them compile.
Attachment #629656 - Flags: feedback?(jonas)
Attachment #629403 - Attachment is obsolete: true
> I think I came across another use case for this today in the v1 B2G browser. I need to fetch 
> favicons from different domains to store in global history in IndexedDB.

We can do <img src="the-favicon-url">, of course.

That's not ideal for a variety of reasons (principally, we don't control caching of the favicons and have no way to guarantee that they'll be there), so I agree that using this API would be an improvement.
Comment on attachment 629656 [details] [diff] [review]
wip 2

Review of attachment 629656 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.h
@@ +210,5 @@
>      mPrincipal = aPrincipal;
>      BindToOwner(aOwnerWindow);
>      mBaseURI = aBaseURI;
> +    mIsAnon = false;
> +    mIsSystem = false;

Hmm.. I think setting these in the ctor should be enough. I'd rather not rely on remembering to set these if we add more Construct functions.

::: dom/workers/XMLHttpRequest.cpp
@@ +1465,5 @@
>  XMLHttpRequest*
> +XMLHttpRequest::Constructor(JSContext* aCx,
> +                            JSObject* aGlobal,
> +                            const Optional<jsval>& aParams,
> +                            ErrorResult& aRv)

File a followup to implement in workers.
Attachment #629656 - Flags: feedback?(jonas) → feedback+
Depends on: 761227
(In reply to Jonas Sicking (:sicking) from comment #55)
> ::: dom/workers/XMLHttpRequest.cpp
> @@ +1465,5 @@
> >  XMLHttpRequest*
> > +XMLHttpRequest::Constructor(JSContext* aCx,
> > +                            JSObject* aGlobal,
> > +                            const Optional<jsval>& aParams,
> > +                            ErrorResult& aRv)
> 
> File a followup to implement in workers.

Filed bug 761227.
Depends on: 761479
Attached patch v1 (obsolete) — Splinter Review
It's ready. Well, apart from the two follow-ups (bug 761227, bug 761479).
Attachment #629656 - Attachment is obsolete: true
Attachment #630039 - Flags: review?(jonas)
Attached patch v1.1Splinter Review
rebased, ready to land
Attachment #630039 - Attachment is obsolete: true
Should there be another follow-up bug raised to implement the control that only trusted apps should be able to obtain this permission (instead of the whitelist in preferences)?
https://hg.mozilla.org/mozilla-central/rev/57c5763ac3ac

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment on attachment 630801 [details] [diff] [review]
v1.1

Review of attachment 630801 [details] [diff] [review]:
-----------------------------------------------------------------

drive-by review comments...

::: content/base/src/nsXMLHttpRequest.cpp
@@ +575,5 @@
> +nsXMLHttpRequest::InitParameters(JSContext* aCx, const jsval* aParams)
> +{
> +  XMLHttpRequestParameters* params = new XMLHttpRequestParameters();
> +  nsresult rv = params->Init(aCx, aParams);
> +  NS_ENSURE_SUCCESS(rv, rv);

If params->Init() fails we leak params

@@ +579,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Check for permissions.
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(GetOwner());
> +  NS_ENSURE_TRUE(window && window->GetDocShell(), NS_OK);

If the window or the docshell don't exist we leak params

@@ +588,5 @@
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());
> +    NS_ENSURE_TRUE(doc, NS_OK);
> +
> +    nsCOMPtr<nsIURI> uri;
> +    doc->NodePrincipal()->GetURI(getter_AddRefs(uri));

You should use GetOrigin() instead which handles digging into the innermost URI and punycoding and cases when the URI doesn't match the origin. Or URIIsChromeOrInPref() should take a principal and have that done in there, or add another helper to nsContentUtils (like ::GetAsciiOrigin) that does the right thing for you.

@@ +596,5 @@
> +    }
> +  }
> +
> +  mIsAnon = params->mozAnon;
> +  mIsSystem = params->mozSystem;

looks params is leaked no matter what
Depends on: 763814
(In reply to Paul Theriault [:pauljt] from comment #61)
> Should there be another follow-up bug raised to implement the control that
> only trusted apps should be able to obtain this permission (instead of the
> whitelist in preferences)?

Yes. We'll have to do this for pretty much every API.

(In reply to Daniel Veditz [:dveditz] from comment #63)
> drive-by review comments...

Thanks! Ouch, leaks. Filed bug 763814.

> > +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());
> > +    NS_ENSURE_TRUE(doc, NS_OK);
> > +
> > +    nsCOMPtr<nsIURI> uri;
> > +    doc->NodePrincipal()->GetURI(getter_AddRefs(uri));
> 
> You should use GetOrigin() instead which handles digging into the innermost
> URI and punycoding and cases when the URI doesn't match the origin. Or
> URIIsChromeOrInPref() should take a principal and have that done in there,
> or add another helper to nsContentUtils (like ::GetAsciiOrigin) that does
> the right thing for you.

Thanks for pointer! Will be sure to take that into account when we rewrite this to use the permissions manager.
Depends on: 765468
This sounds important to verify; is there a clear QA-reproducible testcase that
we can run through, and if so, can you please change [qa?] to [qa+] in the
whiteboard status, with steps?  Thanks!
Whiteboard: [sec-assigned:curtisk:749372] → [sec-assigned:curtisk:749372][qa?]
(In reply to Stephen Donner [:stephend] from comment #65)
> This sounds important to verify; is there a clear QA-reproducible testcase

User-facing? No. You could, of course, write some HTML test cases and verify them manually. The mochitests that were added as part of this patch do this in an automated fashion.
Depends on: 768804
Flags: sec-review?(dveditz)
Whiteboard: [sec-assigned:curtisk:749372][qa?] → [sec-assigned:curtisk:749372], [qa-]
Flags: sec-review?(dveditz) → sec-review?(ptheriault)
Whiteboard: [sec-assigned:curtisk:749372], [qa-] → [qa-]
Depends on: 788369
Can somebody please summarize what has changed on the JS/web side with this patch? E. g. a description of the new parameters that can be passed to XHR? Docs are still needed …
This adds the ability to specify two parameters when constructing an XHR object:

new XMLHttpRequest({ anon: true, system: true });

Both are optional and default to false.


new XMLHttpRequest({ anon: true });

is equivalent to the AnonXMLHttpRequest from the XHR spec [1]. So far noone has implemented the AnonXMLHttpRequest constructor, and the anon flag received positive feedback when I suggested it on the list.


new XMLHttpRequest({ anon: true, system: true });

is *only* available in reviewed (privileged) apps. The 'system' flag may only be set to true if the 'anon' flag is also set to true. When 'system' is set to true, it allows the XHR object to make cross-site requests without using CORS.

Again, this is *only* available to reviewed apps, and can't be used in web pages opened in the Firefox browser since it wouldn't be safe.


[1] http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html
Thank you. I just amended the MDN article: https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest#XMLHttpRequest%28%29

Is this enough as documentation?
I clarified the docs a little bit. Please note that CORS didn't remove any functionality, it just added functionality. This adds further functionality.
Thank you. 

One question is left: In which Firefox version will this arrive? I used Firefox 18 although this bug targets Milestone 16. 

I guess Firefox 18 (??) for developers [1] should also be amended?

[1] https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers
So, just to clarify ... there will be no third-party podcatcher for Firefox OS (will be there a default one at least)? Negative reply would sound pretty bad for me, as listening to podcasts is for me (and I guess I am not the only one) the most often activity on my phone. Transfer of multimegabyte media files through a server seems like a good way how to eliminate all independent developers. Is there any better bug for this?
writing a podcast app is completely unrelated to this bug. Please file a separate bug on that. This bug is fixed.
(In reply to Jonas Sicking (:sicking) from comment #73)
> writing a podcast app is completely unrelated to this bug. Please file a
> separate bug on that. This bug is fixed.

Filed as bug 809280
Flags: sec-review?(ptheriault) → sec-review+
Attached patch ClickerSplinter Review
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?
Attachment #8675438 - Flags: ui-review+
Attachment #8675438 - Flags: superreview?
Attachment #8675438 - Flags: sec-approval?
Attachment #8675438 - Flags: review+
Attachment #8675438 - Flags: qa-approval?
Attachment #8675438 - Flags: feedback-
Attachment #8675438 - Flags: checkin+
Attachment #8675438 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8675438 - Flags: approval-mozilla-release?
Attachment #8675438 - Flags: approval-mozilla-esr38?
Attachment #8675438 - Flags: approval-mozilla-esr31?
Attachment #8675438 - Flags: approval-mozilla-beta?
Attachment #8675438 - Flags: approval-mozilla-b2g37?
Attachment #8675438 - Flags: approval-mozilla-aurora?
Attachment #8675438 - Flags: a11y-review-
[Tracking Requested - why for this release]:

[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.2?
blocking-basecamp: + → ?
blocking-kilimanjaro: + → ---
Attachment #8675438 - Flags: ui-review+
Attachment #8675438 - Flags: superreview?
Attachment #8675438 - Flags: sec-approval?
Attachment #8675438 - Flags: review+
Attachment #8675438 - Flags: qa-approval?
Attachment #8675438 - Flags: feedback-
Attachment #8675438 - Flags: checkin+
Attachment #8675438 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8675438 - Flags: approval-mozilla-release?
Attachment #8675438 - Flags: approval-mozilla-esr38?
Attachment #8675438 - Flags: approval-mozilla-esr31?
Attachment #8675438 - Flags: approval-mozilla-beta?
Attachment #8675438 - Flags: approval-mozilla-b2g37?
Attachment #8675438 - Flags: approval-mozilla-aurora?
Attachment #8675438 - Flags: a11y-review-
You need to log in before you can comment on or make changes to this bug.