Fix loading success.html at the end of chrome://chrome-signin flow

extensions/browser/url_request_util.cc

Index: extensions/browser/url_request_util.cc
diff --git a/extensions/browser/url_request_util.cc b/extensions/browser/url_request_util.cc
index 20e32c61b77d19208bb7ce156c7bc4a895d2415c..901dd8d25754e6604779cef6bcd9b4f13648cae1 100644
--- a/extensions/browser/url_request_util.cc
+++ b/extensions/browser/url_request_util.cc
@@ -155,7 +155,13 @@ bool AllowCrossRendererResourceLoadHelper(bool is_guest,
   if (is_guest) {
     // An extension's resources should only be accessible to WebViews owned by
     // that extension.
-    if (owner_extension != extension) {
+    // The only exception is the sign-in extension loaded by
+    // chrome://chrome-signin, which is allowed to load web-accessible resources
+    // even if the webview that is loading the resource does not belog to the

[Charlie Reis, 2017/04/25 16:52:54]

nit: belong

[msarda, 2017/04/26 11:14:35]

Done.

+    // extension.
+    bool is_signin_extension =
+        extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik";

[Charlie Reis, 2017/04/25 16:52:54]

Is there a constant we can use here rather than a magic string?  Or is it in a
layer we can't access?

[msarda, 2017/04/26 11:14:35]

The constant is defined in
https://cs.chromium.org/chromium/src/chrome/browser/extensions/signin/gaia_au...
, but that is not accessible in //extensions/. I think having the string here
makes more sense than having a client/provider API as I do not think this should
be something configurable by the user of extensions.

+    if (owner_extension != extension && !is_signin_extension) {

[Charlie Reis, 2017/04/25 16:52:54]

Devlin: What do you think about whitelisting signin vs removing the whole
owner_extension check for now (which you suggested on the bug)?  I wasn't in
that review, so I'm not sure how important the check is.

[Devlin, 2017/04/26 01:59:37]

Lucas knows more about the history of this check than I do.  I was worried that
this might be something other apps are legitimately doing, but it seems like
maybe not?  If that's the case, I'm fine with a whitelist approach.

[msarda, 2017/04/26 11:14:34]

Lucas: Please advise on whether to keep this code as is or remove the it.

[lfg, 2017/04/26 16:24:18]

We should use the whitelist approach instead of removing the check. Removing the
owners check would mean that any app could load resources listed in the manifest
from any other app.

[Charlie Reis, 2017/04/26 20:10:25]

Acknowledged.

       *allowed = false;
       return true;
     }