Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519103005:
Part 3.2: Is policy list subsumed under subsuming policy? (Closed)
DescriptionPart 3.2: Is policy list subsumed under subsuming policy?
This is part of an experimental feature Embedding-CSP.
Here we add support for `self` source, which gets compiled
to CSPSources for normalization (finding intersection) and
subsumption.
For example, if the secure origin is "https://example.test/"
then for a list of policies:
`
Content-Security-Policy: script-src 'self'
Content-Security-Policy: script-src https://example.test/
`
an intersection would be:
`Content-Security-Policy: script-src https://example.test/`
BUG=647588
Committed: https://crrev.com/685a394fd8ebd2dc4b14ef7277ce80f9776a770f
Cr-Commit-Position: refs/heads/master@{#435883}
Patch Set 1 #Patch Set 2 : Adding more tests #
Total comments: 4
Patch Set 3 : Adding explicit self #Patch Set 4 : Adding back secureOrigin setup #Patch Set 5 : Changing meaning of 'self' #
Total comments: 2
Patch Set 6 : Debugging #Patch Set 7 : Debugging #Patch Set 8 : Adding a failing test to get scoped_tracing info on all of cases #Patch Set 9 : Splitting the tests #
Total comments: 4
Patch Set 10 : Debugging #Patch Set 11 : Removing debugging traces #
Dependent Patchsets: Messages
Total messages: 45 (31 generated)
amalika@google.com changed reviewers: + jochen@chromium.org, mkwst@chromium.org
Here we now also consider `self` sources which we compile to be CSPSources, which are easier to normalize.
Description was changed from ========== Part 3.2: Is policy list subsumed under subsuming policy? This patch adds support for `self` source. BUG=647588 ========== to ========== Part 3.2: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. Here we add support for `self` source, which get compiled to CSPSources for normalization (finding intersection) and subsumption. For example, if the secure origin is "https://example.test/" then for a list of policies: ` Content-Security-Policy: script-src 'self' Content-Security-Policy: script-src https://example.test/ ` an intersection would be: `Content-Security-Policy: script-src https://example.test/` BUG=647588 ==========
Looking good. One question about matching, otherwise straightforward. https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:407: // `self` of A and B match. Might be worth noting somewhere here that `'self'` will be "https://example.test/". https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:411: {{"'self'", "'self'", "https://*.example.test/"}, `*.example.text` doesn't match `example.test`, does it?
Description was changed from ========== Part 3.2: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. Here we add support for `self` source, which get compiled to CSPSources for normalization (finding intersection) and subsumption. For example, if the secure origin is "https://example.test/" then for a list of policies: ` Content-Security-Policy: script-src 'self' Content-Security-Policy: script-src https://example.test/ ` an intersection would be: `Content-Security-Policy: script-src https://example.test/` BUG=647588 ========== to ========== Part 3.2: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. Here we add support for `self` source, which gets compiled to CSPSources for normalization (finding intersection) and subsumption. For example, if the secure origin is "https://example.test/" then for a list of policies: ` Content-Security-Policy: script-src 'self' Content-Security-Policy: script-src https://example.test/ ` an intersection would be: `Content-Security-Policy: script-src https://example.test/` BUG=647588 ==========
https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:407: // `self` of A and B match. On 2016/11/23 at 11:22:02, Mike West (slow) wrote: > Might be worth noting somewhere here that `'self'` will be "https://example.test/". Added! https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:411: {{"'self'", "'self'", "https://*.example.test/"}, On 2016/11/23 at 11:22:02, Mike West (slow) wrote: > `*.example.text` doesn't match `example.test`, does it? It is a vector of policies and since `self` is `example.test` w/o a wildcard, normalized policy will be w/o a wildcard either even though one of the policies has a wildcard.
On 2016/11/23 at 14:09:53, amalika wrote: > https://codereview.chromium.org/2519103005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:411: {{"'self'", "'self'", "https://*.example.test/"}, > On 2016/11/23 at 11:22:02, Mike West (slow) wrote: > > `*.example.text` doesn't match `example.test`, does it? > > It is a vector of policies and since `self` is `example.test` w/o a wildcard, normalized policy will be w/o a wildcard either even though one of the policies has a wildcard. I see, thank you. LGTM!
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by amalika@google.com
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/2519103005/#ps80001 (title: "Rebasing")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #4 (id:100001) has been deleted
Patchset #6 (id:160001) has been deleted
As per our discussion, 'self' specified in Embedding-CSP refers to the origin of the embedded iframe. + Changed tests accordingly. https://codereview.chromium.org/2519103005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2519103005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:590: normalizedA.append(other[0]->m_policy->getSelfSource()); This way, we do not even have to set 'self' on Embedding-CSP.
Still LGTM. Thanks for making the change!
https://codereview.chromium.org/2519103005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2519103005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:551: ContentSecurityPolicy* cspB = SetUpWithOrigin(test.originB); It will be easier to debug the failing bot if you add a `SCOPED_TRACE` that tells you which item is failing. Right now, you've got basically nothing to go on. :(
Patchset #7 (id:200001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #7 (id:240001) has been deleted
Patchset #10 (id:320001) has been deleted
It worries me a bit that you weren't able to track down a root cause. That said, if this passes the bots, land it to unblock the remaining patches. I'd suggest sending an email to oilpan-reviews@ to help with a general review of what we're passing around; I can't help but suspecting things being retained/reused that shouldn't be, which makes me suspicious of things like `Member`. https://codereview.chromium.org/2519103005/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2519103005/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:586: // false}, These tests are commented out; do they pass? https://codereview.chromium.org/2519103005/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:600: SCOPED_TRACE(testing::Message() << "--------------------------------------------------\n"); This (and above) seems cleaner as one SCOPED_TRACE rather than 4.
The CQ bit was checked by amalika@google.com
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/2519103005/#ps360001 (title: "Splitting the tests")
The CQ bit was unchecked by amalika@google.com
Actually, the other patches don't really need these `self` changes so I guess I could rebase other ones and commit those. I will try to debug this one more & email oilpan-reviews@ as you suggested. Thanks! https://codereview.chromium.org/2519103005/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2519103005/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:586: // false}, On 2016/12/01 at 13:06:15, Mike West (slow) wrote: > These tests are commented out; do they pass? They were passing before. But I commented them out because they all have different origins. I will put them back now that we know the above work! https://codereview.chromium.org/2519103005/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:600: SCOPED_TRACE(testing::Message() << "--------------------------------------------------\n"); On 2016/12/01 at 13:06:15, Mike West (slow) wrote: > This (and above) seems cleaner as one SCOPED_TRACE rather than 4. I will remove these!
Patchset #10 (id:340001) has been deleted
Patchset #9 (id:300001) has been deleted
Patchset #10 (id:380001) has been deleted
Patchset #10 (id:400001) has been deleted
The CQ bit was checked by amalika@google.com to run a CQ dry run
The CQ bit was unchecked by amalika@google.com
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amalika@google.com
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/2519103005/#ps440001 (title: "Removing debugging traces")
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": 440001, "attempt_start_ts": 1480666623731890,
"parent_rev": "b9c47c58895c47721bf5a334026560a4dd493645", "commit_rev":
"46c691fbf6eeb335d5e4cca6f97e81c99fc8033c"}
Message was sent while issue was closed.
Committed patchset #11 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Part 3.2: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. Here we add support for `self` source, which gets compiled to CSPSources for normalization (finding intersection) and subsumption. For example, if the secure origin is "https://example.test/" then for a list of policies: ` Content-Security-Policy: script-src 'self' Content-Security-Policy: script-src https://example.test/ ` an intersection would be: `Content-Security-Policy: script-src https://example.test/` BUG=647588 ========== to ========== Part 3.2: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. Here we add support for `self` source, which gets compiled to CSPSources for normalization (finding intersection) and subsumption. For example, if the secure origin is "https://example.test/" then for a list of policies: ` Content-Security-Policy: script-src 'self' Content-Security-Policy: script-src https://example.test/ ` an intersection would be: `Content-Security-Policy: script-src https://example.test/` BUG=647588 Committed: https://crrev.com/685a394fd8ebd2dc4b14ef7277ce80f9776a770f Cr-Commit-Position: refs/heads/master@{#435883} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/685a394fd8ebd2dc4b14ef7277ce80f9776a770f Cr-Commit-Position: refs/heads/master@{#435883} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
