601295 - Make worker sandboxes non-privileged, but compatible with jQuery and other frameworks without leaking objects to document

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Few devs reported that current implementation of PageMods leaks objects to the pages global scope. It seems to happen because frameworks like jQuery use window object to expose their public API.  

http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/8c7be66270a3dfbe?hl=en

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

After looking at this issue I see two possible options to solve it:

1) setting __proto__ of the sandbox to window instead of window.wrappedJSObject, but for some reason on FF 3.* window in sandbox still points to window.wrappedJSObject

2) We can expose decadents of the window and document instead:

sandbox.__proto__ = { window: { __proto__: window }, document: { __proto__ window.document }, __proto__: window }

This approach works fine with all versions of FF but it's impossible to port all of the current tests.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 3

[Alexandre Poirot [:ochameau]]

This bug is still true.
I've built a testcase to confirm that jquery works in pagemod,
and this testcase check for this scope polluting:
https://bugzilla.mozilla.org/attachment.cgi?id=521231

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment for bug triage: I think it's important to have this fixed for 1.0 as some addons may interfere with the page's js and harm firefox experience.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Use wrappers in content script and remove principal privileages

The main issue here is that content script globals are merged with document one.
That's because we set sandbox prototype to unwrapped window object.

There is at least two way to fix this:
1/ set a wrapped window as prototype
2/ do not set prototype but add all necessary document globals in content script one (window, document, navigator, ...)

I choose to implement the first solution as it add an important and common security layer. I tried to follow same name convention than greasemonkey:
- window, document, navigator and all are wrapped. you can't be trapped by overloaded DOM, but you can't access to document custom javascript functions/values,
- unsafeWindow, is an equivalent of window.wrappedJSWindow and allow to access to document JS with an explicit warning that it is unsafe
http://wiki.greasespot.net/UnsafeWindow

So you can easily import jQuery and others frameworks without polluting document. As JS values set to window are not replicated to document globals.
This is the key difference with the second solution where jquery is going to leak to the document.

The only problem I see with this approach is that it throw exception when you try to set a function listener as an attribute, like domNode.onkeyup = function(){}. This prevent using old jQuery version that are known to fail in greasemonkey. That's why I've upgraded jquery in examples and replace onkeyup by addEventListener in annotator example. 

It fix bug 636147 as I had to replace chrome privileages by document domain principal.

Finally, I added a parameter to worker to remove these wrappers in page-worker case, where we trust and control the target document.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Patch with example update

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 526698 [details] [diff] [review]
Patch with example update

Irakli: you may have some good insight on this bug as you start working on bug 636147.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

One issue I noticed when using `wantXrays: true` is that element.onevent property get / set throws exceptions like following:

Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: /Users/gozala/Projects/addon-sdk/packages/jetpack-test4jquery/data/jquery/test/unit/event.js :: <TOP_LEVEL> :: line 721"  data: no]

This breaks test for the latest version of jQuery.

Comment 9

[Alexandre Poirot [:ochameau]]

This is a well known limitation in greasemonkey community:
https://developer.mozilla.org/en/XPCNativeWrapper#Limitations_of_XPCNativeWrapper
http://www.oreillynet.com/pub/a/network/2005/11/01/avoid-common-greasemonkey-pitfalls.html?page=1

And here is some interesting doc on all possible limitations:
https://developer.mozilla.org/en/XPCNativeWrapper#Limitations_of_XPCNativeWrapper

Comment 10

[Myk Melez [:myk] [@mykmelez]]

I'm all in favor of fixing this bug, but XPCNativeWrappers place too many restrictions on the kinds of interactions content scripts can have with content, leading to problems like the one Irakli noted with jQuery.

Also, note that content wrappers have evolved since Greasemonkey was written.  In particular, XPCNativeWrapper.wrappedJSObject now returns an XPCSafeJSObjectWrapper rather than a completely unwrapped object, which guarantees that content code will run with content privileges (although it will still be possible for content to override DOM properties and methods).  More info:

  https://developer.mozilla.org/en/XPConnect_wrappers
  https://developer.mozilla.org/en/XPConnect_security_membranes

So I think we should employ option 2 from comment 1/comment 5: define `window`, `document`, and the like in the content script, making them objects whose prototypes are the "unwrapped" (really XPCSafeJSObjectWrapper-wrapped) content globals, so when frameworks set their properties, they set them on the content script globals rather than the content globals.

This is similar to the model employed by Chrome as described in Aaron Boodman's Content Scripts in Chromium blog post <http://www.aaronboodman.com/2009/04/content-scripts-in-chromium.html>, except that Chrome hides content changes to the `window` content global from content scripts, and it prevents content scripts from accessing the `window` content global via walking the prototype chain.

But it does make the `window` content global available via the `contentWindow` content script global.  And it doesn't (didn't?) prevent content scripts from accessing the values of DOM properties set by content (f.e. document.body.onclick).

I can imagine shifting more toward a Chrome approach over time.  I can also imagine using JS proxies to wrap the properties of content globals in a mix of XPCNativeWrapper and XPCSafeJSObjectWrapper, depending on the property.  But those approaches seem out of scope or too risky and uncertain for this bug at this time.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 526698 [details] [diff] [review]
Patch with example update

Cancel feedback request as Myk already commented on the desired approach.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Unit test to check sandbox scopes, privileages, leaks and visibility

First, here is a usefull unit test in order to check easily a sandbox against all behaviors we want in a content script. Like avoid document JS pollution, avoid accessing document javascript by default to avoid security issues, ...

I implemented 3 possible ways of doing such sandbox:
1/ building global and window objects and avoid "Content script to Document" pollution using window prototype. There is many security issue in this implementation.
2/ using XPCNativeWrapper passed all tests except reading "onevent" attributes on DOM nodes. We get an exception.
3/ A mix of XPCNativeWrapper and Proxy. It seems to be working but building Proxies of DOM Nodes is far from being easy. These proxy are here mainly to catch exception of case 2/.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Method 1: Use prototype

So here is a first patch against worker.js that use prototype to secure scopes.
There is only XPCSafeObjectwrapper involved in this method. The main problem here is that these wrapper doesn't help managing different scope as these objects are exactly the same as document javascript ones. If you set a JS attribute it will end up in the document. And it doesn't solve any security issues when the document tries to override DOM native functions (document.getElementById for ex).
There may be some additional tricks to add some more security on `document`, but we won't be able to have the same security level to any abitrary node.
For example we are going to be trapped by:
document.getElementById("foo").setAttribute("value", "xxx");
`setAttribute` can be overriden and we will call this evil function.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Method 2: Use XPCNativeWrapper

Here we are using XPCNativeWrapper and wantXrays attribute from sandbox in order to provide a clean security layer and different scopes.
Drawbacks of this method are:
- we have totally different JS scopes, if we need to access to JS value of the document we have to do it explicitely, through `unsafeWindow` or `window.wrappedJSobject`. But I'd prefer to say that's an asset because of all security issues behind this.
- some DOM capabilities that involves javascript attributes are throwing exceptions with incomprehensible message "Component is not available". Like accessing node.onkeyup. But others features don't work, like: `document.inputID` that allow to get the <input> node with id equal to "inputID".

It seems that all features that are not working with a XPCNativeWrapper are mostly old deprecated HTML features and so we can warn that it doesn't work in content script. But we should work on exceptions that are thrown. We can at least make the message more explicit or returning false instead of throwing.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Method 3: Use Proxy and XPCNativeWrapper

Finally here is an "experience" using a Proxy. This proxy mainly aims to catch exception thrown by XPCNativeWrappers. But building proxies of DOM node is compicated as we can't pass them to native function as they are not working with Proxy objects. (we get NS_NOINTERFACE exception on them)

Comment 16

[Alexandre Poirot [:ochameau]]

Here is some test results against jQuery test:
Method 1: 5 failures
Method 2: 6 failures (the additional failure is "onevent exception"
Method 3: too much failures to count them!

I agree with Godmar and Silmon in bug 607352. 
We have to build such environnement. And it appears that XRayWrapper and XPCNativeWrapper are the kind of wrapper that are the closest to what we want: they ensure that we are calling native methods and they store different JS attributes than document ones.
But there is these JS uses in DOM that doesn't appear to work. So how it is working in other worlds:
- Greasemonkey uses XPCNativeWrapper, so they have this problem but it doesn't prevent using various big JS framework,
- Chrome doesn't offer ANY access to JS values! You have to create a <script> tag with some JS code stringified :o So I can't imagine that they have access to onevent attributes. 
> http://www.aaronboodman.com/2009/04/content-scripts-in-chromium.html
> Future Directions - Idea 1: Completely separate content scripts and page JavaScript
Seems to say that they are(were?) not supporting onevent in content scripts.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #16)
> I agree with Godmar and Silmon in bug 607352. 
> We have to build such environnement. And it appears that XRayWrapper and
> XPCNativeWrapper are the kind of wrapper that are the closest to what we want:
> they ensure that we are calling native methods and they store different JS
> attributes than document ones.

Umm, but Simon does want access to the page-defined `onclick` values of elements, which XPCNativeWrapper/XRayWrapper do not provide.  It isn't entirely clear what Godmar wants, but he seems to want something like Google Chrome's "isolated worlds", which does give addons access to the page-defined `onclick` values of elements, although it isolates content scripts from content in other ways.


> - Greasemonkey uses XPCNativeWrapper, so they have this problem but it doesn't
> prevent using various big JS framework,

How does Greasemonkey make JS frameworks work with XPCNativeWrapper?


> - Chrome doesn't offer ANY access to JS values! You have to create a <script>
> tag with some JS code stringified :o So I can't imagine that they have access
> to onevent attributes.
> > http://www.aaronboodman.com/2009/04/content-scripts-in-chromium.html
> > Future Directions - Idea 1: Completely separate content scripts and page JavaScript
> Seems to say that they are(were?) not supporting onevent in content scripts.

I tried it out today, and Chrome does expose the values of on* properties to addons.


In any case, the question is not so much what Simon and Godmar want or what Chrome does but what we should do to enable addons to modify pages without exposing them to undue security risks.

The primary security concern is content code executing with chrome privileges, for which the current level of wrapping via XPCSafeJSObjectWrapper is sufficient.

A secondary concern is the exposure of certain addon modifications to content, in particular the exposure of jQuery (and other frameworks), which reveals information to websites about the addons a user is using and can conflict with content's own use of frameworks and global variables.  Hence this bug.

A tertiary concern is the ability for content to trick addons into behaving differently (or, in extreme cases, escalating content privileges) through DOM object sharing, for which our first line of defense is the isolation between privileged addon code in CommonJS modules and their unprivileged content scripts.

I'm amenable to addressing these secondary and tertiary concerns, including further isolating content scripts from content via any means at our disposal.  But we can't break popular JavaScript frameworks and other common use cases in the process.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Having said all that, however, I'd be very interested to hear what our Gecko and security hackers think about all this.  Cc:ing bz and dchan for their thoughts.

Comment 19

[Boris Zbarsky [:bzbarsky]]

How ok are we with breaking compat here?

In particular, once on* move into IDL (which is what HTML5 calls for), they will work on XRayWrappers.  So would we do something like use unwrapped objects now but switch to XrayWrappers later?  It'd break compat in the process, obviously...

Comment 20

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #19)
> How ok are we with breaking compat here?
> 
> In particular, once on* move into IDL (which is what HTML5 calls for), they
> will work on XRayWrappers.  So would we do something like use unwrapped
> objects now but switch to XrayWrappers later?  It'd break compat in the
> process, obviously...

We're not very ok with breaking compat.  We'll do it if absolutely necessary, but one of the primary goals of the project is to provide developers with stable APIs that don't break in newer versions of Firefox and the SDK.

Nevertheless, it isn't clear how many addons such a change will actually break.  The dangers of access to page-defined values seem mostly theoretical.  In practice, pages don't typically redefine getElementById or other DOM methods, and addons don't typically access the page-defined values of on* properties (Simon's use case notwithstanding).

And the jQuery issue with on* might be resolved merely by making accessing those properties not throw (bug 607352), although someone would need to audit jQuery code or test it with a fix for that bug to confirm this.

So if an analysis of SDK-based addons determined that very few addons would be broken by such a change, and if we were able to help fix up addons that do get broken, then I might be amenable to introducing XRayWrappers at a later date.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Nevertheless, it isn't clear how many addons such a change will actually
> break.

Well, anything that relied on touching "expando" properties on the window directly would suddenly break if you went from using unwrapped stuff to XrayWrapper, no?  I would bet money this would be a lot of addons....

> The dangers of access to page-defined values seem mostly theoretical.

This is a separate conversation from the "will addons break?" one, right?  The dangers depend on what your extension is doing.

> In practice, pages don't typically redefine getElementById or other DOM
> methods

The question isn't whether pages _typically_ do it.  The question is whether a malicious page could exploit an addon by doing something like this due to the addon assuming it can trust the return value.

Most simply, an addon that uses window.location.href to make any sort of decision can be tricked by a page into seeing any href the page feels like, if you're working with XPCSafeJSObjectWrapper.  XrayWrapper makes sure you at least see the real location of the page.

> addons don't typically access the page-defined values of on* properties

That would be an argument for using XrayWrapper now, except for this jQuery business.

It sounds to me like the right answer here is to use XrayWrapper and make jQuery work somehow.  The other option is either to give addons the XPCWrappedJSObjectWrapper footgun by default and leave it that way in perpetuity....

Do we control the jQuery in question (as in, is it part of the SDK?), or are addons going to be importing it themselves?

Comment 22

[Alexandre Poirot [:ochameau]]

(In reply to comment #21)
> > Nevertheless, it isn't clear how many addons such a change will actually
> > break.
> 
> Well, anything that relied on touching "expando" properties on the window
> directly would suddenly break if you went from using unwrapped stuff to
> XrayWrapper, no?  I would bet money this would be a lot of addons....

I hope that people mainly use addEventListener instead of expando :/
That being said I think that the main breaking change is for addons that willingly access to document JS objects. For example page mod against gmail, facebook or any big web app. Such addons don't play only with DOM but call/read document Javascript.
Gmail expose an API for Greasemonkey:
http://code.google.com/p/gmail-greasemonkey/wiki/GmailGreasemonkey10API
Current you can access this `gmonkey` object through: `gmonkey` global or `window.gmonkey`. But with this bug being fixed you will have to access it through `unsafeWindow.gmonkey`.

> 
> > The dangers of access to page-defined values seem mostly theoretical.
> 
> This is a separate conversation from the "will addons break?" one, right? 
> The dangers depend on what your extension is doing.
> 
> > In practice, pages don't typically redefine getElementById or other DOM
> > methods
> 
> The question isn't whether pages _typically_ do it.  The question is whether
> a malicious page could exploit an addon by doing something like this due to
> the addon assuming it can trust the return value.
> 
> Most simply, an addon that uses window.location.href to make any sort of
> decision can be tricked by a page into seeing any href the page feels like,
> if you're working with XPCSafeJSObjectWrapper.  XrayWrapper makes sure you
> at least see the real location of the page.

Having suffer from security issues while working on Yoono extension, I confirm that we can be easily tricked in a crazy number of different ways. If we take "Method 1" implementation using prototype. We had to replace `window` by sandbox as we want to have attributes set to window appearing in globals. So if we set a function on `window` and a website replace it, then, when we call it from content script `this` variable is going to be `window` i.e. `sandbox`, right?


> 
> 
> Do we control the jQuery in question (as in, is it part of the SDK?), or are
> addons going to be importing it themselves?

Developers imports it themselves.


This particular error has been fixed in jQuery by an unrelated commit:
https://github.com/jquery/jquery/commit/c1dcad69427b78f3e70628ab1444f42033ee593a#L0L207
(This code is now executed only on IE)

This bug is a well known one in jquery/greasemonkey community:
http://bugs.jquery.com/ticket/5833
https://github.com/greasemonkey/greasemonkey/issues/1104

(In reply to comment #16)
From my previous comment, test results tell that in giant jquery unit test, there is only one test that is failing with "Component not available" exception because of on* attribute access. We can easily submit a patch to jquery community to fix this one.
Then we have 5 tests that are failing whatever we are using XrayWrapper or XPCSafeJSObjectWrapper.

Irakli: Do you have an idea why they are failing? Were you able to have 100% success running these tests?

I will test against other frameworks this week.

Comment 23

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> 
> Irakli: Do you have an idea why they are failing? Were you able to have 100%
> success running these tests?
> 

No I never got 100%, actually I noticed that many test break just by running jQuery them form `resource:` protocol due to false assumptions like following:
https://github.com/jquery/qunit/blob/master/qunit/qunit.js#L475

I worked around some of them by a hack:
https://github.com/Gozala/jetpack-test4jquery/blob/master/data/config.js

But it's likely that remaining failures are also cause by other similar falsy assumptions in test.

Comment 25

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

After discussing this with Alex, I marked Bug 636147 as a duplicate. I wonder if we should move Whiteboard comment of it here ?

Comment 26

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I'm refining title of this bug, and also marking bug 616946 as duplicate, since making sure that jQuery works is part of this bug anyway.

Comment 28

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: jetpack package for running official jquery tests in page-mod

Adding pointer to the jetpack package I've created for running official jquery tests in page-mod

Comment 29

[Alexandre Poirot [:ochameau]]

Attachment: Frameworks unit test

I checked following frameworks against methods 1 and 2 (with/without XPCNativeWrapper): JQuery, Mootools, Prototype, RightJS and jquery UI.
I didn't check all features but mainly core ones.
Without XPCNativeWrapper, everything went fine.
With XPCNativeWrapper, mootools, prototype and jquery UI need to be patched.

# Mootools:
You need a one line fix to make it work, use of `onreadystatechange`.
https://github.com/mootools/mootools-core/blob/master/Source/Utilities/DOMReady.js#L74
> if ('onreadystatechange' in document) document.addListener('readystatechange', check);

# Prototype:
It highlights another specificity of XPCNativeWrapper. HTML elements classes don't have `prototype` attribute set. For ex: `HTMLFormElement.prototype` is null.
Again, a one line fix is needed:
https://github.com/sstephenson/prototype/blob/master/src/prototype/prototype.js#L125
- < if (typeof window.HTMLDivElement !== 'undefined')
+ > if (typeof window.HTMLDivElement !== 'undefined' && typeof window.HTMLDivElement.prototype !== 'undefined')

# jQuery UI
Fails on some cases as they are using on* attribute in html string:
https://github.com/kzys/jquery-ui/blob/f696948fc41c09a78f139af6e47972ff1c483123/ui/jquery.ui.datepicker.js#L1409

Comment 30

[Boris Zbarsky [:bzbarsky]]

How much would it help if we just made on* work for XPCNativeWrapper (in Firefox 6, say)?

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: New unit test that check sandbox scope, privileage, leak and visibility

This unit test:
1/ ensures that sandboxes satisfy our needs in term of scope and security, 
2/ highlights all known issues using XRayWrappers/XPCNativeWrappers

So Method 1 using prototype mainly fails on part 1/, whereas Method 2 using Xraywrappers obviously fails on part 2/.

To resume, I analysed deeply all errors from jquery unit tests: 
Method 1 is failing only on tests that inject JS thought <script> tag. (jQuery test expect that script is executed in same context)
Method 2 is failing on:
 - 2 tests about lack of on* attributes
 - one test where <form name="myForm"> is not available through `document.myForm`
 - lot of failures due to a bug with mozMatchSelector:
>> var mozMatchSelector = document.documentElement.mozMatchSelector
>> mozMatchSelector.call( node, expr ); fails when using Xraywrappers.
Used there:
  https://github.com/jquery/sizzle/blob/master/sizzle.js#L1236
Reported here:
  https://github.com/greasemonkey/greasemonkey/issues/1300
 - one test fails as we can't override native methods, even on the wrapper itself:
>> div[0].detachEvent = div[0].removeEventListener = function(t){
>>   ok( true, type + " Outer " + t + " event unbound" );
>> };
This doesn't throw error, but won't be called either if we do: 
  div[0].removeEventListener(...)
 - one last test is failing because iframe.contentWindow is wrapped, even with same domain, but it may be something that is due to wantXRays option?

All these failures are confirmed in this unit test.

That being said, I still think that such wrapper is what we should have. No matter if it is a XrayWrapper, XPCNativeWrapper, a proxy thing or anything new. May be existing wrappers really don't intend to provide such set of features/behavior.
The final idea is quite simple: having wrappers that hold their own set of attributes, and so do not share any raw JS value with the document in any way, and finally have same capabilities than regular document object.

Now, we have roadmaps. We have to fix current content script implementation before 1.0. Whatever we choose now, we may break some extensions on 1.0 release. But if we choose method 1 now and we end up using cleaner wrapper in future, we are going to break then. If we choose method 2 now, we are going to have limitation/known issues now that hopefully will be solved in next firefox releases.

Do you think it make sense to report all bugs/limitations I faced with xraywrappers individually ?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> having wrappers that hold their own set of attributes, and so do not share
> any raw JS value with the document in any way

That's XrayWrappers.

> and finally have same capabilities than regular document object.

That only works if none of those capabilities depend on raw JS values.

> Do you think it make sense to report all bugs/limitations I faced with
> xraywrappers individually ?

Yes, if not already reported.

I'd really like an answer to my question from comment 30.  Getting that done for Fx6 is getting harder every day.  Getting it done for Fx7 is quite doable.  What does your timeframe look like for 1.0?

Comment 33

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #32)
> I'd really like an answer to my question from comment 30.  Getting that done
> for Fx6 is getting harder every day.  Getting it done for Fx7 is quite
> doable.  What does your timeframe look like for 1.0?

We plan to ship SDK 1.0 in June, and we intend for it to support Firefox 4, so we're looking for a way to fix this problem in Firefox 4 and up.

I can imagine shipping SDK 1.0 with less complete isolation, racheting up the isolation in later versions as we land any necessary platform fixes in Firefox.

(We should then carefully document the current state of affairs as well as our intended future changes, do everything in our power to avoid breaking changes, and do everything in our power to help addon developers cope with breaking changes we can't avoid.)

I can't imagine shipping 1.0 with more complete isolation but without support for using jQuery and other popular JavaScript frameworks in content scripts.

Comment 34

[Boris Zbarsky [:bzbarsky]]

In that case, the only way you can ship 1.0 is with an insecure "window" object as described in comment 21, unless I'm missing something.  The on* thing just won't work with XrayWrapper (certainly not in Fx4!) and it sounds like the various libraries involved depend on it.

Comment 35

[Boris Zbarsky [:bzbarsky]]

There's another long-shot possibility...  Firefox 4 _did_ ship scriptable proxies, though there were some bugs.  It may be possible to create a scripted proxy that forwards everything to the XrayWrapper for a window except the on* properties, which it forwards to the XPCSafeJSObjectWrapper for that window.  Andreas, Blake, is that feasible?

Comment 36

[Myk Melez [:myk] [@mykmelez]]

Alex tried using a proxy (see comment 15 and comment 16), although he ran into problems.  But his proxy used XPCNativeWrapper rather than XRayWrapper; maybe that would make a difference?

Comment 37

[Boris Zbarsky [:bzbarsky]]

No, that would be the same thing.  The proxy impl is proof-of-concept, not complete; it would definitely need more work.  I'm just saying that's your best shot for getting something more or less guaranteed secure on fx4.  :(

Comment 38

[Alexandre Poirot [:ochameau]]

Attachment: Method3: Use Proxy over Xraywrappers - pull request 171

I took some time to build real implementation of such proxy. Having Blake in the Paris office helped me. And having Andreas coming this week could lead to even more cool stuff on this topic!

So after having faced multiples xraywrappers bugs (658909, 658560), and having done various translations of nsDOMClassInfo.cpp to content-proxy.js, I think these wrappers are in a really good shape.

I've added unit test on each bug I faced during their implementation in test-content-proxy.js. RightJS, Mootools, Prototype and jQuery are working.
big jQuery unit tests are green except one test.

This failing exception is about 'toString' on <object> elements:
  let obj = document.createElement("object");
  let proxy = Proxify(obj);
  Object.prototype.toString.call(obj) -> [object HTMLObjectElement];
  Object.prototype.toString.call(proxy) -> [object Function];
  proxy.toString() -> [object HTMLObjectElement];

I tried to overload Object.prototype.toString without success.
I don't think this is very important as framework should not be doing such assumption. This only occurs when using Object.prototype.toString, i.e. toString method on the node itself works correctly.

Andreas: It could be really helpfull to have your thoughts about such project. I asked for you feedback here, but I'll ping you IRL during your visit in Paris office.

Comment 39

[Alexandre Poirot [:ochameau]]

Myk: I asked for your feeback too, sort of "pre-review" in order to be ready to land this quickly and avoid delaying rc1 because of this bug.

Comment 40

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 534336 [details]
Method3: Use Proxy over Xraywrappers - pull request 171

This seems like a reasonable strategy to me. f=myk

There are still some issues to address, however, primarily that a bunch of tests are failing.

One is in test-content-proxy.testProxy, which complains <object> are HTMLObjectElement ("[object Function]" != "[object HTMLObjectElement]").

Others are in addon-kit tests, like test-tabs.testAttachWrappers, which complains Worker has access to javascript content globals (0) ("globalJSVar is not defined" != true).

That seems like a test just needs updating.  But others aren't as obvious, such as test-widget.testWidgetViewsUIEvents, which complains event first argument is equal to the WidgetView.

Comment 41

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 171: Add `unsafeWindow`and fix tests

I pushed two new commits to the pull request (a8309e9, 327962d7).

First, I added `unsafeWindow` in content script globals that gives access to unwrapped document objects. That allowed me to fix all failing tests.
(Widget tests were failing because of another test failing before it)

Then, I disabled content proxy test about Object.prototype.toString(<object> node). As said in comment 38, I do not have a working fix about this particular difference between proxies and regular dom objects.

Comment 42

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 536115 [details]
Pull request 171: Add `unsafeWindow`and fix tests

Alex and I discussed this patch today and agreed that we should not implement unsafeWindow at this time and revisit the feature in the future when we have time to identify the specific use cases and design the best feature to address them.

So tests that require unsafeWindow should be disabled or removed.

I haven't otherwise reviewed the code but will do so soon.  In the meantime, this code could benefit from additional review by Irakli, given that it makes a significant change late in the cycle, and because Irakli is familiar with the code in question.  Thus requesting review from Irakli as well.

Comment 43

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 536115 [details]
Pull request 171: Add `unsafeWindow`and fix tests

I've commented on some issues (mostly nits) in the pull request.

Comment 44

[Alexandre Poirot [:ochameau]]

Comment on attachment 536115 [details]
Pull request 171: Add `unsafeWindow`and fix tests

I've added a new wrapped tests and commented out unwrapped tests.
myk: I've addressed all your comments except one. 
I let console.warn next to throw as it is common to do not catch exception and log them why using addEventListener, for ex. Library code doesn't add try/catch, so the error is never catched nor logged.
So I thought that adding a warning message would help seing these errors.

Comment 45

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 536115 [details]
Pull request 171: Add `unsafeWindow`and fix tests

I made a few more comments in the pull request. r=myk with those comments addressed.

Comment 46

[Alexandre Poirot [:ochameau]]

Comments adressed.

Btw, I didn't created a bug about this:
https://github.com/mozilla/addon-sdk/pull/171#r37490
(undefined error on HTMLCollection)

As I wasn't able to reduce this error to something that ensure platform being buggy. But I recently acquired some skills using gdb on firefox, so I may end up being able to know what's wrong behind this.

Comment 47

[Myk Melez [:myk] [@mykmelez]]

A second review would be helpful, but it isn't absolutely necessary. a=myk for landing during the freeze.

Comment 48

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/6bd222f96e3f737e45e4b094182091cf7acf671f
I didn't changed status to RESOLVED FIXED as we may still make a second review cycle on it? Or do we open another bug for that?

Comment 49

[Myk Melez [:myk] [@mykmelez]]

Irakli can do the second review here, even if we resolve the bug.  Any issues he identifies will have to be addressed in new bugs, though.

Since this is landed, and the issue has been addressed, this bug should be resolved fixed.

Comment 50

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 536115 [details]
Pull request 171: Add `unsafeWindow`and fix tests

Don't know if I should put r- here, but I do it in order to identify that I think there are still few issues to be addressed. Details see in pull request, and in summary it is:

1. You should not trust built in methods of objects that you do not create as they may be overridden for diff reasons. Instead you should always use Object.prototype.* or Function.prototype.* etc..
2. You always need to delegate to the original methods even if you think they are build-in one as they may be overridden: https://github.com/mozilla/addon-sdk/pull/171#r39364
3. You need to make small fixes to make sure your internal properties can't be compromised (overriding __proxy will allow attacker to get access to things you're trying to restrict access to): https://github.com/mozilla/addon-sdk/pull/171#r39371
4. Diff suggestions to minimize complexity of the code like removing __isWrappedProxy etc..
5. Suggestions on improving code readability and maintainability.

Please not that first three are important ones IMO.

Also once again thanks to Alex for big chunk of good work!