679363 - Content script contexts are shared between addons

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

Attachment: Unit test that highlight this issue

As XrayWrappers are singletons, content script proxies end up sharing attributes between multiple addons.
So if a content script do:
  document.foo = {}
This `foo` attribute will be visible from another content script.

For the moment I don't see any JS work around for this issue.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 231

Such change allow to have different sets of xraywrappers.
Currently content script proxies are created in content-proxy module,
this module is unique per addon so that all page-mods in one addon would have only one set of xraywrappers.
Such behavior leads to shared attributes between page-mod matching the same document.

It appears that moving content-proxy module to a specific sandbox solve this issue, as it will have his own set of xraywrapper.
But this move has another important benefit and was suggested by Andreas.
Any object returned by the proxy code is currently coming from a priviledge scope, so that content script may gain priviledge code access through proxies.
For ex, the following code would allow to overload `push` method of native Array, from a content script:
Object.keys(proxyObject).__proto__.push = function (v){});

Finally, the last advantage of this change is for tabs in remote processes. It will be easier to evaluate this proxy code in content processes as it is no longer a common js module.

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 555206 [details]
Pull request 231

Gabor: I'd like to have your feedback on this beforce asking for a review.
If it can help, we can take some time to discuss about that on IRC or in a 1:1.
Just let me know.

From a security or sandbox perspective, here is interesting parts:
- https://github.com/mozilla/addon-sdk/pull/231/files#L2R222
Instead of a regular jetpack/common-js module that runs with system principal,
I create manually a Sandbox with same principal that the web page.
(I do not reuse Sandbox of the content script itself in order to avoid mixing globals, like Object, Array, ... Some js framwork modify these classes and make my proxy code fails)

- https://github.com/mozilla/addon-sdk/pull/231/files#L2L220
I do use wantXrays=true now for these two sandboxes!

- https://github.com/mozilla/addon-sdk/pull/231/files#L2R317
I end up facing same issues that in bug 636145, where `apply` method doesn't exists. We would need to move all content script global definitions to the content proxy sandbox instead of the worker module sandbox (that is a priviledge one). But I thought that it will be better to do so in a seperate bug/patch.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 555206 [details]
Pull request 231

I got a positive feedback from Gabor on irc, 
so I'm asking now for a review.
Irakli: We can take some time while we are both in office do a peer review if it can help ...

Comment 4

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

Comment on attachment 555206 [details]
Pull request 231

I think there are few things that need to be solved before landing, details are in pull request.

Comment 5

[Alexandre Poirot [:ochameau]]

irakli: I've addressed your comments on a new commit, may you give it another round?

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 7

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

Comment on attachment 555206 [details]
Pull request 231

I made another review round, but I don't know if approach is ok or not (exposing UNWRAPPED_ACCESS_KEY to the content scripts, using strings as a value for it). I'd be more comfortable if Myk could provide feedback on such approach before accepting / rejecting this change.

Comment 8

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

Comment on attachment 555206 [details]
Pull request 231

I have some concerns, which are noted in the pull request, and tests also failed with the changes merged into trunk.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 555206 [details]
Pull request 231

I pushed another set of commit with related inline comment.
Can one of you do another round?

Comment 10

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

Comment on attachment 555206 [details]
Pull request 231

Reviewed!

The new use of keys is fine.  I still want a better way to test content proxies that doesn't require exposing a normally-private key to content scripts, since that imperfectly simulates a production environment; but the current way is good enough for now.

I only identified one serious issue: the way test-content-proxy shares a single document with multiple test functions makes it prone to bugs that are hard to identify and fix, so it should create a document for each test function.

Otherwise I just have a bunch of code documentation requests and some nits.

So this is close! :-)

Note: I did both a review of the strategy that Irakli asked for and also a code review, so this doesn't need further code review from Irakli.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 555206 [details]
Pull request 231

I've pushed a new commit and added comments on pull request.
Ready for another round!
Thanks again for all language fixes.

Comment 12

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

Comment on attachment 555206 [details]
Pull request 231

Looks good!  A few more nits noted in the pull request, and there's a merge conflict to resolve.

Comment 13

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/b9f73b04d856121e18de1674bf785fc197da7681