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

content/common/content_security_policy/csp_source_unittest.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/content_security_policy/csp_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
 namespace {
@@ skipping 28 matching lines @@
 }
 
 TEST(CSPSourceTest, AllowScheme) {
   CSPContext context;
 
   // http -> {http, https}.
   {
     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));
     EXPECT_FALSE(Allow(source, GURL("wss://a.com"), &context));
   }
 
   // ws -> {ws, wss}.
   {
     CSPSource source("ws", "", false, url::PORT_UNSPECIFIED, false, "");
     EXPECT_FALSE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_FALSE(Allow(source, GURL("https://a.com"), &context));
@@ skipping 34 matching lines @@
     EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("http-so://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("https-so://a.com"), &context));
     EXPECT_FALSE(Allow(source, GURL("ftp://a.com"), &context));
 
     // Self's is https.
     context.SetSelf(url::Origin(GURL("https://a.com")));
     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.
     context.SetSelf(url::Origin(GURL("ftp://a.com/")));
     EXPECT_FALSE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("ftp://a.com"), &context));
 
     // Self's scheme is unique.
     context.SetSelf(url::Origin(GURL("non-standard-scheme://a.com")));
     // TODO(mkwst, arthursonzogni): This result might be wrong.
@@ skipping 59 matching lines @@
     EXPECT_TRUE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context));
   }
 
   // Source's port is "*".
   {
     CSPSource source("", "a.com", false, url::PORT_UNSPECIFIED, true, "");
     EXPECT_TRUE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_TRUE(Allow(source, GURL("http://a.com:80"), &context));
     EXPECT_TRUE(Allow(source, GURL("http://a.com:8080"), &context));
     EXPECT_TRUE(Allow(source, GURL("https://a.com:8080"), &context));

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

What about this test expectation?

Source's scheme is empty, so the scheme should be the same as the scheme of
'self' (e.g http).
(https, 8080) is not an upgrade from (http, 80), so it mustn't be allowed.

The example I made have the opposite test expectation.

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

This is a wildcard port though so it should pass.

[arthursonzogni, 2017/04/06 15:05:00]

I think it should no pass for the reasons I have explained above, even if there
is a wildcard port. (https, 8080) is not an upgrade from (http, 80)  and (https,
8080) doesn't matches "a.com:*" because 'self' scheme is http.

     EXPECT_TRUE(Allow(source, GURL("https://a.com:0"), &context));
     EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context));
   }
 
   // Source has a port.
   {
     CSPSource source("", "a.com", false, 80, false, "");
     EXPECT_TRUE(Allow(source, GURL("http://a.com:80"), &context));
     EXPECT_TRUE(Allow(source, GURL("http://a.com"), &context));
     EXPECT_FALSE(Allow(source, GURL("http://a.com:8080"), &context));
     EXPECT_TRUE(Allow(source, GURL("https://a.com"), &context));
   }
 
   // Allow upgrade from :80 to :443
   {
     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
   {
     CSPSource source("http", "", true, 111, false, "");
     EXPECT_TRUE(Allow(source, GURL("http://a.com:111"), &context));
     EXPECT_FALSE(Allow(source, GURL("http://a.com:222"), &context));
   }
 }
 
@@ skipping 58 matching lines @@
     EXPECT_TRUE(Allow(source, GURL("http://a.com/allowed-path"), &context));
     EXPECT_FALSE(Allow(source, GURL("http://a.com/disallowed-path"), &context));
   }
 }
 
 TEST(CSPSourceTest, RedirectMatching) {
   CSPContext context;
   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));
 }
 
 TEST(CSPSourceTest, ToString) {
   {
     CSPSource source("http", "", false, url::PORT_UNSPECIFIED, false, "");
     EXPECT_EQ("http:", source.ToString());
   }
@@ skipping 20 matching lines @@
   {
     CSPSource source("", "a.com", false, url::PORT_UNSPECIFIED, true, "");
     EXPECT_EQ("a.com:*", source.ToString());
   }
   {
     CSPSource source("", "a.com", false, url::PORT_UNSPECIFIED, false, "/path");
     EXPECT_EQ("a.com/path", source.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