Block content scripts from executing until user grants permission

extensions/renderer/script_injection.cc

Index: extensions/renderer/script_injection.cc
diff --git a/extensions/renderer/script_injection.cc b/extensions/renderer/script_injection.cc
index 4db534a1f68c1934127ec61a5617c701d28aa40b..bee9ed843e3d74767201d234304bc12b16b4cf2e 100644
--- a/extensions/renderer/script_injection.cc
+++ b/extensions/renderer/script_injection.cc
@@ -9,6 +9,7 @@
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram.h"
 #include "content/public/common/url_constants.h"
+#include "content/public/renderer/render_view.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/permissions/permissions_data.h"
@@ -20,6 +21,7 @@
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebScriptSource.h"
+#include "third_party/WebKit/public/web/WebView.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "url/gurl.h"
 
@@ -27,6 +29,9 @@ namespace extensions {
 
 namespace {
 
+// The id of the next pending injection.
+int64 g_next_pending_id = 0;
+
 // These two strings are injected before and after the Greasemonkey API and
 // user script to wrap it in an anonymous scope.
 const char kUserScriptHead[] = "(function (unsafeWindow) {\n";
@@ -57,6 +62,40 @@ ScriptInjection::ScriptsRunInfo::ScriptsRunInfo() : num_css(0u), num_js(0u) {
 ScriptInjection::ScriptsRunInfo::~ScriptsRunInfo() {
 }
 
+struct ScriptInjection::PendingInjection {
+  PendingInjection(const blink::WebString& web_frame_name,
+                   UserScript::RunLocation run_location,
+                   int page_id);
+  ~PendingInjection();
+
+  // The globally-unique id of this request.
+  int64 id;

[not at google - send to devlin, 2014/05/21 15:01:07]

it seems like |page_id| is enough to distinguish requests? you're checking
script injection per extension, so if an extension is allowed then all its
script injections (and into the future, for that page) should be allowed too.

(note to future self/devlin: check for race conditions with active tab and/or
just generally optional permissions)

having this modelled as a request ID can't *hurt* I suppose but it is redundant
and complicates things a little. I think the only code that would need to change
is not terminating the loop in OnContentScriptGrantedPermission?

[Devlin, 2014/05/21 17:05:11]

We'd actually need page id + extension id, because just sending back a page id
doesn't let us know which extension is approved.  Just by that, the int64 is
smaller and faster to send across (and I think int64 is preferred to even an
extension id string for IPC, but the security folks would know much better).

[not at google - send to devlin, 2014/05/21 17:36:23]

good point.

+
+  // The name of the web frame into which to inject.
+  blink::WebString web_frame_name;

[not at google - send to devlin, 2014/05/21 15:01:07]

why not just hold onto the WebFrame pointer? code below doesn't imply it's
unsafe given you're observing NotifyFrameDetached.

[Devlin, 2014/05/21 17:05:11]

Good point.  Let's try that.

+
+  // The run location to inject at.
+  // Note: This could be a lie - we might inject well after this run location
+  // has come and gone. But we need to know it to know which scripts to inject.
+  UserScript::RunLocation run_location;
+
+  // The corresponding page id, to protect against races.
+  int page_id;
+};
+
+ScriptInjection::PendingInjection::PendingInjection(
+    const blink::WebString& web_frame_name,
+    UserScript::RunLocation run_location,
+    int page_id)
+    : id(g_next_pending_id++),
+      web_frame_name(web_frame_name),
+      run_location(run_location),
+      page_id(page_id) {
+}
+
+ScriptInjection::PendingInjection::~PendingInjection() {
+}
+
 // static
 GURL ScriptInjection::GetDocumentUrlForFrame(blink::WebFrame* frame) {
   GURL data_source_url = ScriptContext::GetDataSourceURLForFrame(frame);
@@ -81,6 +120,94 @@ ScriptInjection::ScriptInjection(
 ScriptInjection::~ScriptInjection() {
 }
 
+void ScriptInjection::InjectIfAllowed(blink::WebFrame* frame,
+                                      UserScript::RunLocation run_location,
+                                      const GURL& document_url,
+                                      ScriptsRunInfo* scripts_run_info) {
+  if (!WantsToRun(frame, run_location, document_url))
+    return;
+
+  const Extension* extension = user_script_slave_->GetExtension(extension_id_);
+  DCHECK(extension);  // WantsToRun() should be false if there's no extension.

[not at google - send to devlin, 2014/05/21 15:01:07]

CHECK

[Devlin, 2014/05/21 17:05:11]

If you insist...

My thinking is generally that if you can *know* (logically) that something is a
certain way, there's no point in CHECKing (and slowing down a production build
by x fractions of a microsecond).  Instead, DCHECK, and if there's something
wrong, we'll learn about it that way.  But, anyway.  Just my $0.02.  Done.

[not at google - send to devlin, 2014/05/21 17:36:23]

I trust many things, but not the existence of extensions.

+
+  content::RenderView* top_render_view =
+      content::RenderView::FromWebView(frame->top()->view());

[not at google - send to devlin, 2014/05/21 15:01:07]

please write a nice comment explaining why you're checking the top render view
here, not the render view for |frame|.

[Devlin, 2014/05/21 17:05:11]

Done.

+  if (PermissionsData::RequiresActionForScriptExecution(extension)) {
+    int page_id = top_render_view->GetPageId();
+    ScopedVector<PendingInjection>::iterator pending_injection =
+        pending_injections_.insert(
+            pending_injections_.end(),
+            new PendingInjection(frame->uniqueName(), run_location, page_id));
+
+    top_render_view->Send(
+        new ExtensionHostMsg_RequestContentScriptPermission(
+            top_render_view->GetRoutingID(),
+            extension->id(),
+            page_id,
+            (*pending_injection)->id));
+  } else {
+    Inject(frame, run_location, scripts_run_info);
+  }
+}
+
+bool ScriptInjection::NotifyScriptPermitted(
+    int64 request_id,
+    content::RenderView* render_view,
+    ScriptsRunInfo* scripts_run_info,
+    blink::WebFrame** frame_out) {
+  if (!render_view)

[not at google - send to devlin, 2014/05/21 15:01:07]

does this actually get called with a null render view..?

[Devlin, 2014/05/21 17:05:11]

I'd certainly hope not, but I hate to make crashing assumptions. ;)  Want me to
change it to a CHECK(), omit it, or leave it as-is?

[not at google - send to devlin, 2014/05/21 17:36:23]

I'd take it out entirely. it's seemingly arbitrary to check the render view and
not, say, |scripts_run_info|. if there's a good reasons to check it (i.e. my
question) then do so but doesn't seem that way.

[Devlin, 2014/05/21 18:28:28]

Done.

+    return false;
+
+  ScopedVector<PendingInjection>::iterator iter = pending_injections_.begin();
+  while (iter != pending_injections_.end() && (*iter)->id != request_id)
+    ++iter;
+
+  // No matching request.
+  if (iter == pending_injections_.end())
+    return false;
+
+  // We found the request, so pull it out of the pending list.
+  scoped_ptr<PendingInjection> pending_injection(*iter);
+  pending_injections_.weak_erase(iter);
+
+  // Ensure the WebView, WebFrame, Extension, and Page ID all still exist and
+  // match. Otherwise, don't inject.
+  if (render_view->GetPageId() != pending_injection->page_id)
+    return false;
+
+  blink::WebView* web_view = render_view->GetWebView();
+  if (!web_view)
+    return false;
+
+  blink::WebFrame* web_frame =
+      web_view->findFrameByName(pending_injection->web_frame_name);
+  if (!web_frame)
+    return false;
+
+  const Extension* extension = user_script_slave_->GetExtension(extension_id_);
+  if (!extension)
+    return false;
+
+  // Everything matches! Inject the script.
+  if (frame_out)
+    *frame_out = web_frame;
+  Inject(web_frame, pending_injection->run_location, scripts_run_info);
+  return true;
+}
+
+void ScriptInjection::NotifyFrameDetached(blink::WebFrame* frame) {

[not at google - send to devlin, 2014/05/21 15:01:07]

ditto FrameDetached

[Devlin, 2014/05/21 17:05:11]

Done.

+  // Any pending injections associated with the given frame will never run.
+  // Remove them.
+  for (ScopedVector<PendingInjection>::iterator iter =
+           pending_injections_.begin();
+       iter != pending_injections_.end();) {
+    if ((*iter)->web_frame_name == frame->uniqueName())
+      pending_injections_.erase(iter);
+    else
+      ++iter;
+  }
+}
+
 bool ScriptInjection::WantsToRun(blink::WebFrame* frame,
                                  UserScript::RunLocation run_location,
                                  const GURL& document_url) const {