949488 - postMessage's targetOrigin argument should accept /

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anne (:annevk)]

> postMessage(1, "/")

should not throw an exception.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  This was a somewhat-recent change to the spec to mean "same origin as caller" or something, I think.

Andrea, want to take this?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: postMessage.patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8346741 [details] [diff] [review]
postMessage.patch

>+    providedOrigin = win->GetDocumentURI();

Shouldn't this come from the principal?  This seems like it would fail for an about:blank document, say.

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8346741 [details] [diff] [review]
> postMessage.patch
> 
> >+    providedOrigin = win->GetDocumentURI();
> 
> Shouldn't this come from the principal?  This seems like it would fail for
> an about:blank document, say.

Yep, I was not sure if include the principal->GetURI(). I do that in the next patch.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: postMessage.patch

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8346773 [details] [diff] [review]
postMessage.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +7838,5 @@
>    // Convert the provided origin string into a URI for comparison purposes.
> +  nsCOMPtr<nsIURI> providedOrigin;
> +
> +  if (aTargetOrigin.EqualsASCII("/")) {
> +    nsIGlobalObject* sgo = BrokenGetEntryGlobal();

this should be |go|, because it's not an nsIScriptGlobalObject.

@@ +7839,5 @@
> +  nsCOMPtr<nsIURI> providedOrigin;
> +
> +  if (aTargetOrigin.EqualsASCII("/")) {
> +    nsIGlobalObject* sgo = BrokenGetEntryGlobal();
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(sgo);

But actually, I think we should just collapse these two lines.

@@ +7843,5 @@
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(sgo);
> +    nsRefPtr<nsGlobalWindow> window = static_cast<nsGlobalWindow*>(win.get());
> +    if (!window) {
> +      return;
> +    }

This will break if the entry point is not a window, which can happen (if not currently, then eventually).

I think we should add an inline non-virtual method on nsIGlobalObject:

nsIPrincipal* PrincipalOrNull() {
  JSObject *global = GetGlobalJSObject();
  if (NS_WARN_IF(!global))
    return nullptr;
  return nsContentUtils::GetObjectPrincipal(global);
}

Then, we can just skip straight to the principal part here.

@@ +7852,5 @@
> +    }
> +
> +    if (NS_FAILED(principal->GetURI(getter_AddRefs(providedOrigin)))) {
> +      return;
> +    }

We should be using NS_WARN_IF macros here.

@@ +7854,5 @@
> +    if (NS_FAILED(principal->GetURI(getter_AddRefs(providedOrigin)))) {
> +      return;
> +    }
> +
> +    if (!providedOrigin) {

Why the null-check here?

@@ +7855,5 @@
> +      return;
> +    }
> +
> +    if (!providedOrigin) {
> +      providedOrigin = window->GetDocumentURI();

Seems like we should be pulling the URI off the principal. But then what the heck do we do in the system principal case?

I think we probably need to change things around so we pass a principal, rather than a URI, to PostMessageEvent. bz, do you agree?

::: dom/base/test/test_postMessage_solidus.html
@@ +14,5 @@
> +  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=949488">Mozilla Bug 949488</a>
> +  <div id="content"></div>
> +  <script type="application/javascript">
> +
> +  function basicPostMessage() {

maybe call this 'selfMessage'?

@@ +36,5 @@
> +
> +    function iframeLoaded() {
> +      addEventListener('message', receiveMessage, false);
> +      function receiveMessage(evt) {
> +        is(evt.data, 42, "Message received");

maybe use a different magic value for each of the tests, to avoid confusion?

@@ +37,5 @@
> +    function iframeLoaded() {
> +      addEventListener('message', receiveMessage, false);
> +      function receiveMessage(evt) {
> +        is(evt.data, 42, "Message received");
> +        removeEventListener('message', receiveMessage, false);

In modern Firefox, the final bool param is optional, I think.

@@ +62,5 @@
> +        runTest();
> +      }
> +
> +      ifr.contentWindow.postMessage(41, '/');
> +      ifr.contentWindow.postMessage(42, '*');

Maybe SimpleTest.executeSoon the second one so that the first one has more time to trigger the handler and cause a failure, in case there are any funny ordering issues going on?

Comment 7

[Bobby Holley (:bholley)]

NeedInfo bz about making PostMessageEvent take a principal rather than a URI.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Passing a principal to PostMessageEvent make sense to me, yes.

Comment 9

[Andrea Marchesini [:baku]]

> > +
> > +    if (!providedOrigin) {
> 
> Why the null-check here?

The URI from the Principal can be null. If that happens, this code tries to retrieve the URI from the window->document as PostMessageEvent does.

> I think we probably need to change things around so we pass a principal,
> rather than a URI, to PostMessageEvent. bz, do you agree?

If we do that, what about the case where targetOrigin is a URL? Can I convert that in a principal?
I'm talking about:  else if (!aTargetOrigin.EqualsASCII("*")) { ...

Comment 10

[Anne (:annevk)]

http://w3c-test.org/web-platform-tests/master/webmessaging/without-ports/004.html is the W3C test.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini (:baku) from comment #9)
> > Why the null-check here?
>  
> The URI from the Principal can be null. If that happens, this code tries to
> retrieve the URI from the window->document as PostMessageEvent does.

Oh yeah. Duh. I didn't notice that we had already tried to get providedOrigin above. 


> > I think we probably need to change things around so we pass a principal,
> > rather than a URI, to PostMessageEvent. bz, do you agree?
> 
> If we do that, what about the case where targetOrigin is a URL? Can I
> convert that in a principal?
> I'm talking about:  else if (!aTargetOrigin.EqualsASCII("*")) { ...

You can grab the SSM and call GetSimpleCodebasePrincipal, passing in an nsIURI.

This also begs the question of what we should do in the case where the caller is in a webapp or mozBrowser and passes 'http://www.example.com'. Do we want it to only match if the target is also in a webapp or mozBrowser? Or should we ignore the app and mozBrowser attributes completely? We should probably ask sicking or someone.

If we want to ignore them, we should probably provide a helper on nsIPrincipal like so:

nsIPrincipal* asSimplePrincipal();

Which just generates and returns a principal whose codebase matches |this|, but with no mozbrowser/app status. For nsExpandedPrincipal, nsNullPrincipal, and nsSystemPrincipal, this can just be an identity operation.

Comment 12

[Bobby Holley (:bholley)]

I guess the current setup effectively ignores those attributes. So it's worth double-checking with sicking that we want to keep doing that, but that's likely the way forward here.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: postMessage.patch

Still checking if it's green on try.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8347307 [details] [diff] [review]
postMessage.patch

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

r- for the app/mozBrowser thing. Everything else looks fine.

::: dom/base/nsGlobalWindow.cpp
@@ +7682,4 @@
>      // Get the target's origin either from its principal or, in the case the
>      // principal doesn't carry a URI (e.g. the system principal), the target's
>      // document.
>      nsIPrincipal* targetPrin = targetWindow->GetPrincipal();

So, this is going to cause us to fall victim to the app issues I mentioned in comment 11. That'll need to be fixed before this patch is correct.

@@ +7683,5 @@
>      // principal doesn't carry a URI (e.g. the system principal), the target's
>      // document.
>      nsIPrincipal* targetPrin = targetWindow->GetPrincipal();
>      if (!targetPrin)
>        return NS_OK;

Let's make this an NS_WARN_IF while we're here.

@@ +7831,5 @@
> +  if (aTargetOrigin.EqualsASCII("/")) {
> +    providedPrincipal = BrokenGetEntryGlobal()->PrincipalOrNull();
> +    if (!providedPrincipal) {
> +      return;
> +    }

This should be:

if (NS_WARN_IF(!providedPrincipal))
  return;

(Note the lack of braces, which is a stylistic exception bsmedberg made for the NS_WARN_IF pattern).

@@ +7850,5 @@
> +
> +    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +    if (NS_WARN_IF(NS_FAILED(ssm->GetSimpleCodebasePrincipal(
> +                                         originURI,
> +                                         getter_AddRefs(providedPrincipal))))) {

This indentation is kind of weird, though it's a bit of a tricky situation. Is this following a convention I can read about somewhere?

::: dom/base/nsIGlobalObject.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsIGlobalObject.h"

Need to #include "nsContentUtils.h" here.

@@ +11,5 @@
> +{
> +  JSObject *global = GetGlobalJSObject();
> +  if (NS_WARN_IF(!global)) {
> +    return nullptr;
> +  }

No braces here.

::: dom/base/nsIGlobalObject.h
@@ +21,5 @@
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_IGLOBALOBJECT_IID)
>  
>    virtual JSObject* GetGlobalJSObject() = 0;
> +
> +  nsIPrincipal* PrincipalOrNull();

Maybe add a comment that this is explicitly non-virtual?

Comment 15

[Andrea Marchesini [:baku]]

Jonas, can you tell us something about comment 11 ?

Comment 16

[Andrea Marchesini [:baku]]

> > +    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> > +    if (NS_WARN_IF(NS_FAILED(ssm->GetSimpleCodebasePrincipal(
> > +                                         originURI,
> > +                                         getter_AddRefs(providedPrincipal))))) {
> 
> This indentation is kind of weird, though it's a bit of a tricky situation.
> Is this following a convention I can read about somewhere?

This is how code is written in workers and in IDB. Does it count? :)

Comment 17

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini (:baku) from comment #16)
> > > +    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> > > +    if (NS_WARN_IF(NS_FAILED(ssm->GetSimpleCodebasePrincipal(
> > > +                                         originURI,
> > > +                                         getter_AddRefs(providedPrincipal))))) {
> > 
> > This indentation is kind of weird, though it's a bit of a tricky situation.
> > Is this following a convention I can read about somewhere?
> 
> This is how code is written in workers and in IDB. Does it count? :)

Ok. But what is the rule for the number of spaces? 12 spaces from the nearest ( on the preceding line?

I'm really ok with whatever, as long as it's consistent and we can apply it throughout Gecko. If this is something we want to do, we should get it added to the style guide IMO.

Comment 18

[Andrea Marchesini [:baku]]

(In reply to Bobby Holley (:bholley) from comment #17)
> (In reply to Andrea Marchesini (:baku) from comment #16)
> > > > +    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> > > > +    if (NS_WARN_IF(NS_FAILED(ssm->GetSimpleCodebasePrincipal(
> > > > +                                         originURI,
> > > > +                                         getter_AddRefs(providedPrincipal))))) {
> > > 
> > > This indentation is kind of weird, though it's a bit of a tricky situation.
> > > Is this following a convention I can read about somewhere?
> > 
> > This is how code is written in workers and in IDB. Does it count? :)
> 
> Ok. But what is the rule for the number of spaces? 12 spaces from the
> nearest ( on the preceding line?

No, the idea is:
1 if they can, all the params on the same line. max 80 chars.
2 otherwise multiple lines, aligned with the beginning of the params list
3 if 1 param alone goes over 80chars, it must be align in order to stay in the 80chars and the rest of the params must be align with it.

This is another example of this approach:

https://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1647

Comment 19

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini (:baku) from comment #18)
> No, the idea is:
> 1 if they can, all the params on the same line. max 80 chars.
> 2 otherwise multiple lines, aligned with the beginning of the params list
> 3 if 1 param alone goes over 80chars, it must be align in order to stay in
> the 80chars and the rest of the params must be align with it.
> 
> This is another example of this approach:
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1647

That seems reasonable. Would you mind sending mail to dev-platform proposing that this be added to the style guide?

Comment 20

[Andrea Marchesini [:baku]]

Jonas, any news about comment 11 ?

Comment 21

[Andrea Marchesini [:baku]]

Attachment: postMessage.patch

This other approach implements EqualIgnoringDomainAndAppId.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8355552 [details] [diff] [review]
postMessage.patch

baku and I talked about this on IRC. We decided that the best approach is to inherit the app id and mozbrowserness of the caller.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: postMessage.patch

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8355607 [details] [diff] [review]
postMessage.patch

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

r=bholley with comments addressed.

::: dom/base/nsGlobalWindow.cpp
@@ +7863,5 @@
> +    }
> +
> +    nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager();
> +    nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal();
> +    if (!ssm || !principal) {

NS_WARN_IF, but actually I think you can MOZ_ASSERT both of these.

@@ +7870,5 @@
> +
> +    uint32_t appId;
> +    bool isInBrowser;
> +    if (NS_FAILED(principal->GetAppId(&appId)) ||
> +        NS_FAILED(principal->GetIsInBrowserElement(&isInBrowser))) {

NS_WARN_IF. And maybe these should be separated?

@@ +7874,5 @@
> +        NS_FAILED(principal->GetIsInBrowserElement(&isInBrowser))) {
> +      return;
> +    }
> +
> +    rv = ssm->GetAppCodebasePrincipal(originURI, appId, isInBrowser,

Add a comment explaining that we're inheriting the app/browser attributes from the caller?

@@ +7881,2 @@
>        return;
>      }

As noted in comment 14, we should not use braces for NS_WARN_IF checks.

::: dom/base/nsIGlobalObject.h
@@ +21,5 @@
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_IGLOBALOBJECT_IID)
>  
>    virtual JSObject* GetGlobalJSObject() = 0;
> +
> +  // This method is not meant to be override.

s/override/overridden/.

Comment 25

[Andrea Marchesini [:baku]]

Actually it turned out that mochitest dom/tests/mochitest/localstorage/test_clear_browser_data.html uses postMessage() to communicate with a child app. I can easily change this test in order to use '*' instead the origin URL, but maybe it's wrong.

What about if I implement what we discuss on IRC: asSimplePrincipal()? Or a better name method that returns a principal without any attribute?

Comment 26

[Bobby Holley (:bholley)]

I just talked with fabrice, and he thinks we should just inherit and fix the test to pass '*'.

Comment 27

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e451b39305f6

Ok. test changed to '*'

Comment 28

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

Backed out for being the apparent cause of https://tbpl.mozilla.org/php/getParsedLog.php?id=32608429&tree=Mozilla-Inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/b359c08f9a9f

(Or at least, http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/storageevent/test_storageSessionStorageEventCheckNoPropagation.html?force=1#6 runs http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/storageevent/interOriginTest2.js#31 which calls postMessage(). )

Comment 29

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=66d596edace6

https://hg.mozilla.org/integration/mozilla-inbound/rev/de68c6ddc8fd

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/de68c6ddc8fd