Fix extension bindings injection for iframes

chrome/browser/extensions/extension_bindings_apitest.cc

Index: chrome/browser/extensions/extension_bindings_apitest.cc
diff --git a/chrome/browser/extensions/extension_bindings_apitest.cc b/chrome/browser/extensions/extension_bindings_apitest.cc
index 387576f78c9594f291a0b3d98392fb7d0088d19d..eb78d8fdaab0c483a139f1542b859b4e73d60f07 100644
--- a/chrome/browser/extensions/extension_bindings_apitest.cc
+++ b/chrome/browser/extensions/extension_bindings_apitest.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/browser_test_utils.h"
@@ -208,5 +209,63 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest,
   EXPECT_EQ("success", result);
 }
 
+// This tests that web pages with iframes or child windows pointing at
+// chrome-extenison:// urls, both web_accessible and non-existing pages, don't

[Devlin, 2016/07/21 00:44:54]

nit: s/non-existing/nonexistent

[asargent_no_longer_on_chrome, 2016/07/21 18:12:22]

Done.

+// get improper extensions bindings injected while they briefly still point at
+// about:blank and are still scriptable by their parent.
+//
+// The general idea is to load up 2 extensions, one which listens for external
+// messages ("receiver") and one which we'll try first faking messages from in
+// the web page's iframe, as well as actually send a message from later
+// ("sender").
+IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, FramesBeforeNavigation) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(

[Devlin, 2016/07/21 00:44:54]

Should this be in SetUpCommandLine()?

[asargent_no_longer_on_chrome, 2016/07/21 18:12:22]

It works fine being here, I think because we don't need the value set until the
renderer for the "frames_before_navigate.html" page is created when we open the
page below in the test. Also, since we don't need it for the other
ExtensionBindingsApiTest tests, if I did put it in a SetUpCommandLine, I'd need
to make a new subclass of ExtensionBindingsApiTest.

[Devlin, 2016/07/21 22:20:34]

I have two concerns:
- If we ever change chrome to cache this command line flag, rather than checking
it as we launch a renderer, this will stop working.
- If the browser test runs in a different process than chrome, this won't work
(I think we do/did/thought about doing this at point?  I can't recall.)

Since we have the canonical way of modifying the command line through
SetUpCommandLine, I'd be more comfortable with the extra 5 lines to subclass
ExtensionBindingsApiTest.  But I don't feel super strongly, so I'll leave it up
to you.

+      switches::kDisablePopupBlocking);
+
+  // Load the sender and receiver extensions, and make sure they are ready.
+  ExtensionTestMessageListener sender_ready("sender_ready", true);
+  const Extension* sender = LoadExtension(
+      test_data_dir_.AppendASCII("bindings").AppendASCII("message_sender"));
+  ASSERT_NE(nullptr, sender);
+  sender_ready.set_extension_id(sender->id());

[Devlin, 2016/07/21 00:44:54]

If the listener was satisfied before this point, this criterion has no effect (I
wonder if we should DCHECK that in the listener code - or make it work).  Since
(hopefully) no other extensions are going to send "sender_ready", I think we can
remove this line.

[asargent_no_longer_on_chrome, 2016/07/21 18:12:21]

Good point - I think in early iterations of the test I had been using multiple
ExtensionTestMessageListener's with the "listen for any message" feature, so I
needed to restrict by id, but that isn't needed anymore. 

+  ASSERT_TRUE(sender_ready.WaitUntilSatisfied());
+
+  ExtensionTestMessageListener receiver_ready("receiver_ready", false);
+  const Extension* receiver =
+      LoadExtension(test_data_dir_.AppendASCII("bindings")
+                        .AppendASCII("external_message_listener"));
+  ASSERT_NE(nullptr, receiver);
+  receiver_ready.set_extension_id(receiver->id());

[Devlin, 2016/07/21 00:44:54]

ditto

[asargent_no_longer_on_chrome, 2016/07/21 18:12:22]

Done.

+  ASSERT_TRUE(receiver_ready.WaitUntilSatisfied());
+
+  // Load the web page which tries to impersonate the sender extension via
+  // scripting iframes/child windows before they finish navigating to pages
+  // within the sender extension.
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ui_test_utils::NavigateToURL(
+      browser(),
+      embedded_test_server()->GetURL(
+          "/extensions/api_test/bindings/frames_before_navigation.html"));
+
+  bool page_success = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetWebContentsAt(0), "getResult()",
+      &page_success));
+  EXPECT_TRUE(page_success);
+
+  ExtensionTestMessageListener receiver_count(false);
+  receiver_count.set_extension_id(receiver->id());
+
+  // This should cause |sender| to send a real message over to |receiver|, at

[Devlin, 2016/07/21 00:44:54]

This part, to me, is just a little more difficult to follow.  I wonder if this
would be more clear by just using ExecuteScriptAndExtractInt() to get the value
of messagesReceived.

To me, that seems slightly more straightforward than setting up the callback ->
sender, sender message -> receiver, receiver message -> C++ that we have now.

WDYT?  This isn't that bad as it is now (just struck me as a little harder to
grok), so if you prefer this way, that's fine.

[asargent_no_longer_on_chrome, 2016/07/21 18:12:22]

I rewrote it using ExecuteScriptAndExtractInt, which does make this part of the
C++ code a little simpler. I did have to add a tiny bit of complexity to the
existing background.js file of the |receiver|, since we don't want to return the
count until after we've received the message from |sender|. Let me know if that
seems easier to grok. 

[Devlin, 2016/07/21 22:20:34]

Ah, I see, we need this in order to ensure there's no race with when the
messages could have been delivered.  Got it.

+  // which point |receiver| will call test.sendMessage to send over the total
+  // count of messages it got.
+  sender_ready.Reply(receiver->id());
+  ASSERT_TRUE(receiver_count.WaitUntilSatisfied());
+
+  // If the code is correct, |receiver| will not have received an impersonated
+  // messages sent by iframe_before_navigate.html, so the result should be 1.
+  EXPECT_EQ("1", receiver_count.message());
+}
+
 }  // namespace
 }  // namespace extensions