[Extensions] Split up ScriptContext::GetEffectiveDocumentURL()

ScriptContext::GetEffectiveDocumentURL() is used to
1) Classify javascript contexts (e.g., associating about:blank frames
   with an extension context), and
2) Determining an "effective" URL for script injection (to allow content
   scripts to run in about:blank frames if an extension has access to
   the parent).

However, these have different traits. For instance, for case 1), we
don't want to consider sandboxed frames (which cannot access their
parent frames) - a sandboxed frame should not inherit its privileges.
But for case 2), we want to consider sandboxed frames, so that content
scripts can run in these.

Split ScriptContext::GetEffectiveDocumentURL() into
ScriptContext::GetEffectiveDocumentURLForContext() and
ScriptContext::GetEffectiveDocumentURLForInjection() to account for
these differences; this fixes an issue where scripts wouldn't correctly
inject in sandboxed frames. Modify existing tests for both
GetEffectiveDocumentURL() and injecting scripts in sandboxed frames.

This also paves some more of the way to injecting in data: URLs.

Bug: 1108505, 55084
Change-Id: I97707b7bba520f8177f59a4568971df543f69785
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2314810
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792349}

chrome/browser/extensions/content_script_apitest.cc

@@ -972,7 +972,7 @@ IN_PROC_BROWSER_TEST_F(ContentScriptApiTest, ExecuteScriptBypassingSandbox) {
 IN_PROC_BROWSER_TEST_F(ContentScriptApiTest, InifiniteLoopInGetEffectiveURL) {
   // Create an extension that injects content scripts into about:blank frames
   // (and therefore has a chance to trigger an infinite loop in
-  // ScriptContext::GetEffectiveDocumentURL).
+  // ScriptContext::GetEffectiveDocumentURLForInjection()).
   TestExtensionDir test_dir;
   test_dir.WriteManifest(
       R"({

chrome/test/data/extensions/api_test/executescript/frame_id/test.js

@@ -23,8 +23,8 @@ var R_FRAME_TOP = /frames\.html/;
 var ID_FRAME_SRCDOC;
 var R_FRAME_SRCDOC = /about:srcdoc/;
 // Frame with (unique-origin) sandboxed about:blank.
-var ID_FRAME_UNREACHABLE;
-var R_FRAME_UNREACHABLE = /about:blank/;
+var ID_FRAME_SANDBOXED;
+var R_FRAME_SANDBOXED = /about:blank/;
 // Frame with same-origin page.
 var ID_FRAME_SECOND;
 var R_FRAME_SECOND = /frame\.html/;
@@ -92,7 +92,7 @@ chrome.test.getConfig(function(config) {
       }
 
       ID_FRAME_SRCDOC = getFrameId(R_FRAME_SRCDOC);
-      ID_FRAME_UNREACHABLE = getFrameId(R_FRAME_UNREACHABLE);
+      ID_FRAME_SANDBOXED = getFrameId(R_FRAME_SANDBOXED);
       ID_FRAME_SECOND = getFrameId(R_FRAME_SECOND);
       ID_FRAME_THIRD = getFrameId(R_FRAME_THIRD);
       ID_FRAME_NOPERMISSION = getFrameId(R_FRAME_NOPERMISSION);
@@ -125,9 +125,10 @@ function runTests(config) {
             code: 'document.URL'
           },
           pass(function(results) {
-            assertEq(4, results.length);
+            assertEq(5, results.length);
             assertTrue(matchesAny(results, R_FRAME_TOP));
             assertTrue(matchesAny(results, R_FRAME_SRCDOC));
+            assertTrue(matchesAny(results, R_FRAME_SANDBOXED));
             assertTrue(matchesAny(results, R_FRAME_SECOND));
             assertTrue(matchesAny(results, R_FRAME_THIRD));
           }));
@@ -172,14 +173,14 @@ function runTests(config) {
     function executeScriptInSandboxedFrame() {
       chrome.tabs.executeScript(
           tabId, {
-            frameId: ID_FRAME_UNREACHABLE,
+            frameId: ID_FRAME_SANDBOXED,
             matchAboutBlank: true,
             code: 'document.URL'
           },
-          fail(
-              'Cannot access "about:blank" at origin "null". Extension must ' +
-              'have permission to access the frame\'s origin, and ' +
-              'matchAboutBlank must be true.'));
+          pass((results) => {
+            assertEq(1, results.length);
+            assertTrue(matchesAny(results, R_FRAME_SANDBOXED));
+          }));
     },
 
     function executeScriptInSubFrame() {
@@ -271,9 +272,10 @@ function runTests(config) {
       insertCSS(
           tabId, {frameId: 0, matchAboutBlank: true, allFrames: true},
           pass(function(results) {
-            assertEq(4, results.length);
+            assertEq(5, results.length);
             assertTrue(matchesAny(results, R_FRAME_TOP));
             assertTrue(matchesAny(results, R_FRAME_SRCDOC));
+            assertTrue(matchesAny(results, R_FRAME_SANDBOXED));
             assertTrue(matchesAny(results, R_FRAME_SECOND));
             assertTrue(matchesAny(results, R_FRAME_THIRD));
           }));
@@ -308,16 +310,17 @@ function runTests(config) {
     },
 
     function insertCSSInSandboxedFrame() {
-      chrome.tabs.insertCSS(
+      insertCSS(
           tabId, {
-            frameId: ID_FRAME_UNREACHABLE,
+            frameId: ID_FRAME_SANDBOXED,
             matchAboutBlank: true,
+            allFrames: true,
             code: 'body{color:red}'
           },
-          fail(
-              'Cannot access "about:blank" at origin "null". Extension must ' +
-              'have permission to access the frame\'s origin, and ' +
-              'matchAboutBlank must be true.'));
+          pass((results) => {
+            assertEq(1, results.length);
+            assertTrue(matchesAny(results, R_FRAME_SANDBOXED));
+          }));
     },
 
     function insertCSSInSubFrame() {

extensions/browser/url_loader_factory_manager.cc

@@ -404,12 +404,12 @@ void MarkIsolatedWorldsAsRequiringSeparateURLLoaderFactory(
 // first non-about-scheme document and returns its url.  Otherwise, simply
 // returns |document_url|.
 //
-// This function approximates ScriptContext::GetEffectiveDocumentURL from the
-// renderer side.  Unlike the renderer version of this code (in
-// ScriptContext::GetEffectiveDocumentURL) the code below doesn't consider
-// whether security origin of |frame| can access |next_candidate|.  This is
-// okay, because our only caller (DoesContentScriptMatchNavigatingFrame) expects
-// false positives.
+// This function approximates
+// ScriptContext::GetEffectiveDocumentURLForInjection() from the renderer side.
+// Unlike the renderer code, this just iterates up frame tree, and doesn't look
+// at the effective or precursor origin of the frame. This is okay, because our
+// only caller (DoesContentScriptMatchNavigatingFrame()) expects false
+// positives.
 GURL GetEffectiveDocumentURL(content::RenderFrameHost* frame,
                              const GURL& document_url,
                              bool match_about_blank) {

extensions/renderer/dispatcher.cc

@@ -604,11 +604,16 @@ void Dispatcher::WillDestroyServiceWorkerContextOnWorkerThread(
 }
 
 void Dispatcher::DidCreateDocumentElement(blink::WebLocalFrame* frame) {
-  // Note: use GetEffectiveDocumentURL not just frame->document()->url()
-  // so that this also injects the stylesheet on about:blank frames that
-  // are hosted in the extension process.
-  GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL(
-      frame, frame->GetDocument().Url(), true /* match_about_blank */);
+  // Note: use GetEffectiveDocumentURLForContext() and not just
+  // frame->document()->url() so that this also injects the stylesheet on
+  // about:blank frames that are hosted in the extension process. (Even though
+  // this is used to determine whether to inject a stylesheet, we don't use
+  // GetEffectiveDocumentURLForInjection() because we inject based on whether
+  // it is an extension context, rather than based on the extension's injection
+  // permissions.)
+  GURL effective_document_url =
+      ScriptContext::GetEffectiveDocumentURLForContext(
+          frame, frame->GetDocument().Url(), true /* match_about_blank */);
 
   const Extension* extension =
       RendererExtensionRegistry::Get()->GetExtensionOrAppByURL(

extensions/renderer/native_extension_bindings_system.cc

@@ -970,7 +970,8 @@ void NativeExtensionBindingsSystem::UpdateContentCapabilities(
     // We allow about:blank pages to take on the privileges of their parents if
     // they aren't sandboxed.
     if (web_frame && !web_frame->GetSecurityOrigin().IsOpaque())
-      url = ScriptContext::GetEffectiveDocumentURL(web_frame, url, true);
+      url = ScriptContext::GetEffectiveDocumentURLForContext(web_frame, url,
+                                                             true);
     const ContentCapabilitiesInfo& info =
         ContentCapabilitiesInfo::Get(extension.get());
     if (info.url_patterns.MatchesURL(url)) {

extensions/renderer/programmatic_script_injector.cc

@@ -80,8 +80,9 @@ PermissionsData::PageAccess ProgrammaticScriptInjector::CanExecuteOnFrame(
   if (url_.SchemeIs(url::kAboutScheme)) {
     origin_for_about_error_ = frame->GetSecurityOrigin().ToString().Utf8();
   }
-  GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL(
-      frame, frame->GetDocument().Url(), params_->match_about_blank);
+  GURL effective_document_url =
+      ScriptContext::GetEffectiveDocumentURLForInjection(
+          frame, frame->GetDocument().Url(), params_->match_about_blank);
   if (params_->is_web_view) {
     if (frame->Parent()) {
       // This is a subframe inside <webview>, so allow it.

extensions/renderer/script_context.h

@@ -192,6 +192,10 @@ class ScriptContext {
     DISALLOW_COPY_AND_ASSIGN(ScopedFrameDocumentLoader);
   };
 
+  // TODO(devlin): Move all these Get*URL*() methods out of here? While they are
+  // vaguely ScriptContext related, there's enough here that they probably
+  // warrant another class or utility file.
+
   // Utility to get the URL we will match against for a frame. If the frame has
   // committed, this is the commited URL. Otherwise it is the provisional URL.
   // The returned URL may be invalid.
@@ -204,12 +208,28 @@ class ScriptContext {
   // this instead of GetDocumentLoaderURLForFrame.
   static GURL GetAccessCheckedFrameURL(const blink::WebLocalFrame* frame);
 
-  // Returns the first non-about:-URL in the document hierarchy above and
-  // including |frame|. The document hierarchy is only traversed if
-  // |document_url| is an about:-URL and if |match_about_blank| is true.
-  static GURL GetEffectiveDocumentURL(blink::WebLocalFrame* frame,
-                                      const GURL& document_url,
-                                      bool match_about_blank);
+  // Used to determine the "effective" URL in context classification, such as to
+  // associate an about:blank frame in an extension context with its extension.
+  // If |document_url| is an about: URL, returns the "owner" of the about:
+  // frame (i.e., the one that can access it). This may not be the immediate
+  // parent. Returns |document_url| if it is not an about: URL, if
+  // |match_about_blank| is false, or if a suitable parent cannot be found.
+  // Will not check parent contexts that cannot be accessed (as is the case
+  // for sandboxed frames).
+  static GURL GetEffectiveDocumentURLForContext(blink::WebLocalFrame* frame,
+                                                const GURL& document_url,
+                                                bool match_about_blank);
+
+  // Used to determine the "effective" URL for extension script injection.
+  // If |document_url| is an about: URL, returns the "owner" of the about:
+  // frame (i.e., the one that can access it). This may not be the immediate
+  // parent. Returns |document_url| if it is not an about: URL, if
+  // |match_about_blank| is false, or if a suitable parent cannot be found.
+  // Considers parent contexts that cannot be accessed (as is the case for
+  // sandboxed frames).
+  static GURL GetEffectiveDocumentURLForInjection(blink::WebLocalFrame* frame,
+                                                  const GURL& document_url,
+                                                  bool match_about_blank);
 
   // Grants a set of content capabilities to this context.
   void set_content_capabilities(APIPermissionSet capabilities) {

extensions/renderer/script_context_browsertest.cc

@@ -18,8 +18,12 @@ namespace {
 
 class ScriptContextTest : public ChromeRenderViewTest {
  protected:
-  GURL GetEffectiveDocumentURL(WebLocalFrame* frame) {
-    return ScriptContext::GetEffectiveDocumentURL(
+  GURL GetEffectiveDocumentURLForContext(WebLocalFrame* frame) {
+    return ScriptContext::GetEffectiveDocumentURLForContext(
+        frame, frame->GetDocument().Url(), true);
+  }
+  GURL GetEffectiveDocumentURLForInjection(WebLocalFrame* frame) {
+    return ScriptContext::GetEffectiveDocumentURLForInjection(
         frame, frame->GetDocument().Url(), true);
   }
 };
@@ -31,16 +35,18 @@ TEST_F(ScriptContextTest, GetEffectiveDocumentURL) {
   GURL srcdoc_url("about:srcdoc");
 
   const char frame_html[] =
-      "<iframe name='frame1' srcdoc=\""
-      "  <iframe name='frame1_1'></iframe>"
-      "  <iframe name='frame1_2' sandbox=''></iframe>"
-      "\"></iframe>"
-      "<iframe name='frame2' sandbox='' srcdoc=\""
-      "  <iframe name='frame2_1'></iframe>"
-      "\"></iframe>"
-      "<iframe name='frame3'></iframe>";
-
-  const char frame3_html[] = "<iframe name='frame3_1'></iframe>";
+      R"(<iframe name='frame1' srcdoc="
+           <iframe name='frame1_1'></iframe>
+           <iframe name='frame1_2' sandbox=''></iframe>
+         "></iframe>
+         <iframe name='frame2' sandbox='' srcdoc="
+           <iframe name='frame2_1'></iframe>
+         "></iframe>
+         <iframe name='frame3'></iframe>)";
+
+  const char frame3_html[] =
+      R"(<iframe name='frame3_1'></iframe>
+         <iframe name='frame3_2' sandbox=''></iframe>)";
 
   WebLocalFrame* frame = GetMainFrame();
   ASSERT_TRUE(frame);
@@ -77,25 +83,44 @@ TEST_F(ScriptContextTest, GetEffectiveDocumentURL) {
   WebLocalFrame* frame3_1 = frame3->FirstChild()->ToWebLocalFrame();
   ASSERT_TRUE(frame3_1);
   ASSERT_EQ("frame3_1", frame3_1->AssignedName());
+  WebLocalFrame* frame3_2 = frame3_1->NextSibling()->ToWebLocalFrame();
+  ASSERT_TRUE(frame3_2);
+  ASSERT_EQ("frame3_2", frame3_2->AssignedName());
 
   // Top-level frame
-  EXPECT_EQ(GetEffectiveDocumentURL(frame), top_url);
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForContext(frame));
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForInjection(frame));
   // top -> srcdoc = inherit
-  EXPECT_EQ(GetEffectiveDocumentURL(frame1), top_url);
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForContext(frame1));
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForInjection(frame1));
   // top -> srcdoc -> about:blank = inherit
-  EXPECT_EQ(GetEffectiveDocumentURL(frame1_1), top_url);
-  // top -> srcdoc -> about:blank sandboxed = same URL
-  EXPECT_EQ(GetEffectiveDocumentURL(frame1_2), blank_url);
-
-  // top -> srcdoc [sandboxed] = same URL
-  EXPECT_EQ(GetEffectiveDocumentURL(frame2), srcdoc_url);
-  // top -> srcdoc [sandboxed] -> about:blank = same URL
-  EXPECT_EQ(GetEffectiveDocumentURL(frame2_1), blank_url);
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForContext(frame1_1));
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForInjection(frame1_1));
+  // top -> srcdoc -> about:blank sandboxed = same URL when classifying
+  // contexts, but inherited url when injecting scripts.
+  EXPECT_EQ(blank_url, GetEffectiveDocumentURLForContext(frame1_2));
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForInjection(frame1_2));
+
+  // top -> srcdoc [sandboxed] = same URL when classifying contexts,
+  // but inherited url when injecting scripts.
+  EXPECT_EQ(srcdoc_url, GetEffectiveDocumentURLForContext(frame2));
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForInjection(frame2));
+  // top -> srcdoc [sandboxed] -> about:blank = same URL when classifying
+  // contexts, but inherited url when injecting scripts.
+  EXPECT_EQ(blank_url, GetEffectiveDocumentURLForContext(frame2_1));
+  EXPECT_EQ(top_url, GetEffectiveDocumentURLForInjection(frame2_1));
 
   // top -> different origin = different origin
-  EXPECT_EQ(GetEffectiveDocumentURL(frame3), different_url);
-  // top -> different origin -> about:blank = inherit
-  EXPECT_EQ(GetEffectiveDocumentURL(frame3_1), different_url);
+  EXPECT_EQ(different_url, GetEffectiveDocumentURLForContext(frame3));
+  EXPECT_EQ(different_url, GetEffectiveDocumentURLForInjection(frame3));
+  // top -> different origin -> about:blank = inherit (from different origin)
+  EXPECT_EQ(different_url, GetEffectiveDocumentURLForContext(frame3_1));
+  EXPECT_EQ(different_url, GetEffectiveDocumentURLForInjection(frame3_1));
+  // top -> different origin -> about:blank sandboxed = same URL when
+  // classifying contexts, but inherited (from different origin) url when
+  // injecting scripts.
+  EXPECT_EQ(blank_url, GetEffectiveDocumentURLForContext(frame3_2));
+  EXPECT_EQ(different_url, GetEffectiveDocumentURLForInjection(frame3_2));
 }
 
 TEST_F(ScriptContextTest, GetMainWorldContextForFrame) {

extensions/renderer/script_context_set.cc

@@ -50,7 +50,7 @@ ScriptContext* ScriptContextSet::Register(
       extension, world_id, frame_url, frame->GetDocument().GetSecurityOrigin());
   Feature::Context effective_context_type = ClassifyJavaScriptContext(
       effective_extension, world_id,
-      ScriptContext::GetEffectiveDocumentURL(frame, frame_url, true),
+      ScriptContext::GetEffectiveDocumentURLForContext(frame, frame_url, true),
       frame->GetDocument().GetSecurityOrigin());
 
   ScriptContext* context =
@@ -154,8 +154,8 @@ const Extension* ScriptContextSet::GetExtensionFromFrameAndWorld(
     // an about:blank script context that is scriptable by their parent/opener
     // before they finish navigating.
     GURL frame_url = ScriptContext::GetAccessCheckedFrameURL(frame);
-    frame_url = ScriptContext::GetEffectiveDocumentURL(frame, frame_url,
-                                                       use_effective_url);
+    frame_url = ScriptContext::GetEffectiveDocumentURLForContext(
+        frame, frame_url, use_effective_url);
     extension_id =
         RendererExtensionRegistry::Get()->GetExtensionOrAppIDByURL(frame_url);
   }

extensions/renderer/user_script_injector.cc

@@ -201,8 +201,10 @@ PermissionsData::PageAccess UserScriptInjector::CanExecuteOnFrame(
                    : PermissionsData::PageAccess::kDenied;
   }
 
-  GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL(
-      web_frame, web_frame->GetDocument().Url(), script_->match_about_blank());
+  GURL effective_document_url =
+      ScriptContext::GetEffectiveDocumentURLForInjection(
+          web_frame, web_frame->GetDocument().Url(),
+          script_->match_about_blank());
 
   return injection_host->CanExecuteOnFrame(
       effective_document_url,

extensions/renderer/user_script_set.cc

@@ -215,8 +215,9 @@ std::unique_ptr<ScriptInjection> UserScriptSet::GetInjectionForScript(
     injection_host.reset(new WebUIInjectionHost(host_id));
   }
 
-  GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL(
-      web_frame, document_url, script->match_about_blank());
+  GURL effective_document_url =
+      ScriptContext::GetEffectiveDocumentURLForInjection(
+          web_frame, document_url, script->match_about_blank());
 
   bool is_subframe = web_frame->Parent();
   if (!script->MatchesDocument(effective_document_url, is_subframe))