1139849 - postMessage to incorrect target domain should print a console security error

Status: RESOLVED FIXED

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

Description

[Shane Tomlinson [:stomlinson]]

Attachment: Message in Chrome about the postMessage not being sent.

It would be useful to let a developer know when invoking postMessage with a target that does not match the target window. In this scenario, the postMessage will not occur, but the developer/user will be left in the dark. 

Chrome notifies users via their developer console. It would be nice if we did as well, possibly under the Security section of the console.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I believe this has to be handled in the DOM code. If an error message is generated there, the web console will happily display it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah.  For whoever wants to work on this, the idea is that in nsGlobalWindow.cpp in PostMessageEvent::Run, where we do this:

8160     if (!targetPrin->Equals(mProvidedPrincipal)) {
8161       return NS_OK;
8162     }

we should log a console error using nsContentUtils::ReportToConsole.  Presumably one associated with the window id of the incumbent global at the callsite to postMessage (so we'll need to cart along the nsIDocument involved in the PostMessageEvent, and null it out after this point in Run ideally).

I'm happy to mentor anyone who wants to work on this one.

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

+1, would love to see this. cc'ing Stephen as we've been discussing security-related messages in the console.

Comment 4

[Vaibhav Bhosale]

Hey,
I would like to work on this bug.

Comment 5

[Josh Matthews [:jdm]]

Do you have any questions after look through the references in comment 2?

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Thanks for the cc Jeff. Looks like Vaibhav has this with bz as mentor, right?

cc'ing Christoph to keep it on his radar.

Comment 7

[Donato Sciarra]

Hi! I would like to work on this bug. Some naive questions following comment 2:
 - How do I get the nsIDocument involved in the PostMessageEvent?
 - What is meant by "incumbent global"?

Thanks!

Comment 8

[Boris Zbarsky [:bzbarsky]]

> - How do I get the nsIDocument involved in the PostMessageEvent?

You need to pass it in from the code that creates the event.

> - What is meant by "incumbent global"?

The global of the code that made the postMessage call.  See https://html.spec.whatwg.org/multipage/webappapis.html#incumbent-settings-object for the spec bits, but they're not that informative.

For your purposes, if you look at nsGlobalWindow::PostMessageMoz, it has a callerInnerWindow (which may be null) which it gets the callerPrin from.  What you want to do is to get an nsIDocument (via ->GetDoc()) from it as well and pass that to the event.  If callerInnerWin is null, you won't be able to get a document, of course, but that's OK; ReportToConsole can handle a null document.

Does that help?

Comment 9

[Donato Sciarra]

Attachment: bug1139849.patch

not a final patch. I just would like to know, if I am on the right path...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8582694 [details] [diff] [review]
bug1139849.patch

That's the right idea, but it needs to handle callerInnerWin being null, you want nsCOMPtr, not nsRefPtr, and you want to null out mDocument once you're done with it, to avoid the event entraining it.

Comment 11

[Donato Sciarra]

Attachment: bug1139849.patch

There is something not working as expected. I get "undefined" if a run this in the webconsole: 
window.postMessage("BlaBla","http://example.org");
Do I receive the error message only if I have built firefox with the en-US locale?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Do I receive the error message only if I have built firefox with the en-US locale?

It's possible; I'm not sure localization falls back to the en-US string if it can't find the one in the current langpack.... :(

You should be able to run against a clean profile with no langpacks installed to check, I'd think.

One other note: the GetURI() of a principal might be null, so the code needs to handle that.  And we still want to null out mDocumence once we're done.

Comment 13

[Joshpal Sahota]

Hi, my name is Josh. I am studying the Open-Source Development module at Coventry University and would like to work on this bug as my "First Bug".

Would that be okay?

Regards

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yes, absolutely.  Please feel free to mail me directly if you run into any problems!  You probably want to start by picking up the patch that's already in this bug.

Comment 15

[James Burke [:jrburke]]

Attachment: MozReview Request: Bug 1139849 - postMessage to incorrect target domain should print a console security error

Review commit: https://reviewboard.mozilla.org/r/32731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32731/

Comment 16

[Boris Zbarsky [:bzbarsky]]

James, did you mean to request review from someone?

Comment 17

[James Burke [:jrburke]]

Attachment: Snapshot of MozReview change in action

I apologize for not asking to work on this bug first. I am new to the Gecko side of things and reviewboard, and was trying a bug out, including pushing to reviewboard, as I saw it in an info meeting about contributing. I was so focused on confirming the right steps for the reviewboard part that it seems to have attached to this bug.

I am happy to obsolete this change though if someone else was working on it, or if I messed up with reviewboard. I will set the r? flag though shortly, forgot to set that up correctly before a push.

This attachment shows how the error appears in my local build, and it matches the word style and placement of the different origins with the Chrome error. This will hopefully make it easier for people searching on the internet for the error message to find better, consolidated results.

Some other notes on the changeset:

* builds on the previous patch bug.
* Moved the code to PostMessageEvent.h/cpp, since the code seemed to have moved since last patch.
* Used nsIScriptError::errorFlag instead of warningFlag, since that also matches better with the Chrome behavior.
* Used origin instead of URI in the message, as that matches to what is being tested, matches the Chrome result too.
* Sets mSourceDocument = nullptr after the principal check is done.
* I tested locally by setting targetOrigin = nullptr to see if it had problems, but it seemed to still work, just printed '' in the error message.

Comment 18

[James Burke [:jrburke]]

Comment on attachment 8712977 [details]
MozReview Request: Bug 1139849 - postMessage to incorrect target domain should print a console security error

Setting review flag to :bz as he is a mentor on this bug, but happy to retarget to lessen review load.

Comment 19

[Boris Zbarsky [:bzbarsky]]

James, no one is actively working on this right now that I know of.  So no worries on that score.  ;)

Thank you for working on it!  I'll do the review tomorrow morning (US Eastern time).

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8712977 [details]
MozReview Request: Bug 1139849 - postMessage to incorrect target domain should print a console security error

https://reviewboard.mozilla.org/r/32731/#review29609

Sorry for the lag; some urgent things came up this morning.  :(

::: dom/base/PostMessageEvent.cpp:98
(Diff revision 1)
> +      nsresult rv = targetPrin->GetOrigin(targetOrigin);

Instead of using nsIPrincipal::GetOrigin, which is an internal thing that does quite interesting things in some situations that would be confusing in an error message targeting web developers, I think this code should use nsContentUtils::GetUTFOrigin, which gets what the HTML spec defines as the "origin".

So like this:

  nsAutoString providedOrigin, targetOrigin;
  nsresult rv = nsContentUtils::GetUTFOrigin(targetPrin, targetOrigin);
  
And yes, I think you should call the thing you get from mProvidedPrincipal "providedOrigin".

Then you won't need to do extra UTF8-to-UTF16 conversions either.

::: dom/base/nsGlobalWindow.cpp:7972
(Diff revision 1)
> +                         !callerInnerWin

Probably better to reverse the sense of this check.  Up above it's the way it is because of the IsCallerChrome() bit in the boolean or, but here we've just got a simple condition, and starting it with ! is a bit odd.

r=me with those two nits fixed, and thank you for picking this up!

I'm not sure what the current status of mozreview is in terms of being able to push to it.  Once you have the updated patch, if you can't push it to mozreview please just attach it to the bug directly and I'll get it landed for you.

Comment 21

[James Burke [:jrburke]]

Attachment: postMessage to incorrect target domain should print a console security error

Comment 22

[James Burke [:jrburke]]

Comment on attachment 8713483 [details] [diff] [review]
postMessage to incorrect target domain should print a console security error

I had some issue with MozReview/my local clone so ended up with this patch.

It includes the two changes around nsContentUtils::GetUTFOrigin and reversing the callerInnerWin? tertiary logic to place the positive case first.

Built and tested locally, the message appears the same as in the previously uploaded screenshot.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8713483 [details] [diff] [review]
postMessage to incorrect target domain should print a console security error

Actually, I just realized one more issue: If the principal check _fails_ we never null out mSourceDocument.

What we should probably do is put it in a stack variable at the top of Run(), null it out, and use the stack variable for the ReportToConsole call...

Comment 24

[James Burke [:jrburke]]

Ah right, I can do that, can post a fresh patch tomorrow.

Comment 25

[James Burke [:jrburke]]

Attachment: postMessage to incorrect target domain should print a console security error

Comment 26

[James Burke [:jrburke]]

Comment on attachment 8713803 [details] [diff] [review]
postMessage to incorrect target domain should print a console security error

Updated patch, the diff from the previous patch is just the stack variable for the source document. Built and run locally, still works the same:

diff --git a/dom/base/PostMessageEvent.cpp b/dom/base/PostMessageEvent.cpp
--- a/dom/base/PostMessageEvent.cpp
+++ b/dom/base/PostMessageEvent.cpp
@@ -54,16 +54,22 @@ PostMessageEvent::Run()
              "should have been passed an outer window!");
   MOZ_ASSERT(!mSource || mSource->IsOuterWindow(),
              "should have been passed an outer window!");
 
   AutoJSAPI jsapi;
   jsapi.Init();
   JSContext* cx = jsapi.cx();
 
+  // The document is just used for the principal mismatch error message below.
+  // Use a stack variable so mSourceDocument is not held onto after this method
+  // finishes, regardless of the method outcome.
+  nsIDocument* sourceDocument = mSourceDocument;
+  mSourceDocument = nullptr;
+
   // If we bailed before this point we're going to leak mMessage, but
   // that's probably better than crashing.
 
   RefPtr<nsGlobalWindow> targetWindow;
   if (mTargetWindow->IsClosedOrClosing() ||
       !(targetWindow = mTargetWindow->GetCurrentInnerWindowInternal()) ||
       targetWindow->IsClosedOrClosing())
     return NS_OK;
@@ -98,29 +104,25 @@ PostMessageEvent::Run()
       nsresult rv = nsContentUtils::GetUTFOrigin(targetPrin, targetOrigin);
       NS_ENSURE_SUCCESS(rv, rv);
       rv = nsContentUtils::GetUTFOrigin(mProvidedPrincipal, providedOrigin);
       NS_ENSURE_SUCCESS(rv, rv);
 
       const char16_t* params[] = { providedOrigin.get(), targetOrigin.get() };
 
       nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
-        NS_LITERAL_CSTRING("DOM Window"), mSourceDocument,
+        NS_LITERAL_CSTRING("DOM Window"), sourceDocument,
         nsContentUtils::eDOM_PROPERTIES,
         "TargetPrincipalDoesNotMatch",
         params, ArrayLength(params));
 
       return NS_OK;
     }
   }
 
-  // The document is just used for the principal mismatch check above, now it is
-  // no longer needed.
-  mSourceDocument = nullptr;
-
   ErrorResult rv;
   JS::Rooted<JS::Value> messageData(cx);
   nsCOMPtr<nsPIDOMWindow> window = targetWindow.get();
 
   Read(window, cx, &messageData, rv);
   if (NS_WARN_IF(rv.Failed())) {
     return rv.StealNSResult();
   }

Comment 27

[Boris Zbarsky [:bzbarsky]]

>+  nsIDocument* sourceDocument = mSourceDocument;

That needs to be an nsCOMPtr<nsIDocument> so the object is kept alive.  So something like this:

  nsCOMPtr<nsIDocument> sourceDocument;
  sourceDocument.swap(mSourceDocument);

Comment 28

[Boris Zbarsky [:bzbarsky]]

The tree was open for once, so I just made that change and pushed this.  Thank you again for the patch!

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13d8daa68e24

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/13d8daa68e24