Stop CSP from matching independent scheme/port upgrades

third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp

 // Copyright 2015 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 "core/frame/csp/CSPSource.h"
 
 #include "core/dom/Document.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "platform/network/ResourceRequest.h"
 #include "platform/weborigin/KURL.h"
@@ skipping 87 matching lines @@
   KURL base;
   CSPSource source(csp.get(), "http", "example.com", 8000, "/bar/",
                    CSPSource::NoWildcard, CSPSource::NoWildcard);
 
   EXPECT_TRUE(
       source.matches(KURL(base, "http://example.com:8000/"),
                      ResourceRequest::RedirectStatus::FollowedRedirect));
   EXPECT_TRUE(
       source.matches(KURL(base, "http://example.com:8000/foo"),
                      ResourceRequest::RedirectStatus::FollowedRedirect));
-  EXPECT_TRUE(
+  // Should not allow upgrade of port or scheme without upgrading both
+  EXPECT_FALSE(
       source.matches(KURL(base, "https://example.com:8000/foo"),
                      ResourceRequest::RedirectStatus::FollowedRedirect));
-
   EXPECT_FALSE(
       source.matches(KURL(base, "http://not-example.com:8000/foo"),
                      ResourceRequest::RedirectStatus::FollowedRedirect));
   EXPECT_FALSE(source.matches(KURL(base, "http://example.com:9000/foo/"),
                               ResourceRequest::RedirectStatus::NoRedirect));
 }
 
 TEST_F(CSPSourceTest, InsecureSchemeMatchesSecureScheme) {
   KURL base;
   CSPSource source(csp.get(), "http", "", 0, "/", CSPSource::NoWildcard,
@@ skipping 35 matching lines @@
 
   // Self scheme is https.
   {
     Persistent<ContentSecurityPolicy> csp(ContentSecurityPolicy::create());
     csp->setupSelf(*SecurityOrigin::createFromString("https://a.com/"));
     CSPSource source(csp.get(), "", "a.com", 0, "/", CSPSource::NoWildcard,
                      CSPSource::NoWildcard);
     EXPECT_FALSE(source.matches(KURL(base, "http://a.com")));
     EXPECT_TRUE(source.matches(KURL(base, "https://a.com")));
     EXPECT_FALSE(source.matches(KURL(base, "http-so://a.com")));
-    // TODO(mkwst, arthursonzogni): Maybe it should return true.
-    // See http://crbug.com/692442
-    EXPECT_FALSE(source.matches(KURL(base, "https-so://a.com")));
+    EXPECT_TRUE(source.matches(KURL(base, "https-so://a.com")));

[andypaicu, 2017/02/24 08:41:00]

Added 692442 since this will also fix that bug

[Mike West, 2017/02/24 10:56:28]

Hrm. I'm not actually sure this is a bug. :(

This is dumb, but please add something like `// TODO(jochen): Maybe it should
return false?` since he's now responsible for suborigins. Muwahaha.

[andypaicu, 2017/03/13 10:07:20]

I've added the comment and let Jochen know.

     EXPECT_FALSE(source.matches(KURL(base, "ftp://a.com")));
   }
 
   // Self scheme is not in the http familly.
   {
     Persistent<ContentSecurityPolicy> csp(ContentSecurityPolicy::create());
     csp->setupSelf(*SecurityOrigin::createFromString("ftp://a.com/"));
     CSPSource source(csp.get(), "", "a.com", 0, "/", CSPSource::NoWildcard,
                      CSPSource::NoWildcard);
     EXPECT_FALSE(source.matches(KURL(base, "http://a.com")));
@@ skipping 18 matching lines @@
 
 TEST_F(CSPSourceTest, InsecureHostSchemePortMatchesSecurePort) {
   KURL base;
 
   // source scheme is "http"
   {
     CSPSource source(csp.get(), "http", "example.com", 80, "/",
                      CSPSource::NoWildcard, CSPSource::NoWildcard);
     EXPECT_TRUE(source.matches(KURL(base, "http://example.com/")));
     EXPECT_TRUE(source.matches(KURL(base, "http://example.com:80/")));
-    // TODO(mkwst, arthursonzogni): It is weird to upgrade the port without the
-    // sheme. See http://crbug.com/692499
-    EXPECT_TRUE(source.matches(KURL(base, "http://example.com:443/")));
+
+    // Should not allow scheme upgrades unless both port and scheme are upgraded
+    EXPECT_FALSE(source.matches(KURL(base, "http://example.com:443/")));
     EXPECT_TRUE(source.matches(KURL(base, "https://example.com/")));
-    // TODO(mkwst, arthursonzogni): It is weird to upgrade the scheme without
-    // the port. See http://crbug.com/692499
-    EXPECT_TRUE(source.matches(KURL(base, "https://example.com:80/")));
+    EXPECT_FALSE(source.matches(KURL(base, "https://example.com:80/")));
+
     EXPECT_TRUE(source.matches(KURL(base, "https://example.com:443/")));
 
     EXPECT_FALSE(source.matches(KURL(base, "http://example.com:8443/")));
     EXPECT_FALSE(source.matches(KURL(base, "https://example.com:8443/")));
 
     EXPECT_FALSE(source.matches(KURL(base, "http://not-example.com/")));
     EXPECT_FALSE(source.matches(KURL(base, "http://not-example.com:80/")));
     EXPECT_FALSE(source.matches(KURL(base, "http://not-example.com:443/")));
     EXPECT_FALSE(source.matches(KURL(base, "https://not-example.com/")));
     EXPECT_FALSE(source.matches(KURL(base, "https://not-example.com:80/")));
     EXPECT_FALSE(source.matches(KURL(base, "https://not-example.com:443/")));
   }
 
   // source scheme is empty
   {
     Persistent<ContentSecurityPolicy> csp(ContentSecurityPolicy::create());
     csp->setupSelf(*SecurityOrigin::createFromString("http://example.com"));
     CSPSource source(csp.get(), "", "example.com", 80, "/",
                      CSPSource::NoWildcard, CSPSource::NoWildcard);
     EXPECT_TRUE(source.matches(KURL(base, "http://example.com/")));
     EXPECT_TRUE(source.matches(KURL(base, "https://example.com:443")));
-    // TODO(mkwst, arthursonzogni): It is weird to upgrade the port without the
-    // sheme. See http://crbug.com/692499
-    EXPECT_TRUE(source.matches(KURL(base, "http://example.com:443")));
+    // Should not allow upgrade of port or scheme without upgrading both
+    EXPECT_FALSE(source.matches(KURL(base, "http://example.com:443")));
+  }
+
+  // source port is empty
+  {
+    CSPSource source(csp.get(), "http", "example.com", 0, "/",
+                     CSPSource::NoWildcard, CSPSource::NoWildcard);
+
+    EXPECT_TRUE(source.matches(KURL(base, "http://example.com")));
+    EXPECT_TRUE(source.matches(KURL(base, "https://example.com")));
+    EXPECT_TRUE(source.matches(KURL(base, "https://example.com:443")));
+    // Should not allow upgrade of port or scheme without upgrading both
+    EXPECT_FALSE(source.matches(KURL(base, "https://example.com:80")));
+    EXPECT_FALSE(source.matches(KURL(base, "http://example.com:443")));
   }
 }
 
 TEST_F(CSPSourceTest, HostMatches) {
   KURL base;
   Persistent<ContentSecurityPolicy> csp(ContentSecurityPolicy::create());
   csp->setupSelf(*SecurityOrigin::createFromString("http://a.com"));
 
   // Host is * (source-expression = "http://*")
   {
@@ skipping 649 matching lines @@
     normalized = B->intersect(A);
     Source intersectBA = {
         normalized->m_scheme,       normalized->m_host,
         normalized->m_path,         normalized->m_port,
         normalized->m_hostWildcard, normalized->m_portWildcard};
     EXPECT_TRUE(equalSources(intersectBA, test.normalized));
   }
 }
 
 }  // namespace blink