Closed Bug 949488 Opened 11 years ago Closed 11 years ago

postMessage's targetOrigin argument should accept /

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: annevk, Assigned: baku)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

> postMessage(1, "/")

should not throw an exception.
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?
Assignee: nobody → amarchesini
Attached patch postMessage.patch (obsolete) — Splinter Review
Attachment #8346741 - Flags: review?(bobbyholley+bmo)
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.
(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.
Attached patch postMessage.patch (obsolete) — Splinter Review
Attachment #8346741 - Attachment is obsolete: true
Attachment #8346741 - Flags: review?(bobbyholley+bmo)
Attachment #8346773 - Flags: review?(bobbyholley+bmo)
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?
Attachment #8346773 - Flags: review?(bobbyholley+bmo) → review-
NeedInfo bz about making PostMessageEvent take a principal rather than a URI.
Flags: needinfo?(bzbarsky)
Passing a principal to PostMessageEvent make sense to me, yes.
Flags: needinfo?(bzbarsky)
> > +
> > +    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("*")) { ...
(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.
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.
Attached patch postMessage.patch (obsolete) — Splinter Review
Still checking if it's green on try.
Attachment #8346773 - Attachment is obsolete: true
Attachment #8347307 - Flags: review?(bobbyholley+bmo)
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?
Attachment #8347307 - Flags: review?(bobbyholley+bmo) → review-
Jonas, can you tell us something about comment 11 ?
Flags: needinfo?(jonas)
> > +    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? :)
(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.
(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
(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?
Jonas, any news about comment 11 ?
Keywords: dev-doc-needed
Attached patch postMessage.patch (obsolete) — Splinter Review
This other approach implements EqualIgnoringDomainAndAppId.
Attachment #8347307 - Attachment is obsolete: true
Attachment #8355552 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8355552 - Flags: review?(bobbyholley+bmo)
Attachment #8355552 - Attachment is obsolete: true
Attachment #8355607 - Flags: review?(bobbyholley+bmo)
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/.
Attachment #8355607 - Flags: review?(bobbyholley+bmo) → review+
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?
Flags: needinfo?(bobbyholley+bmo)
I just talked with fabrice, and he thinks we should just inherit and fix the test to pass '*'.
Flags: needinfo?(bobbyholley+bmo)
https://hg.mozilla.org/mozilla-central/rev/de68c6ddc8fd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Flags: needinfo?(jonas)
Target Milestone: mozilla29 → ---
Target Milestone: --- → mozilla29
Flags: in-testsuite+
Depends on: 1022229
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: