Stop CSP from matching independent scheme/port upgrades (content layer)

content/common/content_security_policy/csp_source_unittest.cc

Index: content/common/content_security_policy/csp_source_unittest.cc
diff --git a/content/common/content_security_policy/csp_source_unittest.cc b/content/common/content_security_policy/csp_source_unittest.cc
index 1e8a2a4b82a53310cf7f2ffb890bb4f8f674ad1d..b82b001663ae4df7ad6bce45e6186f6ca703b486 100644
--- a/content/common/content_security_policy/csp_source_unittest.cc
+++ b/content/common/content_security_policy/csp_source_unittest.cc
@@ -46,8 +46,7 @@ TEST(CSPSourceTest, AllowScheme) {
     CSPSource source("http", "", false, url::PORT_UNSPECIFIED, false, "");
     EXPECT_TRUE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context));
-    // TODO(mkwst, arthursonzogni): It is weird to upgrade the scheme without
-    // the port. See http://crbug.com/692499
+    // This passes because the source is "scheme only" so the upgrade is allowed

[arthursonzogni, 2017/04/07 09:20:27]

Nit: a dot is missing at the end of this comment.

[andypaicu, 2017/04/07 11:34:24]

Done

     EXPECT_TRUE(Allow(source, GURL("https://a.com:80"), &context));

[arthursonzogni, 2017/04/05 12:14:55]

The example I made have the opposite test expectation here.
(https, 80) doesn't match the source-expression and it is not upgrade from the
insecure (http, 80). So it isn't allowed.

[andypaicu, 2017/04/06 09:05:51]

Yeah this should be EXPECT_FALSE regardless of the implementation.

[andypaicu, 2017/04/06 09:05:51]

Yeah you're right it should not pass here, regardless of implementation this
should be changed to EXPECT_FALSE

     EXPECT_FALSE(Allow(source, GURL("ftp://a.com"), &context));
     EXPECT_FALSE(Allow(source, GURL("ws://a.com"), &context));
@@ -103,9 +102,8 @@ TEST(CSPSourceTest, AllowScheme) {
     EXPECT_FALSE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context));
     EXPECT_FALSE(Allow(source, GURL("http-so://a.com"), &context));
-    // TODO(mkwst, arthursonzogni): Maybe it should return true.
-    // See http://crbug.com/692442:
-    EXPECT_FALSE(Allow(source, GURL("https-so://a.com"), &context));
+    // TODO(jochen): Maybe it should return false?
+    EXPECT_TRUE(Allow(source, GURL("https-so://a.com"), &context));
     EXPECT_FALSE(Allow(source, GURL("ftp://a.com"), &context));
 
     // Self's scheme is not in the http familly.
@@ -203,9 +201,8 @@ TEST(CSPSourceTest, AllowPort) {
   {
     CSPSource source("", "a.com", false, 80, false, "");
     EXPECT_TRUE(Allow(source, GURL("https://a.com:443"), &context));
-    // TODO(mkwst, arthursonzogni): It is weird to upgrade the port without the
-    // sheme. See http://crbug.com/692499
-    EXPECT_TRUE(Allow(source, GURL("http://a.com:443"), &context));
+    // Should not allow scheme upgrades unless both port and scheme are upgraded

[arthursonzogni, 2017/04/07 09:20:27]

Nit: A dot is missing at the end of this comment.

[andypaicu, 2017/04/07 11:34:24]

Done

+    EXPECT_FALSE(Allow(source, GURL("http://a.com:443"), &context));
   }
 
   // Host is * but port is specified
@@ -284,7 +281,7 @@ TEST(CSPSourceTest, RedirectMatching) {
   CSPSource source("http", "a.com", false, 8000, false, "/bar/");
   EXPECT_TRUE(Allow(source, GURL("http://a.com:8000/"), &context, true));
   EXPECT_TRUE(Allow(source, GURL("http://a.com:8000/foo"), &context, true));
-  EXPECT_TRUE(Allow(source, GURL("https://a.com:8000/foo"), &context, true));
+  EXPECT_FALSE(Allow(source, GURL("https://a.com:8000/foo"), &context, true));
   EXPECT_FALSE(
       Allow(source, GURL("http://not-a.com:8000/foo"), &context, true));
   EXPECT_FALSE(Allow(source, GURL("http://a.com:9000/foo/"), &context, false));
@@ -325,4 +322,17 @@ TEST(CSPSourceTest, ToString) {
   }
 }
 
+TEST(CSPSourceTest, UpgradeRequests) {
+  CSPContext context;
+  CSPSource source("http", "a.com", false, 80, false, "");
+  EXPECT_TRUE(Allow(source, GURL("http://a.com:80"), &context, true));
+  EXPECT_FALSE(Allow(source, GURL("https://a.com:80"), &context, true));
+  EXPECT_FALSE(Allow(source, GURL("http://a.com:443"), &context, true));
+  EXPECT_TRUE(Allow(source, GURL("https://a.com:443"), &context, true));
+

[arthursonzogni, 2017/04/07 09:20:27]

Nit: I think you can probably remove this empty line.

[andypaicu, 2017/04/07 11:34:24]

Done

+  EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context, true));
+}
+
+
+
 }  // namespace content