Closed Bug 756173 Opened 10 years ago Closed 9 years ago

XMLHttpRequest sandbox issues

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: mixedpuppy, Unassigned)

References

Details

Attachments

(1 file)

We're hitting multiple problems in making XHR available to the frameworker sandbox.  

1. on osx, it seems like the iframe doesn't always have XHR, so we are unable to add it to the sandbox.
2. we're having to hack the XHR class to make it work (when it is avialable):

workerWindow.XMLHttpRequest.__exposedProps__ = [];

3. if we get past the above issues, then XHR only works with CORS enabled servers, it will not communicate back to the principle server the sandbox is created with.
more background.  we're NOT using the iframe window as the sandboxPrototype and are trying to import specific functions and classes into the sandbox.  We're doing this in order to have a sandbox that roughly matches a web worker.  We cannot use a real worker because we need access to several APIs not available to the fx implementation of workers.

One fix is to simply give up on that, and use the iframe window as the sandboxPrototype.  This fixes our XHR problem, but also introduces the possibility that we will not be able to switch over to worker in the future.
Some more information, please. What is a frameworker sandbox?
A testcase attached to this bug would be good.
Attaching a browser test case that demonstrates the problem.  At the risk of telling people how to suck eggs, once applied the test can be run from your obj-dir by executing:

% make -C dom/base && TEST_PATH=dom/base make -C . mochitest-browser-chrome

This test simulates what our "frameworker" does - it creates a hidden window attached to an origin, then creates a sandbox based on that window and evaluates code in that sandbox (the "worker" simulation)

It demonstrates 2 problems:

* There is a line:
  workerWindow.XMLHttpRequest.__exposedProps__ = [];

  which is necessary to get XMLHttpRequest working at all - without this we end up with a wrapper that fails to grant access to the object, so "new XMLHttpRequest()" returns undefined.

* If this hack is in place, XMLHttpRequest *almost* works, but doesn't respect the same-origin policy.  The code works as expected if you change the url to, say "http://enable-cors.org/" as that server supports CORS.  We expect it to work as written as it is attempting to fetch the exact same URL as the window has so should be in the same origin.
I meant to add - the test fails with:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/base/test/browser_xhr_sandbox.js | check the sandbox code was happy - Got ERROR: got request status of 0, expected ok

indicating the XHR completed with req.status == 0, which has all the hallmarks of being rejected due to lack of CORS headers.
> * There is a line:
>   workerWindow.XMLHttpRequest.__exposedProps__ = [];


This seems to only work on windows, because, if you create multiple "frameworkers", XMLHttpRequest is not even available after the first.
(In reply to Mark Hammond (:markh) from comment #3)

> * There is a line:
>   workerWindow.XMLHttpRequest.__exposedProps__ = [];


> * If this hack is in place, XMLHttpRequest *almost* works, but doesn't
> respect the same-origin policy.  The code works as expected if you change
> the url to, say "http://enable-cors.org/" as that server supports CORS.  We
> expect it to work as written as it is attempting to fetch the exact same URL
> as the window has so should be in the same origin.

Wait, is the worker being attached to a chrome window or to a content window? __exposedProps__ only makes sense for chrome objects being exposed to content.

More importantly, this hack doesn't make much sense to me:
1 - COWs are always callable anyway, regardless of __exposedProps__
2 - This code effectively grants zero permissions.

Also, FWIW, __exposedProps__ is supposed to be an object, not an array.
(In reply to Bobby Holley (:bholley) from comment #6)
> (In reply to Mark Hammond (:markh) from comment #3)
> 
> > * There is a line:
> >   workerWindow.XMLHttpRequest.__exposedProps__ = [];
> 
> 
> > * If this hack is in place, XMLHttpRequest *almost* works, but doesn't
> > respect the same-origin policy.  The code works as expected if you change
> > the url to, say "http://enable-cors.org/" as that server supports CORS.  We
> > expect it to work as written as it is attempting to fetch the exact same URL
> > as the window has so should be in the same origin.
> 
> Wait, is the worker being attached to a chrome window or to a content
> window? 

content window.  

> __exposedProps__ only makes sense for chrome objects being exposed
> to content.
> 
> More importantly, this hack doesn't make much sense to me:
> 1 - COWs are always callable anyway, regardless of __exposedProps__
> 2 - This code effectively grants zero permissions.
> 
> Also, FWIW, __exposedProps__ is supposed to be an object, not an array.

until Mark changed exposedProps, "new XMLHttpRequest()" returned undefined.

Once he changed it, he got back a request object that ONLY worked with CORS enabled.  The hack shouldn't make sense, but it works around a bug.
Did you try ´new somewindowobject.XMLHttpRequest();´ ?
(In reply to Olli Pettay [:smaug] from comment #8)
> Did you try ´new somewindowobject.XMLHttpRequest();´ ?

the window is intentionally not available to the sandbox, so no, we cannot do that.
Depends on: 760076
I finally looked into the __exposedProps__ stuff. For some inexplicable reason, the wrapper code treats |construct| differently from |call|. I'm going to see if I can fix that.
Also, sorry this took so long - I've been pretty swamped these days :-(
(In reply to Bobby Holley (:bholley) from comment #11)
> Also, sorry this took so long - I've been pretty swamped these days :-(

No problem - thanks for getting back to it!

Note the the __exposedProps__ thing is annoying, but is a work-around.  The more pressing issue from our POV is what seems to be the same-origin problems that happen trying to get the data after we've made the work-around :)
(In reply to Mark Hammond (:markh) from comment #12)
> Note the the __exposedProps__ thing is annoying, but is a work-around.  The
> more pressing issue from our POV is what seems to be the same-origin
> problems that happen trying to get the data after we've made the work-around
> :)

That's probably something you're going to want smaug/gabor's help with.
__exposedProps__ is not really a workaround, since it only fixes things on windows.   Touching exposedProps on osx caused a js exception.
(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> __exposedProps__ is not really a workaround, since it only fixes things on
> windows.   Touching exposedProps on osx caused a js exception.

Fix just landed FWIW.
So part 5 of bug 734891 will add an optional XHR constructor to sandbox, and I will soon land the patch, but it will still take some time util it lands. On the other hand this is a case that I think jetpack folks already solved so I CC Alex.

And just out of curiosity, if you try creating the XHR first and export that to the sandbox instead of the constructor... does that change anything? I just don't know if the constructor when exported will be bound to the window object or not, and if not maybe the XHR object will be initialized differently...
We are doing different things in Jetpack. We are using sandboxPrototype and the sandbox global object is a proxy mapping to the content window.

Having said that, I'm wondering if there is any difference between:
  sandbox.importFunction(workerWindow.XMLHttpRequest, "XMLHttpRequest");
and
  sandbox.XMLHttpRequest = workerWindow.XMLHttpRequest;
or
  var sandbox = Cu.Sandbox(workerWindow, {XMLHttpRequest: workerWindow.XMLHttpRequest});
?

(we are closer to the last pattern in jetpack)

Finally, I was wondering which XHR contructor is going to be called:
- nsXMLHttpRequest::Init()
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#488
- nsXMLHttpRequest::Init(nsIPrincipal* ,nsIScriptContext*,
                         nsPIDOMWindow*, nsIURI* aBaseURI)
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#529
- nsXMLHttpRequest::Initialize(nsISupports*, JSContext*, JSObject*,
                               PRUint32 , jsval*)
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#545

I'd imagine that this bug is related to one of these methods fetching an unexpected global window or script principal?
We are used to face similar bugs when using multiple DOM features in sandboxes, where it is always expected that the global object is nsIPDOMWindow:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#517
(for example, bug 733035)
importFunction is unnecessary since FF4.
(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > __exposedProps__ is not really a workaround, since it only fixes things on
> > windows.   Touching exposedProps on osx caused a js exception.
> 
> Fix just landed FWIW.

I just ran a test against this fix, I still see the same issue.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> And just out of curiosity, if you try creating the XHR first and export that
> to the sandbox instead of the constructor... does that change anything? I
> just don't know if the constructor when exported will be bound to the window
> object or not, and if not maybe the XHR object will be initialized
> differently...

I tried three different things with various levels of fail.

let s = Cu.Sandbox(iframe.contentWindow);

// #1 XMLHttpRequest is undefined in the sandbox
s.XMLHttpRequest = iframe.contentWindow.XMLHttpRequest;

// #2 throws exception
s.xhr = new iframe.contentWindow.XMLHttpRequest();

// #3 returned value is undefined
s.getXHR = function() {
 return new iframe.contentWindow.XMLHttpRequest();
}

It seems the only way I can make this work is if iframe.contentWindow is the proto for the sandbox.
(In reply to scaraveo from comment #19)
> I just ran a test against this fix, I still see the same issue.

Do you mean that the __exposedProps__ = {} hack is still necessary? Or that the test still fails? The latter is expected, the former would be very surprising.
For me, it's the latter.  It looks like bug 734891 is what we now need.
ochameau helped me find a solution, using iframe.contentWindow.wrappedJSObject fixes this issue:

let Cu = Components.utils;

let proto = {
  XMLHttpRequest: content.wrappedJSObject.XMLHttpRequest,
  location: String(content.location)
};
let s = Cu.Sandbox(content, {sandboxPrototype: proto});

Cu.reportError(Cu.evalInSandbox("var xhr = new XMLHttpRequest(); xhr.open('GET', location, false); xhr.send(); xhr.responseText", s));
So it looks like the real issue is that XMLHttpRequest disappeared from window object xraywrappers, but not all xraywrappers ... only xrays made for system principal. It still works fine for xrays made for content principal.

As always, here is a scratchpad test case:
  var Cu = Components.utils;
  var s = Cu.Sandbox(content, {sandboxPrototype: {window: content}});
  Cu.reportError("from chrome: " + content.XMLHttpRequest);
  Cu.reportError("from content: " + Cu.evalInSandbox("window.XMLHttpRequest", s));
  // Just to ensure that we have an xray. Yes, we have one!
  Cu.reportError("isxray: " + Cu.evalInSandbox("window.wrappedJSObject", s));

content.XMLHttpRequest is undefined from browser.xul context,
but still works fine for window.XMLHttpRequest, in sandbox context.
That difference explain why Jetpack is still working fine whereas Shane and Mark are facing issues.
How do we attach new DOM binding constructors to the global object? One possibility is that they're not visible to Xray wrappers.
> How do we attach new DOM binding constructors to the global object

Just via the resolve hook, iirc.  See also bug 741267.  I guess the tests there don't actually use xrays?
Blocks: 740069
Blocks: 762569
I'm going to close this, we've solved our problem by:

win = iframe.contentWindow.wrappedJSObject;
s = Cu.Sandbox(win);
s.XMLHttpRequest = win.XMLHttpRequest;

This is part of the implementation in bug 762569.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.