Update ServiceWorkerBypassFetchHandler params to use sha256 checksum

This CL updates the existing kServiceWorkerBypassFetchHandler feature params to accept sha256 script checksum hash strings as an allowlist. It renames the param from `kServiceWorkerBypassFetchHandlerBypassedOrigins` to `kServiceWorkerBypassFetchHandlerBypassedHashStrings`, and changes `ShouldBypassFetchHandlerForMainResource()` in ServiceWorkerControlleeRequestHandler to check sha256 checksum strings rather than origins to decide if the fetch handler can be bypassed or not. (cherry picked from commit 71045c0) Bug: 1371756 Change-Id: I6c9c8d520f1708e2b06d96e2e4f642d87281e792 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4190769 Reviewed-by: Arthur Sonzogni <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Minoru Chikamune <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Original-Commit-Position: refs/heads/main@{#1097785} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4199796 Commit-Queue: Arthur Sonzogni <[email protected]> Auto-Submit: Shunya Shishido <[email protected]> Bot-Commit: Rubber Stamper <[email protected]> Cr-Commit-Position: refs/branch-heads/5563@{#19} Cr-Branched-From: 3ac59a6-refs/heads/main@{#1097615}
blueboxd · Jan 30, 2023 · 608f80a · 608f80a
1 parent 0ccb621
commit 608f80a
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 93 deletions.
diff --git a/content/browser/service_worker/service_worker_browsertest.cc b/content/browser/service_worker/service_worker_browsertest.cc
@@ -4423,79 +4423,50 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerSpeculativeStartupBrowserTest,
       static_cast<int>(blink::ServiceWorkerStatusCode::kOk), 1);
 }
 
-enum class ServiceWorkerBypassFetchHandlerBypassedOriginType {
-  kBypassed,
-  kNotBypassed
-};
-
 class ServiceWorkerBypassFetchHandlerTest
     : public ServiceWorkerBrowserTest,
-      public testing::WithParamInterface<
-          std::tuple<ServiceWorkerBypassFetchHandlerBypassedOriginType, bool>> {
+      public testing::WithParamInterface<std::tuple<bool, bool>> {
  public:
   ServiceWorkerBypassFetchHandlerTest() {
     feature_list_.InitWithFeaturesAndParameters(
         {{features::kServiceWorkerBypassFetchHandler,
-          {{"origins_to_bypass", "https://a.test"},
+          {{"script_checksum_to_bypass",
+            ShouldUseValidChecksum() ? kValidChecksum : kInvalidChecksum},
            {"strategy",
             ShouldUseAllowListStrategy() ? "allowlist" : "optin"}}}},
         {});
   }
   ~ServiceWorkerBypassFetchHandlerTest() override = default;
 
-  void SetUpOnMainThread() override {
-    SetServiceWorkerContextWrapper();
-    host_resolver()->AddRule("*", "127.0.0.1");
-    https_server_.ServeFilesFromSourceDirectory(GetTestDataFilePath());
-    https_server_.SetSSLConfig(net::EmbeddedTestServer::CERT_TEST_NAMES);
-    net::test_server::RegisterDefaultHandlers(&https_server_);
-    ASSERT_TRUE(https_server()->Start());
-  }
-
-  net::EmbeddedTestServer* https_server() { return &https_server_; }
-
   WebContents* web_contents() const { return shell()->web_contents(); }
 
   RenderFrameHost* GetPrimaryMainFrame() {
     return web_contents()->GetPrimaryMainFrame();
   }
 
  protected:
-  ServiceWorkerBypassFetchHandlerBypassedOriginType GetBypassedOriginType() {
-    return std::get<0>(GetParam());
-  }
+  bool ShouldUseValidChecksum() { return std::get<0>(GetParam()); }
   bool ShouldUseAllowListStrategy() { return std::get<1>(GetParam()); }
 
  private:
   base::test::ScopedFeatureList feature_list_;
-  net::EmbeddedTestServer https_server_{net::EmbeddedTestServer::TYPE_HTTPS};
+  std::string kValidChecksum =
+      "B437DA0A66F805F079E1F371F2BFAEF4A35BB9AEEA0A85827B954B05F6D63C6C";
+  std::string kInvalidChecksum = "";
 };
 
-INSTANTIATE_TEST_SUITE_P(
-    All,
-    ServiceWorkerBypassFetchHandlerTest,
-    testing::Combine(
-        testing::Values(
-            ServiceWorkerBypassFetchHandlerBypassedOriginType::kBypassed,
-            ServiceWorkerBypassFetchHandlerBypassedOriginType::kNotBypassed),
-        testing::Bool()));
+INSTANTIATE_TEST_SUITE_P(All,
+                         ServiceWorkerBypassFetchHandlerTest,
+                         testing::Combine(testing::Bool(), testing::Bool()));
 
 IN_PROC_BROWSER_TEST_P(ServiceWorkerBypassFetchHandlerTest, UrlInAllowList) {
-  std::string origin;
-  switch (GetBypassedOriginType()) {
-    case ServiceWorkerBypassFetchHandlerBypassedOriginType::kBypassed:
-      origin = "a.test";
-      break;
-    case ServiceWorkerBypassFetchHandlerBypassedOriginType::kNotBypassed:
-      origin = "b.test";
-      break;
-  }
+  StartServerAndNavigateToSetup();
 
-  const GURL create_service_worker_url(https_server()->GetURL(
-      origin, "/service_worker/create_service_worker.html"));
-  const GURL out_scope_url(https_server()->GetURL(origin, "/empty.html"));
+  const GURL create_service_worker_url(embedded_test_server()->GetURL(
+      "/service_worker/create_service_worker.html"));
+  const GURL out_scope_url(embedded_test_server()->GetURL("/empty.html"));
   const GURL in_scope_url(
-      https_server()->GetURL(origin, "/service_worker/empty.html"));
+      embedded_test_server()->GetURL("/service_worker/empty.html"));
 
   // Register a service worker.
   WorkerRunningStatusObserver observer1(public_context());
@@ -4535,17 +4506,14 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerBypassFetchHandlerTest, UrlInAllowList) {
   EXPECT_TRUE(NavigateToURL(shell(), in_scope_url));
 
   if (ShouldUseAllowListStrategy()) {
-    switch (GetBypassedOriginType()) {
-      case ServiceWorkerBypassFetchHandlerBypassedOriginType::kBypassed:
-        // If bypassing is allowed, the service worker was bypassed and the
-        // navigation request shouldn't be handled by the fetch handler.
-        EXPECT_EQ(0, EvalJs(GetPrimaryMainFrame(), script));
-        break;
-      case ServiceWorkerBypassFetchHandlerBypassedOriginType::kNotBypassed:
-        // If bypassing is not allowed, the navigation request should be handled
-        // by the fetch handler.
-        EXPECT_EQ(1, EvalJs(GetPrimaryMainFrame(), script));
-        break;
+    if (ShouldUseValidChecksum()) {
+      // If bypassing is allowed, the service worker was bypassed and the
+      // navigation request shouldn't be handled by the fetch handler.
+      EXPECT_EQ(0, EvalJs(GetPrimaryMainFrame(), script));
+    } else {
+      // If bypassing is not allowed, the navigation request should be handled
+      // by the fetch handler.
+      EXPECT_EQ(1, EvalJs(GetPrimaryMainFrame(), script));
     }
   } else {
     // If the allowlist isn't used, the service worker was bypassed and the

diff --git a/content/browser/service_worker/service_worker_controllee_request_handler.cc b/content/browser/service_worker/service_worker_controllee_request_handler.cc
@@ -85,23 +85,18 @@ const char* FetchHandlerTypeToString(
   }
 }
 
-// Returns the list of origins in which fetch handlers are bypassed.
-const std::vector<url::Origin> FetchHandlerBypassedOrigins() {
-  std::vector<url::Origin> origins;
-  std::vector<std::string> parsed_params = base::SplitString(
-      features::kServiceWorkerBypassFetchHandlerBypassedOrigins.Get(), ",",
-      base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
-  for (const auto& it : parsed_params) {
-    const GURL url = GURL(it);
-    if (url.is_valid()) {
-      origins.push_back(url::Origin::Create(url));
-    }
-  }
-
-  return origins;
+// Returns the set of hash strings of fetch handlers which can be bypassed.
+const base::flat_set<std::string> FetchHandlerBypassedHashStrings() {
+  const static base::NoDestructor<base::flat_set<std::string>> result(
+      base::SplitString(
+          features::kServiceWorkerBypassFetchHandlerBypassedHashStrings.Get(),
+          ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY));
+
+  return *result;
 }
 
-bool ShouldBypassFetchHandlerForMainResource(const GURL& stripped_url) {
+bool ShouldBypassFetchHandlerForMainResource(
+    const std::string& sha256_script_checksum) {
   if (!base::FeatureList::IsEnabled(
           features::kServiceWorkerBypassFetchHandler)) {
     return false;
@@ -124,21 +119,14 @@ bool ShouldBypassFetchHandlerForMainResource(const GURL& stripped_url) {
               kMainResourceSkippedDueToFeatureFlag);
       return true;
     // If kAllowList, the allowlist should be specified. In this case, main
-    // resource fetch handlers are bypassed only when the url's origin is in
-    // the allowlist.
+    // resource fetch handlers are bypassed only when the sha256 checksum of the
+    // script is in the allowlist.
     case features::ServiceWorkerBypassFetchHandlerStrategy::kAllowList:
-      const static base::NoDestructor<std::vector<url::Origin>>
-          bypassed_origins(FetchHandlerBypassedOrigins());
-      for (const auto& it : *bypassed_origins) {
-        // Skip comparing port numbers because some tests run the mock HTTP
-        // server with a random port number.
-        if (it.scheme() == stripped_url.scheme() &&
-            it.host() == stripped_url.host()) {
-          RecordSkipReason(
-              ServiceWorkerControlleeRequestHandler::FetchHandlerSkipReason::
-                  kMainResourceSkippedBecauseMatchedWithAllowedOriginList);
-          return true;
-        }
+      if (FetchHandlerBypassedHashStrings().contains(sha256_script_checksum)) {
+        RecordSkipReason(
+            ServiceWorkerControlleeRequestHandler::FetchHandlerSkipReason::
+                kMainResourceSkippedBecauseMatchedWithAllowedScriptList);
+        return true;
       }
       return false;
   }
@@ -571,7 +559,8 @@ void ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion(
       // feature flag and the url.
       if (ShouldBypassFetchHandlerForMainResourceByOriginTrial(
               registration->active_version()) ||
-          ShouldBypassFetchHandlerForMainResource(stripped_url_)) {
+          ShouldBypassFetchHandlerForMainResource(
+              registration->active_version()->sha256_script_checksum())) {
         // If true, the main resource request bypasses ServiceWorker and starts
         // the worker in parallel for subsequent subresources.
         CompleteWithoutLoader();

diff --git a/content/browser/service_worker/service_worker_controllee_request_handler.h b/content/browser/service_worker/service_worker_controllee_request_handler.h
@@ -51,16 +51,17 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final {
   // 1. kSkippedForEmptyFetchHandler
   // 2. kMainResourceSkippedDueToOriginTrial
   // 3. kMainResourceSkippedDueToFeatureFlag
-  // 4. kMainResourceSkippedBecauseMatchedWithAllowedOriginList
+  // 4. kMainResourceSkippedBecauseMatchedWithAllowedScriptList
   enum class FetchHandlerSkipReason {
     kNoFetchHandler = 0,
     kNotSkipped = 1,
     kSkippedForEmptyFetchHandler = 2,
     kMainResourceSkippedDueToOriginTrial = 3,
     kMainResourceSkippedDueToFeatureFlag = 4,
-    kMainResourceSkippedBecauseMatchedWithAllowedOriginList = 5,
+    // kMainResourceSkippedBecauseMatchedWithAllowedOriginList = 5,
+    kMainResourceSkippedBecauseMatchedWithAllowedScriptList = 6,
 
-    kMaxValue = kMainResourceSkippedBecauseMatchedWithAllowedOriginList,
+    kMaxValue = kMainResourceSkippedBecauseMatchedWithAllowedScriptList,
   };
 
   // If |skip_service_worker| is true, service workers are bypassed for

diff --git a/content/public/common/content_features.cc b/content/public/common/content_features.cc
@@ -992,11 +992,15 @@ const base::FeatureParam<ServiceWorkerBypassFetchHandlerTarget>
         ServiceWorkerBypassFetchHandlerTarget::kMainResource,
         &service_worker_bypass_fetch_handler_target_options};
 
-// Define origins to bypass ServiceWorker. Origins are expected to be passed as
-// a comma separated string. e.g. https://example1.test,https://example2.test
+// The set of ServiceWorker to bypass while making navigation request.
+// They are represented by a comma separated list of HEX encoded SHA256 hash of
+// the ServiceWorker's scripts.
+// e.g.
+// 9685C8DE399237BDA6FF3AD0F281E9D522D46BB0ECFACE05E98D2B9AAE51D1EF,
+// 20F0D78B280E40C0A17ABB568ACF4BDAFFB9649ADA75B0675F962B3F4FC78EA4
 const base::FeatureParam<std::string>
-    kServiceWorkerBypassFetchHandlerBypassedOrigins{
-        &kServiceWorkerBypassFetchHandler, "origins_to_bypass", ""};
+    kServiceWorkerBypassFetchHandlerBypassedHashStrings{
+        &kServiceWorkerBypassFetchHandler, "script_checksum_to_bypass", ""};
 
 // Enables skipping the service worker fetch handler if the fetch handler is
 // identified as ignorable.

diff --git a/content/public/common/content_features.h b/content/public/common/content_features.h
@@ -257,7 +257,7 @@ CONTENT_EXPORT extern const base::FeatureParam<
     ServiceWorkerBypassFetchHandlerTarget>
     kServiceWorkerBypassFetchHandlerTarget;
 CONTENT_EXPORT extern const base::FeatureParam<std::string>
-    kServiceWorkerBypassFetchHandlerBypassedOrigins;
+    kServiceWorkerBypassFetchHandlerBypassedHashStrings;
 CONTENT_EXPORT BASE_DECLARE_FEATURE(kServiceWorkerSkipIgnorableFetchHandler);
 CONTENT_EXPORT extern const base::FeatureParam<bool> kSkipEmptyFetchHandler;
 CONTENT_EXPORT extern const base::FeatureParam<bool>