service worker: Skip service worker for all Pepper plugins.
Back in issue 413094, we decided to skip service worker for fetches from
Pepper plugins with "private permission" for security purposes. The
motivation was that Pepper plugins without "private permission" could be
assumed to enfore the same-origin policy. However, the spec has since
mandated skipping service workers for the request for the plugin itself,
i.e., the target URL of an EMBED or OBJECT element. This patch makes two
changes:
1) Requests *for* the target URL of EMBED or OBJECT element that load a
Pepper plugin skip service workers. This aligns with recent patches to
skip all EMBED or OBJECT element requests: r537245 skipped them for
embedded HTML content, and r537386 skipped them for MIME handler
plugins.
The code change is in ppb_nacl_private_impl.cc::CreateWebURLRequest.
This stops the requests for the manifest and pexe from being intercepted
by the service worker.
2) Requests *from* any Pepper plugin skip service workers. Previously,
we only skipped if the plugin had private permission, now we skip
regardless of permission.
The code change is in url_request_info_util.cc::CreateWebURLRequest.
One thing I'm not so sure about is PepperPluginInstanceImpl::Navigate
which apparently does a navigation in a frame. This is also changed to
to skip service workers (by changing the utility function
CreateWebURLRequest), but it's unclear whether that's needed.
You might ask why we don't change the service worker interception code
to just skip plugins, instead of changing the plugin callsites. This is
because at the SW interception site, we don't know whether the request
came from a plugin or not: for the manifest request, the
RequestContextType is "INTERNAL", and the ResourceType is "SUBRESOURCE".
It's also worth nothing that NetworkService/S13nServiceWorker already
skip the service worker for all these requests, since we don't hook in
at the URLRequestJob level anymore. In NS/S13nSW, we likely won't
need to set skip service worker at these callsites.
R=kinuko
TBR=bradnelson
Bug: 771933,413094
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I09db0eda46f2e7d9372495a6205f5cb0026de6c7
Reviewed-on: https://chromium-review.googlesource.com/923663
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537706}diff --git a/chrome/browser/chrome_service_worker_browsertest.cc b/chrome/browser/chrome_service_worker_browsertest.cc
index e1b6827..10fca397 100644
--- a/chrome/browser/chrome_service_worker_browsertest.cc
+++ b/chrome/browser/chrome_service_worker_browsertest.cc
@@ -395,6 +395,13 @@
}
#if BUILDFLAG(ENABLE_NACL)
+// This test registers a service worker and then loads a controlled iframe that
+// creates a PNaCl plugin in an <embed> element. Once loaded, the PNaCl plugin
+// is ordered to do a resource request for "/echo". The service worker records
+// all the fetch events it sees. Since requests for plug-ins and requests
+// initiated by plug-ins should not be interecepted by service workers, we
+// expect that the the service worker only see the navigation request for the
+// iframe.
class ChromeServiceWorkerFetchPPAPITest : public ChromeServiceWorkerFetchTest {
protected:
ChromeServiceWorkerFetchPPAPITest() {}
@@ -410,12 +417,8 @@
test_page_url_ = GetURL("/pnacl_url_loader.html");
}
- std::string GetRequestStringForPNACL(const std::string& fragment) const {
- return RequestString(test_page_url_ + fragment, "navigate", "include") +
- RequestString(GetURL("/pnacl_url_loader.nmf"), "same-origin",
- "same-origin") +
- RequestString(GetURL("/pnacl_url_loader_newlib_pnacl.pexe"),
- "same-origin", "same-origin");
+ std::string GetNavigationRequestString(const std::string& fragment) const {
+ return RequestString(test_page_url_ + fragment, "navigate", "include");
}
std::string ExecutePNACLUrlLoaderTest(const std::string& mode) {
@@ -435,153 +438,17 @@
DISALLOW_COPY_AND_ASSIGN(ChromeServiceWorkerFetchPPAPITest);
};
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest, SameOrigin) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("/echo");
- EXPECT_EQ(GetRequestStringForPNACL("#Same") +
- RequestString(GetURL("/echo"), "same-origin", "include"),
- ExecutePNACLUrlLoaderTest("Same"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest, SameOriginCORS) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("/echo");
- // request.SetAllowCrossOriginRequests(true);
- EXPECT_EQ(GetRequestStringForPNACL("#SameCORS") +
- RequestString(GetURL("/echo"), "cors", "omit"),
- ExecutePNACLUrlLoaderTest("SameCORS"));
-}
-
IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest,
- SameOriginCredentials) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("/echo");
- // request.SetAllowCredentials(true);
- EXPECT_EQ(GetRequestStringForPNACL("#SameCredentials") +
- RequestString(GetURL("/echo"), "same-origin", "include"),
- ExecutePNACLUrlLoaderTest("SameCredentials"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest,
- SameOriginCORSCredentials) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("/echo");
- // request.SetAllowCrossOriginRequests(true);
- // request.SetAllowCredentials(true);
- EXPECT_EQ(GetRequestStringForPNACL("#SameCORSCredentials") +
- RequestString(GetURL("/echo"), "cors", "include"),
- ExecutePNACLUrlLoaderTest("SameCORSCredentials"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest, OtherOrigin) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("https://www.example.com/echo");
- // This request fails because AllowCrossOriginRequests is not set.
- EXPECT_EQ(GetRequestStringForPNACL("#Other"),
- ExecutePNACLUrlLoaderTest("Other"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest, OtherOriginCORS) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("https://www.example.com/echo");
- // request.SetAllowCrossOriginRequests(true);
- EXPECT_EQ(GetRequestStringForPNACL("#OtherCORS") +
- RequestString("https://www.example.com/echo", "cors", "omit"),
- ExecutePNACLUrlLoaderTest("OtherCORS"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest,
- OtherOriginCredentials) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("https://www.example.com/echo");
- // request.SetAllowCredentials(true);
- // This request fails because AllowCrossOriginRequests is not set.
- EXPECT_EQ(GetRequestStringForPNACL("#OtherCredentials"),
- ExecutePNACLUrlLoaderTest("OtherCredentials"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPITest,
- OtherOriginCORSCredentials) {
- // In pnacl_url_loader.cc:
- // request.SetMethod("GET");
- // request.SetURL("https://www.example.com/echo");
- // request.SetAllowCrossOriginRequests(true);
- // request.SetAllowCredentials(true);
- EXPECT_EQ(
- GetRequestStringForPNACL("#OtherCORSCredentials") +
- RequestString("https://www.example.com/echo", "cors", "include"),
- ExecutePNACLUrlLoaderTest("OtherCORSCredentials"));
-}
-
-class ChromeServiceWorkerFetchPPAPIPrivateTest
- : public ChromeServiceWorkerFetchPPAPITest {
- protected:
- ChromeServiceWorkerFetchPPAPIPrivateTest() {}
- ~ChromeServiceWorkerFetchPPAPIPrivateTest() override {}
-
- void SetUpCommandLine(base::CommandLine* command_line) override {
- ChromeServiceWorkerFetchPPAPITest::SetUpCommandLine(command_line);
- // Sets this flag to test that the fetch request from the plugins with
- // private permission (PERMISSION_PRIVATE) should not go to the service
- // worker.
- command_line->AppendSwitch(switches::kEnablePepperTesting);
- }
-
- private:
- DISALLOW_COPY_AND_ASSIGN(ChromeServiceWorkerFetchPPAPIPrivateTest);
-};
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest, SameOrigin) {
- EXPECT_EQ(GetRequestStringForPNACL("#Same"),
- ExecutePNACLUrlLoaderTest("Same"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest,
- SameOriginCORS) {
- EXPECT_EQ(GetRequestStringForPNACL("#SameCORS"),
- ExecutePNACLUrlLoaderTest("SameCORS"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest,
- SameOriginCredentials) {
- EXPECT_EQ(GetRequestStringForPNACL("#SameCredentials"),
- ExecutePNACLUrlLoaderTest("SameCredentials"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest,
- SameOriginCORSCredentials) {
- EXPECT_EQ(GetRequestStringForPNACL("#SameCORSCredentials"),
- ExecutePNACLUrlLoaderTest("SameCORSCredentials"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest, OtherOrigin) {
- EXPECT_EQ(GetRequestStringForPNACL("#Other"),
- ExecutePNACLUrlLoaderTest("Other"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest,
- OtherOriginCORS) {
- EXPECT_EQ(GetRequestStringForPNACL("#OtherCORS"),
- ExecutePNACLUrlLoaderTest("OtherCORS"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest,
- OtherOriginCredentials) {
- EXPECT_EQ(GetRequestStringForPNACL("#OtherCredentials"),
- ExecutePNACLUrlLoaderTest("OtherCredentials"));
-}
-
-IN_PROC_BROWSER_TEST_F(ChromeServiceWorkerFetchPPAPIPrivateTest,
- OtherOriginCORSCredentials) {
- EXPECT_EQ(GetRequestStringForPNACL("#OtherCORSCredentials"),
- ExecutePNACLUrlLoaderTest("OtherCORSCredentials"));
+ NotInterceptedByServiceWorker) {
+ // Only the navigation to the iframe should be intercepted by the service
+ // worker. The request for the PNaCl manifest ("/pnacl_url_loader.nmf"),
+ // the request for the compiled code ("/pnacl_url_loader_newlib_pnacl.pexe"),
+ // and any other requests initiated by the plug-in ("/echo") should not be
+ // seen by the service worker.
+ const std::string fragment =
+ "NotIntercepted"; // this string is not important.
+ EXPECT_EQ(GetNavigationRequestString("#" + fragment),
+ ExecutePNACLUrlLoaderTest(fragment));
}
#endif // BUILDFLAG(ENABLE_NACL)
diff --git a/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc b/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc
index 9ef656b..4ec7b52 100644
--- a/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc
+++ b/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc
@@ -22,14 +22,7 @@
command_ = var_message.AsString();
pp::URLRequestInfo request(this);
request.SetMethod("GET");
- if (command_.find("Other") != std::string::npos)
- request.SetURL("https://www.example.com/echo");
- else
- request.SetURL("/echo");
- if (command_.find("CORS") != std::string::npos)
- request.SetAllowCrossOriginRequests(true);
- if (command_.find("Credentials") != std::string::npos)
- request.SetAllowCredentials(true);
+ request.SetURL("/echo");
loader_.Open(request,
factory_.NewCallback(&PnaclUrlLoaderInstance::OnOpen));
return;
diff --git a/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html b/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html
index 0420ab5..696d299 100644
--- a/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html
+++ b/chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html
@@ -11,6 +11,7 @@
embed.addEventListener('load', function() {
embed.postMessage(location.hash.substr(1));
}, false);
+ // The code for this plug-in is in pnacl_url_loader.cc.
embed.src = 'pnacl_url_loader.nmf';
embed.type = 'application/x-pnacl';
document.body.appendChild(embed);
diff --git a/components/nacl/renderer/ppb_nacl_private_impl.cc b/components/nacl/renderer/ppb_nacl_private_impl.cc
index 6cb4005..62dd8cae 100644
--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -343,6 +343,12 @@
network::mojom::FetchCredentialsMode::kOmit);
}
+ // Plug-ins should not load via service workers as plug-ins may have their own
+ // origin checking logic that may get confused if service workers respond with
+ // resources from another origin.
+ // https://w3c.github.io/ServiceWorker/#implementer-concerns
+ request.SetServiceWorkerMode(blink::WebURLRequest::ServiceWorkerMode::kNone);
+
return request;
}
@@ -1034,6 +1040,11 @@
CreateAssociatedURLLoader(document, gurl));
blink::WebURLRequest request = CreateWebURLRequest(document, gurl);
+ // Requests from plug-ins must skip service workers, see the comment in
+ // CreateWebURLRequest.
+ DCHECK_EQ(request.GetServiceWorkerMode(),
+ blink::WebURLRequest::ServiceWorkerMode::kNone);
+
// ManifestDownloader deletes itself after invoking the callback.
ManifestDownloader* manifest_downloader = new ManifestDownloader(
std::move(url_loader), load_manager->is_installed(),
diff --git a/content/renderer/pepper/pepper_url_loader_host.cc b/content/renderer/pepper/pepper_url_loader_host.cc
index 9b01074f..3f41b5b 100644
--- a/content/renderer/pepper/pepper_url_loader_host.cc
+++ b/content/renderer/pepper/pepper_url_loader_host.cc
@@ -257,12 +257,10 @@
web_request.SetRequestContext(WebURLRequest::kRequestContextPlugin);
web_request.SetPluginChildID(renderer_ppapi_host_->GetPluginChildId());
- // The requests from the plugins with private permission which can bypass same
- // origin must skip the ServiceWorker.
- web_request.SetServiceWorkerMode(
- host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE)
- ? WebURLRequest::ServiceWorkerMode::kNone
- : WebURLRequest::ServiceWorkerMode::kAll);
+ // Requests from plug-ins must skip service workers, see the comment in
+ // CreateWebURLRequest.
+ DCHECK_EQ(web_request.GetServiceWorkerMode(),
+ WebURLRequest::ServiceWorkerMode::kNone);
WebAssociatedURLLoaderOptions options;
if (!has_universal_access_) {
diff --git a/content/renderer/pepper/url_request_info_util.cc b/content/renderer/pepper/url_request_info_util.cc
index 1a7cad5..5a169e6 100644
--- a/content/renderer/pepper/url_request_info_util.cc
+++ b/content/renderer/pepper/url_request_info_util.cc
@@ -178,6 +178,12 @@
dest->SetSiteForCookies(frame->GetDocument().SiteForCookies());
+ // Plug-ins should not load via service workers as plug-ins may have their own
+ // origin checking logic that may get confused if service workers respond with
+ // resources from another origin.
+ // https://w3c.github.io/ServiceWorker/#implementer-concerns
+ dest->SetServiceWorkerMode(WebURLRequest::ServiceWorkerMode::kNone);
+
const std::string& headers = data->headers;
if (!headers.empty()) {
net::HttpUtil::HeadersIterator it(headers.begin(), headers.end(), "\n\r");
diff --git a/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter b/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
index 27aa0d96..3b8ed70 100644
--- a/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
+++ b/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
@@ -519,15 +519,6 @@
# Finish ServiceWorker networking glue rewrite.
# http://crbug.com/715640
--ChromeServiceWorkerFetchPPAPIPrivateTest.OtherOrigin
--ChromeServiceWorkerFetchPPAPIPrivateTest.OtherOriginCORS
--ChromeServiceWorkerFetchPPAPIPrivateTest.OtherOriginCORSCredentials
--ChromeServiceWorkerFetchPPAPIPrivateTest.OtherOriginCredentials
--ChromeServiceWorkerFetchPPAPIPrivateTest.SameOrigin
--ChromeServiceWorkerFetchPPAPIPrivateTest.SameOriginCORS
--ChromeServiceWorkerFetchPPAPIPrivateTest.SameOriginCORSCredentials
--ChromeServiceWorkerFetchPPAPIPrivateTest.SameOriginCredentials
--ChromeServiceWorkerFetchPPAPITest.SameOrigin
-ServiceWorkerAppTest.RegisterAndPostMessage
-ServiceWorkerBackgroundSyncTestWithJSBindings/ServiceWorkerBackgroundSyncTest.Sync/0
-ServiceWorkerBackgroundSyncTestWithNativeBindings/ServiceWorkerBackgroundSyncTest.Sync/0
