Add scheme exceptions for isSecureContext

chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc

Index: chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc
diff --git a/chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc b/chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a6180b482fc0fc16a1a4a12f54686db1531b347a
--- /dev/null
+++ b/chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc
@@ -0,0 +1,135 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
+#include "chrome/common/chrome_switches.h"

[Devlin, 2015/10/14 15:24:45]

Do you need all three of these switches.h files?

[jww, 2015/10/15 18:19:41]

Nope, removed chrome_switches.h and content_switches.h.

+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/common/content_switches.h"
+#include "extensions/test/result_catcher.h"
+#include "media/base/media_switches.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+
+namespace extensions {
+
+namespace {
+
+// Used to observe the creation of permission prompt without responding.
+class PermissionRequestObserver : public PermissionBubbleManager::Observer {
+ public:
+  explicit PermissionRequestObserver(content::WebContents* web_contents)
+      : bubble_manager_(PermissionBubbleManager::FromWebContents(web_contents)),
+        request_shown_(false),
+        message_loop_runner_(new content::MessageLoopRunner) {

[Devlin, 2015/10/14 15:24:45]

nitty nit: though it doesn't matter for MessageLoopRunner, prefer
value-initialization for types (i.e., new MessageLoopRunner()) to avoid a random
nasty pitfall.

Though this is probably moot with below.

[jww, 2015/10/15 18:19:41]

Acknowledged.

+    bubble_manager_->AddObserver(this);
+  }
+  ~PermissionRequestObserver() override {
+    // Safe to remove twice if it happens.
+    bubble_manager_->RemoveObserver(this);
+  }
+
+  void Wait() { message_loop_runner_->Run(); }

[Devlin, 2015/10/14 15:24:45]

It's usually better to have a check that the request hasn't been shown yet (if
code executes synchronously in the future, it could be) before running the
message loop, to avoid running endlessly.

That said, I don't think this method is actually used?  (Which means it and the
message loop runner can be removed).

[jww, 2015/10/15 18:19:41]

Yup, I copied this from some WebRTC code and, apparently, didn't go over it
carefully enough :-P Sorry! Removing.

+
+  bool request_shown() const { return request_shown_; }
+
+ private:
+  // PermissionBubbleManager::Observer
+  void OnBubbleAdded() override {
+    request_shown_ = true;
+    bubble_manager_->RemoveObserver(this);
+    message_loop_runner_->Quit();
+  }
+
+  PermissionBubbleManager* bubble_manager_;
+  bool request_shown_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver);
+};
+
+}  // namespace
+
+class WebRtcFromWebAccessibleResourceTest : public ExtensionApiTest {
+ public:
+  WebRtcFromWebAccessibleResourceTest() {}
+  ~WebRtcFromWebAccessibleResourceTest() override {}
+
+  // InProcessBrowserTest:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ExtensionApiTest::SetUpCommandLine(command_line);
+
+    // This test expects to run with fake devices.

[Devlin, 2015/10/14 15:24:45]

nitty nit: this comment tells us nothing that the code doesn't. :)  It should
either be expanded, or removed (I'd probably just remove it).

[jww, 2015/10/15 18:19:41]

Done.

+    command_line->AppendSwitch(switches::kUseFakeDeviceForMediaStream);
+  }
+
+ protected:
+  GURL GetTestServerInsecureUrl(const std::string& path) {
+    GURL url = embedded_test_server()->GetURL(path);
+
+    GURL::Replacements replace_host_and_scheme;
+    replace_host_and_scheme.SetHostStr("a.com");
+    replace_host_and_scheme.SetSchemeStr("http");
+    url = url.ReplaceComponents(replace_host_and_scheme);
+
+    return url;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WebRtcFromWebAccessibleResourceTest);
+};
+
+// Verify that a chrome-extension:// web accessible URL can successfully access
+// getUserMedia(), even if it is embedded in an insecure context.
+IN_PROC_BROWSER_TEST_F(WebRtcFromWebAccessibleResourceTest,
+                       GetUserMediaInWebAccessibleResourceSuccess) {

[robwu, 2015/10/14 09:05:35]

Could you also add a test for <iframe sandbox src=chrome-extension: ...>?

[jww, 2015/10/15 18:19:41]

Unfortunately, I can't, but that's not a change from the status quo. You can't
use getUserMedia in a sandbox because MediaStreamDispatchHost::IsURLAllowed
returns false for sandboxed iframes (unless 'allow-same-origin' is given):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

+  host_resolver()->AddRule("a.com", "127.0.0.1");
+  ASSERT_TRUE(StartEmbeddedTestServer());
+
+  ASSERT_TRUE(RunExtensionTest("webrtc_from_web_accessible_resource"))
+      << message_;
+
+  GURL url = GetTestServerInsecureUrl("/extensions/test_file.html?succeed");
+  ui_test_utils::NavigateToURL(browser(), url);

[Devlin, 2015/10/14 15:24:45]

Is there a race condition in these steps?  If we navigate to the url and the
content script executes, then isn't there a chance gUM will be called before we
set the auto response and set up the result catcher?  Or am I missing something?

(The fix would just be to set the auto response and initialize the result
catcher above the navigation.)

[jww, 2015/10/15 18:19:41]

I believe that's not a problem because it will sit and wait for a
PermissionBubbleManager response, which we give below.

[Devlin, 2015/10/15 18:34:11]

It will sit and wait, but set_auto_response_for_test() doesn't seem to do
anything for existing requests.  So my worry is that if we actually get to the
point of asking for permission before we set the auto response, then the prompt
will just sit there indefinitely.

Is this incorrect?

[robwu, 2015/10/15 20:56:11]

Devlin's comment seems to be right.

To avoid any races, the content script should not call gUM until the
autoresponder has been set up. This can be achieved as follows:


// contentscript.js
chrome.test.sendMessage('onReady', function() {
    // Start gUM test here
});


// webrtc_from_web_accessible_resource_browsertest.cc
ExtensionTestMessageListener listener("onReady", false);
... RunExtensionTest, Navigate, set up autoresponder, etc...
// Now allow the content script to proceed.
EXPECT_TRUE(listener.WaitUntilSatisfied());
listener.Reply("");
... Wait until completion + run assertions...

[Devlin, 2015/10/15 21:21:02]

A simpler solution might be to just set the auto response (and initialize the
result catcher) before navigating (assuming autoresponses aren't cleared on
navigation - I don't think they are).  But either one works - whatever's
easiest.

[jww, 2015/10/15 22:38:06]

Yup, I took Devlin's suggested approach.

+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  PermissionBubbleManager* bubble_manager =
+      PermissionBubbleManager::FromWebContents(web_contents);
+  bubble_manager->set_auto_response_for_test(
+      PermissionBubbleManager::ACCEPT_ALL);
+  PermissionRequestObserver permissionRequestObserver(web_contents);

[Devlin, 2015/10/15 18:34:11]

as I was responding to the comment above, I realized this is camelcase.  Prefer
unix_hacker_style for variables.

[jww, 2015/10/15 22:38:06]

Done.

+
+  extensions::ResultCatcher catcher;
+  ASSERT_TRUE(catcher.GetNextResult());
+  EXPECT_TRUE(permissionRequestObserver.request_shown());
+}
+
+// Verify that a chrome-extension:// web accessible URL will fail to access
+// getUserMedia() if it is denied by the permission bubble, even if it is
+// embedded in an insecure context.
+IN_PROC_BROWSER_TEST_F(WebRtcFromWebAccessibleResourceTest,
+                       GetUserMediaInWebAccessibleResourceFail) {
+  host_resolver()->AddRule("a.com", "127.0.0.1");
+  ASSERT_TRUE(StartEmbeddedTestServer());
+
+  ASSERT_TRUE(RunExtensionTest("webrtc_from_web_accessible_resource"))
+      << message_;
+
+  GURL url = GetTestServerInsecureUrl("/extensions/test_file.html?fail");
+  ui_test_utils::NavigateToURL(browser(), url);
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  PermissionBubbleManager* bubble_manager =
+      PermissionBubbleManager::FromWebContents(web_contents);
+  bubble_manager->set_auto_response_for_test(PermissionBubbleManager::DENY_ALL);
+  PermissionRequestObserver permissionRequestObserver(web_contents);
+
+  extensions::ResultCatcher catcher;
+  ASSERT_TRUE(catcher.GetNextResult());
+  EXPECT_TRUE(permissionRequestObserver.request_shown());
+}
+
+}  // namespace extensions