Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697853002:
Content-Security-Policy: Backporting test of hostmatches in CSPSource. (Closed)
|
Created:
3 years, 10 months ago by arthursonzogni Modified:
3 years, 10 months ago Reviewers:
Mike West CC:
Mike West, alexmos, blink-reviews, chromium-reviews, clamy, nasko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionContent-Security-Policy: Backporting test of hostmatches in CSPSource.
Add tests about CSPSource::hostMatches(...) taken from
https://codereview.chromium.org/2612793002/
BUG=692505
Review-Url: https://codereview.chromium.org/2697853002
Cr-Commit-Position: refs/heads/master@{#450704}
Committed: https://chromium.googlesource.com/chromium/src/+/a87a279a7fd622ca4130806c22e6cf2c9b3bc32e
Patch Set 1 #
Total comments: 9
Patch Set 2 : Add BUG id. #Patch Set 3 : Rebase. #Messages
Total messages: 16 (9 generated)
arthursonzogni@chromium.org changed reviewers: + mkwst@chromium.org
Hi Mike, Again, it is a backport of the tests in: https://codereview.chromium.org/2612793002/ Some of the expectations look strange to you on the browser-side, the results are the same on the renderer-side. Could you take a look and tell me if we have to fill a bug request or to decide it is the correct behavior. https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:185: EXPECT_TRUE(source.matches(KURL(base, "http://*.foo.bar"))); It is strange to me(A wildcard in the host name), but why not. https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:188: EXPECT_TRUE(source.matches(KURL(base, "http://.foo.bar"))); You said it looks strange to you. What do you think should be done? https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:198: EXPECT_FALSE(source.matches(KURL(base, "http://.foo.bar"))); Same here.
LGTM % filing a bug. https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:185: EXPECT_TRUE(source.matches(KURL(base, "http://*.foo.bar"))); On 2017/02/14 at 14:16:30, arthursonzogni wrote: > It is strange to me(A wildcard in the host name), but why not. This should match. It gets canonicalized to something like `%2A.foo.bar`, which matches `foo.bar` with a host wildcard. https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:188: EXPECT_TRUE(source.matches(KURL(base, "http://.foo.bar"))); On 2017/02/14 at 14:16:30, arthursonzogni wrote: > You said it looks strange to you. What do you think should be done? I did say that this looks strange. Please file a bug... I don't think it's a big deal, as we apparently transparently rewrite `.example.com` to `example.com` for navigations, but it's strange regardless. https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:198: EXPECT_FALSE(source.matches(KURL(base, "http://.foo.bar"))); On 2017/02/14 at 14:16:30, arthursonzogni wrote: > Same here. Ditto.
Description was changed from ========== Content-Security-Policy: Backporting test of hostmatches in CSPSource. Add tests about CSPSource::hostMatches(...) taken from https://codereview.chromium.org/2612793002/ BUG=ToBeDefined. ========== to ========== Content-Security-Policy: Backporting test of hostmatches in CSPSource. Add tests about CSPSource::hostMatches(...) taken from https://codereview.chromium.org/2612793002/ BUG=692505 ==========
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2697853002/#ps20001 (title: "Add BUG id.")
Thanks! https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:185: EXPECT_TRUE(source.matches(KURL(base, "http://*.foo.bar"))); On 2017/02/15 06:43:49, Mike West (sloooooow) wrote: > On 2017/02/14 at 14:16:30, arthursonzogni wrote: > > It is strange to me(A wildcard in the host name), but why not. > > This should match. It gets canonicalized to something like `%2A.foo.bar`, which > matches `foo.bar` with a host wildcard. Acknowledged. https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:188: EXPECT_TRUE(source.matches(KURL(base, "http://.foo.bar"))); On 2017/02/15 06:43:49, Mike West (sloooooow) wrote: > On 2017/02/14 at 14:16:30, arthursonzogni wrote: > > You said it looks strange to you. What do you think should be done? > > I did say that this looks strange. Please file a bug... I don't think it's a big > deal, as we apparently transparently rewrite `.example.com` to `example.com` for > navigations, but it's strange regardless. Done. BUG=692505 https://codereview.chromium.org/2697853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:198: EXPECT_FALSE(source.matches(KURL(base, "http://.foo.bar"))); On 2017/02/15 06:43:49, Mike West (sloooooow) wrote: > On 2017/02/14 at 14:16:30, arthursonzogni wrote: > > Same here. > > Ditto. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:161
error: third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp: patch does
not apply
Patch: third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp
Index: third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp
b/third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp
index
5fb8f9373c800ba405f2d7b395dc76b2dd11f347..54d1934d77dd0ef2c94cc98073f9a92e686db2f5
100644
--- a/third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp
@@ -161,6 +161,46 @@ TEST_F(CSPSourceTest,
InsecureHostSchemePortMatchesSecurePort) {
EXPECT_FALSE(source.matches(KURL(base, "https://not-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://*")
+ {
+ CSPSource source(csp.get(), "http", "", 0, "", CSPSource::HasWildcard,
+ CSPSource::NoWildcard);
+ EXPECT_TRUE(source.matches(KURL(base, "http://a.com")));
+ EXPECT_TRUE(source.matches(KURL(base, "http://.")));
+ }
+
+ // Host is *.foo.bar
+ {
+ CSPSource source(csp.get(), "", "foo.bar", 0, "", CSPSource::HasWildcard,
+ CSPSource::NoWildcard);
+ EXPECT_FALSE(source.matches(KURL(base, "http://a.com")));
+ EXPECT_FALSE(source.matches(KURL(base, "http://bar")));
+ EXPECT_FALSE(source.matches(KURL(base, "http://foo.bar")));
+ EXPECT_FALSE(source.matches(KURL(base, "http://o.bar")));
+ EXPECT_TRUE(source.matches(KURL(base, "http://*.foo.bar")));
+ EXPECT_TRUE(source.matches(KURL(base, "http://sub.foo.bar")));
+ EXPECT_TRUE(source.matches(KURL(base, "http://sub.sub.foo.bar")));
+ // Please see http://crbug.com/692505
+ EXPECT_TRUE(source.matches(KURL(base, "http://.foo.bar")));
+ }
+
+ // Host is exact.
+ {
+ CSPSource source(csp.get(), "", "foo.bar", 0, "", CSPSource::NoWildcard,
+ CSPSource::NoWildcard);
+ EXPECT_TRUE(source.matches(KURL(base, "http://foo.bar")));
+ EXPECT_FALSE(source.matches(KURL(base, "http://sub.foo.bar")));
+ EXPECT_FALSE(source.matches(KURL(base, "http://bar")));
+ // Please see http://crbug.com/692505
+ EXPECT_FALSE(source.matches(KURL(base, "http://.foo.bar")));
+ }
+}
+
TEST_F(CSPSourceTest, DoesNotSubsume) {
struct Source {
const char* scheme;
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2697853002/#ps40001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487167975153970,
"parent_rev": "ffe249a3f068cd11200540f93d68f54488402cd5", "commit_rev":
"a87a279a7fd622ca4130806c22e6cf2c9b3bc32e"}
Message was sent while issue was closed.
Description was changed from ========== Content-Security-Policy: Backporting test of hostmatches in CSPSource. Add tests about CSPSource::hostMatches(...) taken from https://codereview.chromium.org/2612793002/ BUG=692505 ========== to ========== Content-Security-Policy: Backporting test of hostmatches in CSPSource. Add tests about CSPSource::hostMatches(...) taken from https://codereview.chromium.org/2612793002/ BUG=692505 Review-Url: https://codereview.chromium.org/2697853002 Cr-Commit-Position: refs/heads/master@{#450704} Committed: https://chromium.googlesource.com/chromium/src/+/a87a279a7fd622ca4130806c22e6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a87a279a7fd622ca4130806c22e6... |
