666547 - No way to interact between content scripts and content js

Status: RESOLVED FIXED

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

Description

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

For some time we had a way to interact between content stript js and js inside a content page itself. Pipe was limited to a string messages passing:

window.postMessage(string, '*')

It looks like recent changes to the worker code broke this.

1. window === self now so window.postMessage is not what one would expect :(
2. document.defaultView.postMessage does not works either, it does not throws exceptions neither does anything useful.
3. unsafeWindow.postMessage has a same behavior as 2.

The worst thing is that page script still can call the content script, but not the other way round, while from the security perspective other way round would've be less harmful.

This is bug is specially annoying when building addons that server local content from data folder with big third party code base that is hard to port over to content scripts.

Comment 1

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/193

Pointer to Github pull-request

Comment 2

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

Comment on attachment 541348 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/193

Creating a pull request with a test case for this bug.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to comment #0)
> The worst thing is that page script still can call the content script, but
> not the other way round, while from the security perspective other way round
> would've be less harmful.
Both way should be considered safe as it is only Event with string data.
But regardless of security threat, this is definitively a painfull bug!

It appears to be a platform one. Here is a sample snippet that can be paste in your JS console:
var w = top.opener.gBrowser.contentDocument.defaultView.wrappedJSObject; 
var s=Components.utils.Sandbox(w); s.w=w; Components.utils.evalInSandbox("w.postMessage('ok','*');", s)

And the matching document to load in a tab before executing it:
data:text/html,<script>window.addEventListener('message', function () {alert('ok');}, false); </script>

Whereas the following script is working:
var w = top.opener.gBrowser.contentDocument.defaultView.wrappedJSObject;
w.postMessage('ok', '*');

I'm going to open a related platform bug and search for a work around.
Finally I'm taking assignment of this bug as I'm familiar with platform issues ;)

Comment 4

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

Hmm, that's strange few of my addons that I've build back in time still work as they were created with older sdk. Same addon's have this issues with the current SDK so I'm puzzled why

Comment 5

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

I realized as soon as clicked the post button :)

Old sdk used to create sandboxes with system principles that probably does not has this issue.

For IE there was a nasty workaround using iframe and location hash changes, I really hope I won't have to do the same here. At the moment I'm bit less ugly  workaround that sets value into a hidden text field :) I think it maybe an influence after seeing phonegap talk :D

Comment 6

[Alexandre Poirot [:ochameau]]

That's almost it, it is not the principal but the global object not being a window :o
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6069
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#5853
(I still need to verify it though gdb)

Comment 7

[Jonathan Mayer]

Inter-frame communication from a content script (e.g. window.parent.postMessage) also appears to have broken between 1.0b5 and 1.0. Seems likely to have the same cause, but I'm not familiar enough with the Jetpack codebase to track it down.

Comment 8

[Ben Adida [:benadida]]

we've noticed this on the Open Web Apps work, too, and we agree that this is probably a platform issue related to resource URIs postMessage'ing into content URIs. Who's the right person to talk to?

Comment 9

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

So there are actually few options to go with until this is fixed:

1. dispatching custom MessageEvents on window:
https://github.com/Gozala/sky-edit/commit/e3cc9d2992587f05463bab06d293adbf50600385

I believe that custom `message` events may be created and dispatched manually so that it listeners of `message` event will be called.

2. exposing `self.port` to content by setting `unsafeWindow.port = self.port`:
https://github.com/Gozala/sky-edit/commit/c7bf54dd8a1e86c749ffaa7e539bf4ab3434d85d

Comment 10

[Alexandre Poirot [:ochameau]]

Jeff: As you worked on initial patch that provides postMessage, you may be the best one to tell us if we can change its security checks?
nsGlobalWindow::PostMessageMoz()
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6095
  Use
nsGlobalWindow::CallerInnerWindow()
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#5863
This code makes the assumtion that global object is a nsGlobalWindow/nsPIDOMWindow. But in Addon SDK case (and other cases, like js modules, xpcoms or other codes using sandboxes), this assumtion is false.
And we end up being unable to use postMessage. (even from priviledge code)

See comment 3, for some code example that fails/work.

Do you think we can change this? Would this still be compatible with HTML spec?

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Fix this in addon sdk, by using eval.

Here is a way to gain the right conditions to make platform code accept our postMessage call.
By using eval, we have the right global object. This is pretty hacky, but this is not the first time we add something similar to fix xraywrapper bugs/limitations.

I tried to implement an alternative by dispatching MessageEvent manually, but we would end up rewriting most of C++ code that do all these security checks. I think Andreas would be happy if we implement this, but this is a big project.

Comment 12

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

Comment on attachment 543138 [details] [diff] [review]
Fix this in addon sdk, by using eval.

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

I don't like solution, but I don't have a better one so I guess it's ok to go with it as long as we have some bug or feature request to support this scenario and comment linking to it. Please make sure to remove try / catch and reuse postMessage function so that window.postMessage === window.postMessage. Other comment on the overrides is nice to have (since get method is full of such hacks already), so I'll let you decide on that one.

::: packages/api-utils/lib/content/content-proxy.js
@@ +416,5 @@
> +        try {
> +          // Ensure that we are on a window object
> +          obj.QueryInterface(Ci.nsIDOMWindow);
> +          // Create a wrapper that is going to call `postMessage` through `eval`
> +          let f = function postMessage(message, targetOrigin) {

I don't think it's good idea to create this function each time property is accessed. window.postMessage !== window.postMessage is bad. Also amount of condition in get grows and makes code unmaintainable. What about an object containing properties with such overrides ?

@@ +425,5 @@
> +            return this.wrappedJSObject.eval(jscode);
> +          };
> +          return getProxyForFunction(f, NativeFunctionWrapper(f));
> +        }
> +        catch(e) {}

I don't think it's vice to swallow error here as it will only make tracking down bug if there is one harder.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 216

I submitted a new pull request as these changes are bigger.
So I added two "fixes" holders:
 - one for methods that need to be cached
 - one for missing attributes

Comment 14

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

Comment on attachment 547105 [details]
Pull request 216

There were some nits, so it's up to you to decide if you want to fix them. In any case there is no need for another cycle!

Thanks!

Comment 15

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

Alex do you want to include my pull request with test case ??

Comment 16

[Alexandre Poirot [:ochameau]]

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

Comment 17

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

Attachment: pull request 193: adds test

Alex: do you want to take this additional test that Irakli wrote?

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 571492 [details] [diff] [review]
pull request 193: adds test

I just sent a comment to the pull request.
This unit test should already be covered by an existing one,
and it looks quite complex for a test.