Restrict extensionview to chrome-extension://

extensions/browser/guest_view/extension_view/extension_view_guest.cc

Index: extensions/browser/guest_view/extension_view/extension_view_guest.cc
diff --git a/extensions/browser/guest_view/extension_view/extension_view_guest.cc b/extensions/browser/guest_view/extension_view/extension_view_guest.cc
index 952ec1331cfb1a9eacee3e86989253bd3f22326a..2ad0b4c14c94aef7ff3963fdc776f7cb21e9df5e 100644
--- a/extensions/browser/guest_view/extension_view/extension_view_guest.cc
+++ b/extensions/browser/guest_view/extension_view/extension_view_guest.cc
@@ -5,6 +5,8 @@
 #include "extensions/browser/guest_view/extension_view/extension_view_guest.h"
 
 #include "base/metrics/user_metrics.h"
+#include "components/crx_file/id_util.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/result_codes.h"
 #include "extensions/browser/api/extensions_api_client.h"
@@ -12,6 +14,7 @@
 #include "extensions/common/constants.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/strings/grit/extensions_strings.h"
+#include "net/base/net_errors.h"
 
 using content::WebContents;
 using namespace extensions::core_api;
@@ -40,19 +43,29 @@ extensions::GuestViewBase* ExtensionViewGuest::Create(
 
 void ExtensionViewGuest::NavigateGuest(const std::string& src,
                                        bool force_navigation) {
-  if (src.empty())
+  GURL url = ResolveURL(src);
+
+  // Do not allow navigating a guest to schemes other than known safe schemes.
+  bool scheme_is_blocked =
+      (!content::ChildProcessSecurityPolicy::GetInstance()->IsWebSafeScheme(

[Fady Samuel, 2015/02/17 18:48:29]

Wow, this works? chrome-extension:// is not a WebSafeScheme I think?

[apacible, 2015/02/17 20:07:30]

Yeah, it does! I know chrome:// isn't a WebSafeScheme, and checked that
something like chrome://settings gets blocked. Since we really just want
chrome-extension://extensionid/pagetonavigateto.html URLs, it may make more
sense to check for that.. something like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
? 

+           url.scheme()) &&
+       !url.SchemeIs(url::kAboutScheme)) ||
+      url.SchemeIs(url::kJavaScriptScheme);
+  if (scheme_is_blocked || !url.is_valid()) {
+    NavigateGuest(url::kAboutBlankURL, true /* force_navigation */);
     return;
+  }
 
-  GURL url(src);
-  if (!url.is_valid() && !force_navigation && (url == view_page_))
+  if (!force_navigation && (view_page_ == url))
     return;
 
-  web_contents()->GetRenderProcessHost()->FilterURL(false, &url);
-  web_contents()->GetController().LoadURL(url, content::Referrer(),
+  GURL validated_url(url);

[apacible, 2015/02/17 18:31:26]

WebViewGuest creates a validated_url here, but is there any difference between
using this and just passing in the earlier url? Is it just for readability?

[Fady Samuel, 2015/02/17 18:48:29]

That's probably some old code that didn't get refactored well somewhere along
the way.

[apacible, 2015/02/17 20:07:30]

Acknowledged.

+  web_contents()->GetRenderProcessHost()->FilterURL(false, &validated_url);
+  web_contents()->GetController().LoadURL(validated_url, content::Referrer(),
                                           ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                           std::string());
 
-  view_page_ = url;
+  view_page_ = validated_url;
 }
 
 // GuestViewBase implementation.
@@ -63,20 +76,33 @@ bool ExtensionViewGuest::CanRunInDetachedState() const {
 void ExtensionViewGuest::CreateWebContents(
     const base::DictionaryValue& create_params,
     const WebContentsCreatedCallback& callback) {
-  std::string str;
-  if (!create_params.GetString(extensionview::kAttributeSrc, &str)) {
+  // Gets the extension ID.
+  create_params.GetString(extensionview::kAttributeExtension, &extension_id_);
+
+  if (!crx_file::id_util::IdIsValid(extension_id_)) {
+    callback.Run(nullptr);
+    return;
+  }
+
+  // Gets the extension URL.
+  extension_url_ =
+      extensions::Extension::GetBaseURLFromExtensionId(extension_id_);
+
+  if (!extension_url_.is_valid()) {
     callback.Run(nullptr);
     return;
   }
 
-  GURL source(str);
-  if (!source.is_valid()) {
+  // Get the src to build URL to render.
+  std::string src;
+  if (!create_params.GetString(extensionview::kAttributeSrc, &src)) {
     callback.Run(nullptr);
     return;
   }
 
   content::SiteInstance* view_site_instance =
-      content::SiteInstance::CreateForURL(browser_context(), source);
+      content::SiteInstance::CreateForURL(browser_context(),
+                                          extension_url_);
 
   WebContents::CreateParams params(browser_context(), view_site_instance);
   params.guest_delegate = this;
@@ -129,4 +155,16 @@ void ExtensionViewGuest::ApplyAttributes(const base::DictionaryValue& params) {
   NavigateGuest(src, false /* force_navigation */);
 }
 
+GURL ExtensionViewGuest::ResolveURL(const std::string& src) {
+  if (src.empty())
+    return GURL();
+
+  GURL default_url(base::StringPrintf("%s://%s/%s",

[Fady Samuel, 2015/02/17 18:48:29]

This doesn't seem necessary to me.

return extension_url_.Resolve(src);

should do the trick I think.

Now that I think about it, you can probably do this inline in NavigateGuest.

[apacible, 2015/02/17 20:07:30]

Done, added inline.

+                                      kExtensionScheme,
+                                      extension_id_.c_str(),
+                                      src.c_str()));
+
+  return default_url.Resolve(src);
+}
+
 }  // namespace extensions