[Extensions] Add an install warning for redundant optional host permissions

Add an install warning for the case when an extension requests optional
host permissions that are already covered by required host permissions.

This is a follow-up to 1265118.

Bug: 859959

Change-Id: Idf639b89016ac72a4df5406d290baa177e83a0e4
Reviewed-on: https://chromium-review.googlesource.com/c/1274054
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600530}

chrome/common/extensions/manifest_tests/permissions_parser_unittest.cc

@@ -15,7 +15,7 @@ using PermissionsParserTest = ChromeManifestTest;
 
 TEST_F(PermissionsParserTest, RemoveOverlappingAPIPermissions) {
   scoped_refptr<Extension> extension(LoadAndExpectWarning(
-      "permissions_parser_overlapping_permissions.json",
+      "permissions_overlapping_api_permissions.json",
       ErrorUtils::FormatErrorMessage(
           manifest_errors::kPermissionMarkedOptionalAndRequired, "tabs")));
 
@@ -34,4 +34,67 @@ TEST_F(PermissionsParserTest, RemoveOverlappingAPIPermissions) {
   EXPECT_THAT(optional_api_names, testing::UnorderedElementsAre("geolocation"));
 }
 
+TEST_F(PermissionsParserTest, RemoveOverlappingHostPermissions) {
+  scoped_refptr<Extension> extension(LoadAndExpectWarning(
+      "permissions_overlapping_host_permissions.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",
+      ErrorUtils::FormatErrorMessage(
+          manifest_errors::kPermissionMarkedOptionalAndRequired,
+          "http://*/*")));
+
+  const URLPatternSet& optional_hosts =
+      PermissionsParser::GetOptionalPermissions(extension.get())
+          .explicit_hosts();
+
+  // Since we specified <all_urls> as a required host permission,
+  // there should be no optional host permissions.
+  EXPECT_THAT(*optional_hosts.ToStringVector(), testing::IsEmpty());
+}
+
+TEST_F(PermissionsParserTest, OptionalHostPermissionsAllURLs) {
+  scoped_refptr<Extension> extension(
+      LoadAndExpectSuccess("permissions_optional_all_urls_permissions.json"));
+
+  const URLPatternSet& required_hosts =
+      PermissionsParser::GetRequiredPermissions(extension.get())
+          .explicit_hosts();
+
+  const URLPatternSet& optional_hosts =
+      PermissionsParser::GetOptionalPermissions(extension.get())
+          .explicit_hosts();
+
+  // This time, required permissions are a subset of optional permissions
+  // so we make sure that permissions remain the same
+  // as what's specified in the manifest.
+  EXPECT_THAT(*required_hosts.ToStringVector(),
+              testing::UnorderedElementsAre("https://*.google.com/*"));
+
+  EXPECT_THAT(*optional_hosts.ToStringVector(),
+              testing::UnorderedElementsAre("*://*/*"));
+}
+
 }  // namespace extensions

chrome/test/data/extensions/manifest_tests/permissions_all_urls_host_permissions.json

+{
+  "name": "All urls required in permissions",
+  "version": "0.0.1.33",
+  "manifest_version": 2,
+  "description": "Extension with <all_urls> defined as a required permission",
+  "permissions": [
+    "<all_urls>"
+  ],
+  "optional_permissions": [
+    "http://*/*"
+  ]
+}

chrome/test/data/extensions/manifest_tests/permissions_optional_all_urls_permissions.json

+{
+  "name": "All urls required in permissions",
+  "version": "0.0.1.33",
+  "manifest_version": 2,
+  "description": "Extension with <all_urls> defined as an optional permission",
+  "permissions": [
+    "https://*.google.com/*"
+  ],
+  "optional_permissions": [
+    "*://*/*"
+  ]
+}

chrome/test/data/extensions/manifest_tests/permissions_parser_overlapping_permissions.json → chrome/test/data/extensions/manifest_tests/permissions_overlapping_api_permissions.json

@@ -2,7 +2,7 @@
   "name": "Tabs both required and optional",
   "version": "0.0.1.33",
   "manifest_version": 2,
-  "description": "Extension with overlapping required and optional permissions",
+  "description": "Extension with overlapping required and optional API permissions",
   "permissions": [
     "tabs",
     "storage"

chrome/test/data/extensions/manifest_tests/permissions_overlapping_host_permissions.json

+{
+  "name": "Redundant extension with optional google maps host permission",
+  "version": "0.0.1.33",
+  "manifest_version": 2,
+  "description": "Extension with overlapping required and optional host permissions",
+  "permissions": [
+    "https://example.com/*",
+    "https://*.google.com/*"
+  ],
+  "optional_permissions": [
+    "https://google.com/maps",
+    "*://chromium.org/*"
+  ]
+}

extensions/common/manifest_constants.cc

@@ -720,8 +720,8 @@ const char kNPAPIPluginsNotSupported[] = "NPAPI plugins are not supported.";
 const char kOneUISurfaceOnly[] =
     "Only one of 'browser_action', 'page_action', and 'app' can be specified.";
 const char kPermissionMarkedOptionalAndRequired[] =
-    "Permission '*' cannot be specified as both required and optional in the "
-    "manifest; this permission will be treated as required.";
+    "Optional permission '*' is redundant with the required permissions;"
+    "this permission will be omitted.";
 const char kPermissionMustBeOptional[] =
     "Permission '*' must be specified in the optional section of the manifest.";
 const char kPermissionNotAllowed[] =

extensions/common/manifest_handlers/permissions_parser.cc

@@ -243,6 +243,67 @@ bool ParseHelper(Extension* extension,
   return true;
 }
 
+void RemoveOverlappingAPIPermissions(
+    Extension* extension,
+    const APIPermissionSet& required_api_permissions,
+    APIPermissionSet* optional_api_permissions) {
+  APIPermissionSet overlapping_api_permissions;
+  APIPermissionSet::Intersection(required_api_permissions,
+                                 *optional_api_permissions,
+                                 &overlapping_api_permissions);
+
+  if (overlapping_api_permissions.empty())
+    return;
+
+  std::vector<InstallWarning> install_warnings;
+  install_warnings.reserve(overlapping_api_permissions.size());
+
+  for (const auto* api_permission : overlapping_api_permissions) {
+    install_warnings.emplace_back(
+        ErrorUtils::FormatErrorMessage(
+            manifest_errors::kPermissionMarkedOptionalAndRequired,
+            api_permission->name()),
+        keys::kOptionalPermissions, api_permission->name());
+  }
+
+  extension->AddInstallWarnings(std::move(install_warnings));
+
+  APIPermissionSet new_optional_api_permissions;
+  APIPermissionSet::Difference(*optional_api_permissions,
+                               required_api_permissions,
+                               &new_optional_api_permissions);
+
+  *optional_api_permissions = new_optional_api_permissions;
+}
+
+void RemoveOverlappingHostPermissions(
+    Extension* extension,
+    const URLPatternSet& required_host_permissions,
+    URLPatternSet* optional_host_permissions) {
+  URLPatternSet new_optional_host_permissions;
+  std::vector<InstallWarning> install_warnings;
+
+  for (const URLPattern& host_permission : *optional_host_permissions) {
+    if (required_host_permissions.ContainsPattern(host_permission)) {
+      // We have detected a URLPattern in the optional hosts permission set that
+      // is a strict subset of at least one URLPattern in the required hosts
+      // permission set so we add an install warning.
+      install_warnings.emplace_back(
+          ErrorUtils::FormatErrorMessage(
+              manifest_errors::kPermissionMarkedOptionalAndRequired,
+              host_permission.GetAsString()),
+          keys::kOptionalPermissions);
+    } else {
+      new_optional_host_permissions.AddPattern(host_permission);
+    }
+  }
+
+  if (!install_warnings.empty())
+    extension->AddInstallWarnings(std::move(install_warnings));
+
+  *optional_host_permissions = new_optional_host_permissions;
+}
+
 }  // namespace
 
 struct PermissionsParser::InitialPermissions {
@@ -280,36 +341,14 @@ bool PermissionsParser::Parse(Extension* extension, base::string16* error) {
   // If permissions are specified as both required and optional
   // add an install warning for each permission and remove them from the
   // optional set while keeping them in the required set.
-  APIPermissionSet overlap_permissions;
-  APIPermissionSet::Intersection(initial_required_permissions_->api_permissions,
-                                 initial_optional_permissions_->api_permissions,
-                                 &overlap_permissions);
-
-  if (!overlap_permissions.empty()) {
-    std::vector<InstallWarning> install_warnings;
-    install_warnings.reserve(overlap_permissions.size());
-
-    for (const auto* api_permission : overlap_permissions) {
-      install_warnings.emplace_back(
-          ErrorUtils::FormatErrorMessage(
-              manifest_errors::kPermissionMarkedOptionalAndRequired,
-              api_permission->name()),
-          keys::kOptionalPermissions, api_permission->name());
-    }
+  RemoveOverlappingAPIPermissions(
+      extension, initial_required_permissions_->api_permissions,
+      &initial_optional_permissions_->api_permissions);
 
-    extension->AddInstallWarnings(std::move(install_warnings));
-
-    APIPermissionSet new_optional_api_permissions;
-    APIPermissionSet::Difference(initial_optional_permissions_->api_permissions,
-                                 initial_required_permissions_->api_permissions,
-                                 &new_optional_api_permissions);
-
-    initial_optional_permissions_->api_permissions =
-        new_optional_api_permissions;
-  }
+  RemoveOverlappingHostPermissions(
+      extension, initial_required_permissions_->host_permissions,
+      &initial_optional_permissions_->host_permissions);
 
-  // TODO(kelvinjiang): Use URLPatternSet to check for cases where a url pattern
-  // in required permissions contains another pattern in optional permissions.
   return true;
 }