Restrict the externally_connectable manifest key to a single origin

per match pattern, by disallowing wildcard hosts and wildcard subdomains
of TLDs (com) and effective TLDs (appspot.com).

R=yoz@chromium.org
BUG=55316

Review URL: https://chromiumcodereview.appspot.com/15862011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@203009 0039d316-1c4b-4281-b951-d872f2087c98

chrome/common/extensions/api/manifest_types.json

@@ -15,11 +15,13 @@
         "description": "The schema of the <code>externally_connectable</code> manifest property",
         "properties": {
           "matches": {
+            "description": "<p>The URL patterns for web pages that are allowed to connect. If left empty or unspecified, no web pages can connect.</p><p>Patterns cannot include wildcard domains nor subdomains of (effective) top level domains; <code>*://google.com/*</code> and <code>http://*.chromium.org/*</code> are valid, while <code>&lt;all_urls&gt;</code>, <code>http://*/*</code>, <code>*://*.com/*</code>, and even <code>http://*.appspot.com/*</code> are not.</p>",
             "optional": true,
             "type": "array",
             "items": {"type": "string"}
           },
           "ids": {
+            "description": "The IDs of extensions or apps that are allowed to connect. If left empty or unspecified, no extensions nor apps can connect. <code>*</code> will match all extension and app IDs.",
             "optional": true,
             "type": "array",
             "items": {"type": "string"}

chrome/common/extensions/manifest_handlers/externally_connectable.cc

@@ -10,13 +10,21 @@
 #include "extensions/common/error_utils.h"
 #include "extensions/common/url_pattern.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+
+namespace rcd = net::registry_controlled_domains;
 
 namespace extensions {
 
 namespace externally_connectable_errors {
-  const char kErrorInvalid[] = "Invalid value for 'externally_connectable'";
-  const char kErrorInvalidMatchPattern[] = "Invalid match pattern '*'";
-  const char kErrorInvalidId[] = "Invalid ID '*'";
+const char kErrorInvalid[] = "Invalid value for 'externally_connectable'";
+const char kErrorInvalidMatchPattern[] = "Invalid match pattern '*'";
+const char kErrorInvalidId[] = "Invalid ID '*'";
+const char kErrorTopLevelDomainsNotAllowed[] =
+    "\"*\" is an effective top level domain for which wildcard subdomains such "
+    "as \"*\" are not allowed";
+const char kErrorWildcardHostsNotAllowed[] =
+    "Wildcard domain patterns such as \"*\" are not allowed";
 }
 
 namespace keys = extension_manifest_keys;
@@ -80,6 +88,42 @@ scoped_ptr<ExternallyConnectableInfo> ExternallyConnectableInfo::FromValue(
             errors::kErrorInvalidMatchPattern, *it);
         return scoped_ptr<ExternallyConnectableInfo>();
       }
+
+      // Wildcard hosts are not allowed.
+      if (pattern.host().empty()) {
+        *error = ErrorUtils::FormatErrorMessageUTF16(
+            errors::kErrorWildcardHostsNotAllowed, *it);
+        return scoped_ptr<ExternallyConnectableInfo>();
+      }
+
+      // Wildcards on subdomains of a TLD are not allowed.
+      size_t registry_length = rcd::GetRegistryLength(
+          pattern.host(),
+          // This means that things that look like TLDs - the foobar in
+          // http://google.foobar - count as TLDs.
+          rcd::INCLUDE_UNKNOWN_REGISTRIES,
+          // This means that effective TLDs like appspot.com count as TLDs;
+          // codereview.appspot.com and evil.appspot.com are different.
+          rcd::INCLUDE_PRIVATE_REGISTRIES);
+
+      if (registry_length == std::string::npos) {
+        // The URL parsing combined with host().empty() should have caught this.
+        NOTREACHED() << *it;
+        *error = ErrorUtils::FormatErrorMessageUTF16(
+            errors::kErrorInvalidMatchPattern, *it);
+        return scoped_ptr<ExternallyConnectableInfo>();
+      }
+
+      // Broad match patterns like "*.com", "*.co.uk", and even "*.appspot.com"
+      // are not allowed. However just "appspot.com" is ok.
+      if (registry_length == 0 && pattern.match_subdomains()) {
+        *error = ErrorUtils::FormatErrorMessageUTF16(
+            errors::kErrorTopLevelDomainsNotAllowed,
+            pattern.host().c_str(),
+            *it);
+        return scoped_ptr<ExternallyConnectableInfo>();
+      }
+
       matches.AddPattern(pattern);
     }
   }

chrome/common/extensions/manifest_handlers/externally_connectable.h

@@ -23,9 +23,11 @@ namespace extensions {
 
 // Error constants used when parsing the externally_connectable manifest entry.
 namespace externally_connectable_errors {
-  extern const char kErrorInvalid[];
-  extern const char kErrorInvalidMatchPattern[];
-  extern const char kErrorInvalidId[];
+extern const char kErrorInvalid[];
+extern const char kErrorInvalidMatchPattern[];
+extern const char kErrorInvalidId[];
+extern const char kErrorTopLevelDomainsNotAllowed[];
+extern const char kErrorWildcardHostsNotAllowed[];
 }
 
 // Parses the externally_connectable manifest entry.

chrome/common/extensions/manifest_handlers/externally_connectable_unittest.cc

@@ -62,6 +62,25 @@ TEST_F(ExternallyConnectableTest, IDsAndMatches) {
 
   EXPECT_FALSE(info->matches.MatchesURL(GURL("http://yahoo.com")));
   EXPECT_FALSE(info->matches.MatchesURL(GURL("http://yahoo.com/")));
+
+  // TLD-style patterns should match just the TLD.
+  EXPECT_TRUE(info->matches.MatchesURL(GURL("http://appspot.com/foo.html")));
+  EXPECT_TRUE(info->matches.MatchesURL(GURL("http://com")));
+  EXPECT_TRUE(info->matches.MatchesURL(GURL("http://go/here")));
+
+  // TLD-style patterns should *not* match any subdomains of the TLD.
+  EXPECT_FALSE(
+      info->matches.MatchesURL(GURL("http://codereview.appspot.com/foo.html")));
+  EXPECT_FALSE(
+      info->matches.MatchesURL(GURL("http://chromium.com/index.html")));
+  EXPECT_FALSE(info->matches.MatchesURL(GURL("http://here.go/somewhere")));
+
+  // Paths that don't have any wildcards should match the exact domain, but
+  // ignore the trailing slash. This is kind of a corner case, so let's test it.
+  EXPECT_TRUE(info->matches.MatchesURL(GURL("http://no.wildcard.path")));
+  EXPECT_TRUE(info->matches.MatchesURL(GURL("http://no.wildcard.path/")));
+  EXPECT_FALSE(info->matches.MatchesURL(
+        GURL("http://no.wildcard.path/index.html")));
 }
 
 TEST_F(ExternallyConnectableTest, IDs) {
@@ -154,4 +173,55 @@ TEST_F(ExternallyConnectableTest, ErrorBadMatches) {
       EXPECT_TYPE_ERROR);
 }
 
+TEST_F(ExternallyConnectableTest, ErrorNoAllURLs) {
+  RunTestcase(
+      Testcase(
+          "externally_connectable_error_all_urls.json",
+          ErrorUtils::FormatErrorMessage(errors::kErrorWildcardHostsNotAllowed,
+                                         "<all_urls>")),
+      EXPECT_TYPE_ERROR);
+}
+
+TEST_F(ExternallyConnectableTest, ErrorWildcardHost) {
+  RunTestcase(
+      Testcase(
+          "externally_connectable_error_wildcard_host.json",
+          ErrorUtils::FormatErrorMessage(errors::kErrorWildcardHostsNotAllowed,
+                                         "http://*/*")),
+      EXPECT_TYPE_ERROR);
+}
+
+TEST_F(ExternallyConnectableTest, ErrorNoTLD) {
+  RunTestcase(
+      Testcase(
+          "externally_connectable_error_tld.json",
+          ErrorUtils::FormatErrorMessage(
+              errors::kErrorTopLevelDomainsNotAllowed,
+              "co.uk",
+              "http://*.co.uk/*")),
+      EXPECT_TYPE_ERROR);
+}
+
+TEST_F(ExternallyConnectableTest, ErrorNoEffectiveTLD) {
+  RunTestcase(
+      Testcase(
+          "externally_connectable_error_effective_tld.json",
+          ErrorUtils::FormatErrorMessage(
+              errors::kErrorTopLevelDomainsNotAllowed,
+              "appspot.com",
+              "http://*.appspot.com/*")),
+      EXPECT_TYPE_ERROR);
+}
+
+TEST_F(ExternallyConnectableTest, ErrorUnknownTLD) {
+  RunTestcase(
+      Testcase(
+          "externally_connectable_error_unknown_tld.json",
+          ErrorUtils::FormatErrorMessage(
+              errors::kErrorTopLevelDomainsNotAllowed,
+              "notatld",
+              "http://*.notatld/*")),
+      EXPECT_TYPE_ERROR);
+}
+
 }  // namespace extensions

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

+{
+  "name": "unit_tests --gtest_filter=ExternallyConnectableTest.ErrorAllURLs",
+  "version": "1",
+  "manifest_version": 2,
+  "externally_connectable": {
+    "matches": [
+      "*://example.com/*",
+      "<all_urls>",
+      "http://build.chromium.org/*"
+    ]
+  }
+}

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

+{
+  "name": "unit_tests --gtest_filter=ExternallyConnectableTest.ErrorEffectiveTLD",
+  "version": "1",
+  "manifest_version": 2,
+  "externally_connectable": {
+    "matches": [
+      "*://example.com/*",
+      "http://*.appspot.com/*",
+      "http://build.chromium.org/*"
+    ]
+  }
+}
+

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

+{
+  "name": "unit_tests --gtest_filter=ExternallyConnectableTest.ErrorTLD",
+  "version": "1",
+  "manifest_version": 2,
+  "externally_connectable": {
+    "matches": [
+      "*://example.com/*",
+      "http://*.co.uk/*",
+      "http://build.chromium.org/*"
+    ]
+  }
+}

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

+{
+  "name": "unit_tests --gtest_filter=ExternallyConnectableTest.ErrorUnknownTLD",
+  "version": "1",
+  "manifest_version": 2,
+  "externally_connectable": {
+    "matches": [
+      "*://example.com/*",
+      "http://*.notatld/*",
+      "http://build.chromium.org/*"
+    ]
+  }
+}

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

+{
+  "name": "unit_tests --gtest_filter=ExternallyConnectableTest.ErrorWildcardHost",
+  "version": "1",
+  "manifest_version": 2,
+  "externally_connectable": {
+    "matches": [
+      "*://example.com/*",
+      "http://*/*",
+      "http://build.chromium.org/*"
+    ]
+  }
+}

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

@@ -10,7 +10,11 @@
     "matches": [
       "http://example.com/",
       "*://*.google.com/*",
-      "http://build.chromium.org/*"
+      "http://build.chromium.org/*",
+      "http://appspot.com/*",
+      "http://com/*",
+      "http://go/*",
+      "http://no.wildcard.path/"
     ]
   }
 }