[Extensions] Add optional host permissions for Manifest V3

Add the optional_host_permissions field for Manifest V3. Permissions specified here behave the same as host permissions specified in optional_permissions for Manifest V2. Bug: 1265064 Change-Id: I5916382317fd5beda2b6908c3cb45cafdf2be341 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3558980 Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org> Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org> Cr-Commit-Position: refs/heads/main@{#988218}
chromium · Apr 2, 2022 · 614b578 · 614b578
1 parent 869c8ce
commit 614b578
Show file tree

Hide file tree

Showing 11 changed files with 140 additions and 34 deletions.
diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc
@@ -10,6 +10,7 @@
 #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
 #include "components/crx_file/id_util.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/features/simple_feature.h"
 #include "extensions/common/manifest_constants.h"
@@ -88,9 +89,14 @@ TEST_F(InitValueManifestTest, InitFromValueInvalid) {
       Testcase("init_invalid_permissions_invalid.json",
                errors::kInvalidPermissions),
       Testcase("init_invalid_host_permissions_invalid.json",
-               errors::kInvalidHostPermissions),
+               ErrorUtils::FormatErrorMessageUTF16(
+                   errors::kInvalidHostPermissions, keys::kHostPermissions)),
       Testcase("init_invalid_permissions_item_invalid.json",
                errors::kInvalidPermission),
+      Testcase(
+          "init_invalid_optional_host_permissions_invalid.json",
+          ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidHostPermissions,
+                                              keys::kOptionalHostPermissions)),
       Testcase("init_invalid_options_url_invalid.json",
                errors::kInvalidOptionsPage),
       Testcase("init_invalid_locale_invalid.json",

diff --git a/chrome/common/extensions/manifest_tests/permissions_parser_unittest.cc b/chrome/common/extensions/manifest_tests/permissions_parser_unittest.cc
@@ -65,6 +65,34 @@ TEST_F(PermissionsParserTest, RemoveOverlappingHostPermissions) {
               testing::UnorderedElementsAre("*://chromium.org/*"));
 }
 
+// Same as the above test, except host permissions are specified in
+// `host_permissions` and `optional_host_permissions` as the extension is
+// running Manifest V3.
+TEST_F(PermissionsParserTest, RemoveOverlappingHostPermissions_ManifestV3) {
+  scoped_refptr<Extension> extension(LoadAndExpectWarning(
+      "permissions_overlapping_host_permissions_mv3.json",
+      ErrorUtils::FormatErrorMessage(
+          manifest_errors::kPermissionMarkedOptionalAndRequired,
+          "https://google.com/*")));
+
+  const URLPatternSet& required_hosts =
+      PermissionsParser::GetRequiredPermissions(extension.get())
+          .explicit_hosts();
+
+  const URLPatternSet& optional_hosts =
+      PermissionsParser::GetOptionalPermissions(extension.get())
+          .explicit_hosts();
+
+  // Check that required hosts have not changed at all while
+  // "https://google.com/maps" is removed from optional hosts as it is a strict
+  // subset of hosts specified as required.
+  EXPECT_THAT(*required_hosts.ToStringVector(),
+              testing::UnorderedElementsAre("https://example.com/*",
+                                            "https://*.google.com/*"));
+  EXPECT_THAT(*optional_hosts.ToStringVector(),
+              testing::UnorderedElementsAre("*://chromium.org/*"));
+}
+
 TEST_F(PermissionsParserTest, RequiredHostPermissionsAllURLs) {
   scoped_refptr<Extension> extension(LoadAndExpectWarning(
       "permissions_all_urls_host_permissions.json",
@@ -114,11 +142,14 @@ TEST_F(PermissionsParserTest, OptionalHostPermissionsInvalidScheme) {
 }
 
 TEST_F(PermissionsParserTest, HostPermissionsKey) {
-  std::string expected_warning = ErrorUtils::FormatErrorMessage(
-      manifest_errors::kPermissionUnknownOrMalformed, "https://google.com/*");
+  std::vector<std::string> expected_warnings;
+  expected_warnings.push_back(ErrorUtils::FormatErrorMessage(
+      manifest_errors::kPermissionUnknownOrMalformed, "https://google.com/*"));
+  expected_warnings.push_back(ErrorUtils::FormatErrorMessage(
+      manifest_errors::kPermissionUnknownOrMalformed, "http://chromium.org/*"));
 
   scoped_refptr<Extension> extension(
-      LoadAndExpectWarning("host_permissions_key.json", expected_warning));
+      LoadAndExpectWarnings("host_permissions_key.json", expected_warnings));
 
   // Expect that the host specified in |host_permissions| is parsed.
   const URLPatternSet& required_hosts =
@@ -127,23 +158,39 @@ TEST_F(PermissionsParserTest, HostPermissionsKey) {
 
   EXPECT_THAT(*required_hosts.ToStringVector(),
               testing::UnorderedElementsAre("https://example.com/*"));
+
+  // Expect that the host specified in |optional_host_permissions| is parsed.
+  const URLPatternSet& optional_hosts =
+      PermissionsParser::GetOptionalPermissions(extension.get())
+          .explicit_hosts();
+
+  EXPECT_THAT(*optional_hosts.ToStringVector(),
+              testing::UnorderedElementsAre("https://optional.com/*"));
 }
 
 TEST_F(PermissionsParserTest, HostPermissionsKeyInvalidHosts) {
-  std::string expected_warning = ErrorUtils::FormatErrorMessage(
-      manifest_errors::kPermissionUnknownOrMalformed, "malformed_host");
+  std::vector<std::string> expected_warnings;
+  expected_warnings.push_back(ErrorUtils::FormatErrorMessage(
+      manifest_errors::kPermissionUnknownOrMalformed, "malformed_host"));
+  expected_warnings.push_back(ErrorUtils::FormatErrorMessage(
+      manifest_errors::kPermissionUnknownOrMalformed,
+      "optional_malformed_host"));
 
-  scoped_refptr<Extension> extension(LoadAndExpectWarning(
-      "host_permissions_key_invalid_hosts.json", expected_warning));
+  scoped_refptr<Extension> extension(LoadAndExpectWarnings(
+      "host_permissions_key_invalid_hosts.json", expected_warnings));
 }
 
 TEST_F(PermissionsParserTest, HostPermissionsKeyInvalidScheme) {
-  std::string expected_warning = ErrorUtils::FormatErrorMessage(
+  std::vector<std::string> expected_warnings;
+  expected_warnings.push_back(ErrorUtils::FormatErrorMessage(
+      manifest_errors::kInvalidPermissionScheme,
+      manifest_keys::kHostPermissions, "chrome://extensions/"));
+  expected_warnings.push_back(ErrorUtils::FormatErrorMessage(
       manifest_errors::kInvalidPermissionScheme,
-      manifest_keys::kHostPermissions, "chrome://extensions/");
+      manifest_keys::kOptionalHostPermissions, "chrome://settings/"));
 
-  scoped_refptr<Extension> extension(LoadAndExpectWarning(
-      "host_permissions_key_invalid_scheme.json", expected_warning));
+  scoped_refptr<Extension> extension(LoadAndExpectWarnings(
+      "host_permissions_key_invalid_scheme.json", expected_warnings));
 }
 
 // Tests that listing a permissions as optional when that permission cannot be

diff --git a/chrome/test/data/extensions/manifest_tests/host_permissions_key.json b/chrome/test/data/extensions/manifest_tests/host_permissions_key.json
@@ -2,11 +2,17 @@
   "name": "Host permissions manifest key",
   "version": "0.1",
   "manifest_version": 3,
-  "description": "extension with hosts in both |permissions| and |host_permissions|",
+  "description": "extension with hosts in |permissions|, |optional_permissions|, |host_permissions| and |optional_host_permissions|",
   "permissions": [
     "https://google.com/*"
   ],
+  "optional_permissions": [
+    "http://chromium.org/*"
+  ],
   "host_permissions": [
     "https://example.com/*"
+  ],
+  "optional_host_permissions": [
+    "https://optional.com/*"
   ]
 }
diff --git a/chrome/test/data/extensions/manifest_tests/host_permissions_key_invalid_hosts.json b/chrome/test/data/extensions/manifest_tests/host_permissions_key_invalid_hosts.json
@@ -2,8 +2,11 @@
   "name": "Host permissions with malformed host",
   "version": "0.1",
   "manifest_version": 3,
-  "description": "Extension with an invalid host in host_permissions",
+  "description": "Extension with an invalid host in host_permissions and optional_host_permissions",
   "host_permissions": [
     "malformed_host"
+  ],
+  "optional_host_permissions": [
+    "optional_malformed_host"
   ]
 }
diff --git a/chrome/test/data/extensions/manifest_tests/host_permissions_key_invalid_scheme.json b/chrome/test/data/extensions/manifest_tests/host_permissions_key_invalid_scheme.json
@@ -2,8 +2,11 @@
   "name": "Host permissions with invalid scheme",
   "version": "0.1",
   "manifest_version": 3,
-  "description": "Extension with a pattern with an invalid scheme in host_permissions",
+  "description": "Extension with a pattern with an invalid scheme in host_permissions and optional_host_permissions",
   "host_permissions": [
     "chrome://extensions/"
+  ],
+  "optional_host_permissions": [
+    "chrome://settings/"
   ]
 }
diff --git a/...e/test/data/extensions/manifest_tests/init_invalid_optional_host_permissions_invalid.json b/...e/test/data/extensions/manifest_tests/init_invalid_optional_host_permissions_invalid.json
@@ -0,0 +1,6 @@
+{
+  "name": "Host permissions not a list",
+  "version": "0.1",
+  "manifest_version": 3,
+  "optional_host_permissions": "Why wont this work?!"
+}
diff --git a/chrome/test/data/extensions/manifest_tests/permissions_overlapping_host_permissions_mv3.json b/chrome/test/data/extensions/manifest_tests/permissions_overlapping_host_permissions_mv3.json
@@ -0,0 +1,14 @@
+{
+  "name": "Redundant extension with optional google maps host permission",
+  "version": "0.0.1.33",
+  "manifest_version": 3,
+  "description": "Extension with overlapping required and optional host permissions",
+  "host_permissions": [
+    "https://example.com/*",
+    "https://*.google.com/*"
+  ],
+  "optional_host_permissions": [
+    "https://google.com/maps",
+    "*://chromium.org/*"
+  ]
+}
diff --git a/extensions/common/api/_manifest_features.json b/extensions/common/api/_manifest_features.json
@@ -309,6 +309,11 @@
       "extension", "legacy_packaged_app", "hosted_app", "platform_app"
     ]
   },
+  "optional_host_permissions": {
+    "channel": "stable",
+    "extension_types": ["extension"],
+    "min_manifest_version": 3
+  },
   "options_ui": {
     "channel": "stable",
     "extension_types": ["extension", "legacy_packaged_app"]

diff --git a/extensions/common/manifest_constants.cc b/extensions/common/manifest_constants.cc
@@ -99,6 +99,7 @@ const char kNaClModulesPath[] = "path";
 const char kNativelyConnectable[] = "natively_connectable";
 const char kOfflineEnabled[] = "offline_enabled";
 const char kOmniboxKeyword[] = "omnibox.keyword";
+const char kOptionalHostPermissions[] = "optional_host_permissions";
 const char kOptionalPermissions[] = "optional_permissions";
 const char kOptionsPage[] = "options_page";
 const char kOptionsUI[] = "options_ui";
@@ -391,9 +392,8 @@ const char kInvalidHomepageOverrideURL[] =
     "Invalid value for overriding homepage url: '[*]'.";
 const char kInvalidHomepageURL[] =
     "Invalid value for homepage url: '[*]'.";
-const char kInvalidHostPermission[] =
-    "Invalid value for 'host_permissions[*]'.";
-const char kInvalidHostPermissions[] = "Invalid value for 'host_permissions'.";
+const char kInvalidHostPermission[] = "Invalid value for '*[*]'.";
+const char kInvalidHostPermissions[] = "Invalid value for '*'.";
 const char kInvalidIconKey[] = "Invalid key in icons: \"*\".";
 const char kInvalidIconPath[] =
     "Invalid value for 'icons[\"*\"]'.";

diff --git a/extensions/common/manifest_constants.h b/extensions/common/manifest_constants.h
@@ -103,6 +103,7 @@ extern const char kName[];
 extern const char kNativelyConnectable[];
 extern const char kOfflineEnabled[];
 extern const char kOmniboxKeyword[];
+extern const char kOptionalHostPermissions[];
 extern const char kOptionalPermissions[];
 extern const char kOptionsPage[];
 extern const char kOptionsUI[];

diff --git a/extensions/common/manifest_handlers/permissions_parser.cc b/extensions/common/manifest_handlers/permissions_parser.cc
@@ -87,17 +87,18 @@ bool CanSpecifyHostPermission(const Extension* extension,
   return true;
 }
 
-// Parses hosts from the |keys::kHostPermissions| key in the extension's
-// manifest into |hosts|.
+// Parses hosts from `key` in the extension's manifest into |hosts|.
 bool ParseHostsFromJSON(Extension* extension,
+                        const char* key,
                         std::vector<std::string>* hosts,
                         std::u16string* error) {
-  if (!extension->manifest()->FindKey(keys::kHostPermissions))
+  if (!extension->manifest()->FindKey(key))
     return true;
 
   const base::Value* permissions = nullptr;
-  if (!extension->manifest()->GetList(keys::kHostPermissions, &permissions)) {
-    *error = base::UTF8ToUTF16(errors::kInvalidHostPermissions);
+  if (!extension->manifest()->GetList(key, &permissions)) {
+    *error = ErrorUtils::FormatErrorMessageUTF16(
+        errors::kInvalidHostPermissions, key);
     return false;
   }
 
@@ -108,7 +109,7 @@ bool ParseHostsFromJSON(Extension* extension,
       hosts->push_back(list_view[i].GetString());
     } else {
       *error = ErrorUtils::FormatErrorMessageUTF16(
-          errors::kInvalidHostPermission, base::NumberToString(i));
+          errors::kInvalidHostPermission, key, base::NumberToString(i));
       return false;
     }
   }
@@ -359,6 +360,9 @@ void RemoveOverlappingHostPermissions(
     URLPatternSet* optional_host_permissions) {
   URLPatternSet new_optional_host_permissions;
   std::vector<InstallWarning> install_warnings;
+  const char* key = extension->manifest_version() >= 3
+                        ? keys::kOptionalHostPermissions
+                        : keys::kOptionalPermissions;
 
   for (const URLPattern& host_permission : *optional_host_permissions) {
     if (required_host_permissions.ContainsPattern(host_permission)) {
@@ -369,7 +373,7 @@ void RemoveOverlappingHostPermissions(
           ErrorUtils::FormatErrorMessage(
               manifest_errors::kPermissionMarkedOptionalAndRequired,
               host_permission.GetAsString()),
-          keys::kOptionalPermissions);
+          key);
     } else {
       new_optional_host_permissions.AddPattern(host_permission);
     }
@@ -406,24 +410,35 @@ bool PermissionsParser::Parse(Extension* extension, std::u16string* error) {
     return false;
   }
 
+  initial_optional_permissions_ = std::make_unique<InitialPermissions>();
+  if (!ParseHelper(extension, keys::kOptionalPermissions,
+                   &initial_optional_permissions_->api_permissions,
+                   &initial_optional_permissions_->host_permissions, error)) {
+    return false;
+  }
+
   if (extension->manifest_version() >= 3) {
     std::vector<std::string> manifest_hosts;
-    if (!ParseHostsFromJSON(extension, &manifest_hosts, error))
+    std::vector<std::string> manifest_optional_hosts;
+    if (!ParseHostsFromJSON(extension, keys::kHostPermissions, &manifest_hosts,
+                            error)) {
       return false;
+    }
+
+    if (!ParseHostsFromJSON(extension, keys::kOptionalHostPermissions,
+                            &manifest_optional_hosts, error)) {
+      return false;
+    }
 
     // TODO(kelvinjiang): Remove the dependency for |api_permissions| here.
     ParseHostPermissions(extension, keys::kHostPermissions, manifest_hosts,
                          initial_required_permissions_->api_permissions,
                          &initial_required_permissions_->host_permissions);
-  }
 
-  initial_optional_permissions_ = std::make_unique<InitialPermissions>();
-  if (!ParseHelper(extension,
-                   keys::kOptionalPermissions,
-                   &initial_optional_permissions_->api_permissions,
-                   &initial_optional_permissions_->host_permissions,
-                   error)) {
-    return false;
+    ParseHostPermissions(extension, keys::kOptionalHostPermissions,
+                         manifest_optional_hosts,
+                         initial_optional_permissions_->api_permissions,
+                         &initial_optional_permissions_->host_permissions);
   }
 
   // Remove and add install warnings for specified optional API permissions