Fix extension bindings injection for iframes

extensions/renderer/script_context_set.cc

Index: extensions/renderer/script_context_set.cc
diff --git a/extensions/renderer/script_context_set.cc b/extensions/renderer/script_context_set.cc
index adefb3838becf53f84c3e4b2443eaf9dc34a45e5..6607e50d3b9af86a00f5bca1f937374ff5178bfe 100644
--- a/extensions/renderer/script_context_set.cc
+++ b/extensions/renderer/script_context_set.cc
@@ -140,8 +140,18 @@ const Extension* ScriptContextSet::GetExtensionFromFrameAndWorld(
     // Isolated worlds (content script).
     extension_id = ScriptInjection::GetHostIdForIsolatedWorld(world_id);
   } else {
-    // Extension pages (chrome-extension:// URLs).
-    GURL frame_url = ScriptContext::GetDataSourceURLForFrame(frame);
+    // Top-level frames pointing at chrome-extension:// urls start out with an
+    // url of about:blank, so we want to use the data source url they are
+    // loading for determining the associated Extension object. On the other
+    // hand, iframes also start out with an url of about:blank, but during
+    // resource fetching they have an javascript context which is scriptable by
+    // their parent, which then gets replaced with a new one once resource
+    // loading is finished, so we *don't* want to use their data source url
+    // for determining the associated Extension.
+    GURL frame_url = frame->parent()

[Devlin, 2016/07/13 23:19:09]

Hmm... so what happens if the web page opens a popup and then navigates it?  In
that case, I think the parent() would be null, but its opener would still be the
current page, so I think scripting would still be allowed, right?

[asargent_no_longer_on_chrome, 2016/07/14 16:38:30]

I just double-checked that using a new window does not have the vulnerability,
but I need to look more closely to explain why - it might be because new windows
don't get the same "briefly create an about:blank script context and allow
parent to script it" behavior that iframes get. I may also add a test for this. 

[Devlin, 2016/07/14 16:46:35]

As long as we can test and assert that it's the case, we're probably okay.  But
I'd be very interested to know why it doesn't reproduce.

[asargent_no_longer_on_chrome, 2016/07/14 21:38:07]

Ok, so the web page is allowed to run script in the new window before it
finishes navigation, just like with iframes. But, it turns out that unlike in
the case of iframes, the initial WebLocalFrame for the new window's about:blank
js context before it does the navigation has a document url that is completely
empty (instead of about:blank), and both its data source and provisional data
sources have empty urls also. So we end up using the empty string as the
frame_url in to the call to GetExtensionOrAppIDByURL below, and this just
returns an empty extension_id. 

The frame just comes to us this way in Dispatcher::DidCreateScriptContext, and I
haven't traced through the Blink code to understand why this is the case. But
now I'm rethinking the nature of my patch - it fixes the problem but it would be
nice to find a way to solve the problem that seems more solid. I'm going to look
into possibly using the WebSecurityOrigin instead, since that seems to be more
reliable than the url or data source url. The whole effective document url thing
below (for implementing about:blank frame matching for context scripts) may
complicate things though. 

[asargent_no_longer_on_chrome, 2016/07/20 21:36:47]

It turned I was able to use the WebSecurityOrigin on the frame, and compare that
against the data source url. 

+                         ? GURL(frame->document().url())
+                         : ScriptContext::GetDataSourceURLForFrame(frame);
+
     frame_url = ScriptContext::GetEffectiveDocumentURL(frame, frame_url,
                                                        use_effective_url);
     extension_id =