Part 4.1: Is policy list subsumed under subsuming policy?

Part 4.1:  Is policy list subsumed under subsuming policy?

This CL is part of an experimental feature Embedding-CSP. In particular,
we add support for CSP level and layout tests. Main points:
- Filter returned CSP by `enforce` type
- Add check for subsumption in DocumentLoader
- Set `self` for Embedding-CSP.

BUG=647588
Committed: https://crrev.com/761fe2ef2f851eb9980dd1064fef65d42ef7628b
Cr-Commit-Position: refs/heads/master@{#435927}

[amalika, 2016-11-28 11:56:21]

amalika@google.com changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[amalika, 2016-11-28 11:56:22]

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1569:
DCHECK(m_policies.size(), 1u);
Since Embedding-CSP can't be more than just one policy, should this be removed?

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:914:
TEST_F(ContentSecurityPolicyTest, Subsumes) {
Since we test subsumption on other levels + layout tests,
this test mainly checks the ContentSecurityPolicyHeaderType and multiplicity of
the returned CSP.

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:473:
KURL(ParsedURLString, parentSecurityOrigin->toString()));
Not sure if there should be other conversions applied on the string. In the
comments of KURL it mentions something about `KURL::string()`.

[Mike West, 2016-11-28 13:08:02]

Looks pretty good. Just minor questions.

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1569:
DCHECK(m_policies.size(), 1u);
On 2016/11/28 at 11:56:22, amalika wrote:
> Since Embedding-CSP can't be more than just one policy, should this be
removed?

I'd suggest returning `false` if more than one policy is present.

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1571: return
m_policies[0]->subsumes(otherVector);
What if `m_policies[0]` is report-only?

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:369: // This
method checks if this CSP subsumes given CSP.
Nit: "... if this policy subsumes a given policy"

Should probably also add a comment that it only works if the policy contains
only a single `CSPDirectiveList`.

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:473:
KURL(ParsedURLString, parentSecurityOrigin->toString()));
On 2016/11/28 at 11:56:22, amalika wrote:
> Not sure if there should be other conversions applied on the string. In the
comments of KURL it mentions something about `KURL::string()`.

What should this do in a sandboxed document (where the origin serializes to
`null`)?

[amalika, 2016-11-29 12:43:52]

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1569:
DCHECK(m_policies.size(), 1u);
On 2016/11/28 at 13:08:02, Mike West (slow) wrote:
> On 2016/11/28 at 11:56:22, amalika wrote:
> > Since Embedding-CSP can't be more than just one policy, should this be
removed?
> 
> I'd suggest returning `false` if more than one policy is present.

Changed!

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1571: return
m_policies[0]->subsumes(otherVector);
On 2016/11/28 at 13:08:02, Mike West (slow) wrote:
> What if `m_policies[0]` is report-only?

Current implementation of Embedding-CSP is such that there is only one string
which becomes one enforced policy. We set it to be of type `Enforce` in
DocumentLoader.
Would there be any other usages of subsumption that we should check `Report`
type here?

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:473:
KURL(ParsedURLString, parentSecurityOrigin->toString()));
On 2016/11/28 at 13:08:02, Mike West (slow) wrote:
> On 2016/11/28 at 11:56:22, amalika wrote:
> > Not sure if there should be other conversions applied on the string. In the
comments of KURL it mentions something about `KURL::string()`.
> 
> What should this do in a sandboxed document (where the origin serializes to
`null`)?

Oh... hmm. Is there any other way we can learn the origin't kurl? Otherwise,
should we require that Embedding-CSP does not contain `self`?

[Mike West, 2016-11-30 10:20:47]

On 2016/11/29 at 12:43:52, amalika wrote:
>
https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
(right):
> 
>
https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1569:
DCHECK(m_policies.size(), 1u);
> On 2016/11/28 at 13:08:02, Mike West (slow) wrote:
> > On 2016/11/28 at 11:56:22, amalika wrote:
> > > Since Embedding-CSP can't be more than just one policy, should this be
removed?
> > 
> > I'd suggest returning `false` if more than one policy is present.
> 
> Changed!
> 
>
https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1571:
return m_policies[0]->subsumes(otherVector);
> On 2016/11/28 at 13:08:02, Mike West (slow) wrote:
> > What if `m_policies[0]` is report-only?
> 
> Current implementation of Embedding-CSP is such that there is only one string
which becomes one enforced policy. We set it to be of type `Enforce` in
DocumentLoader.
> Would there be any other usages of subsumption that we should check `Report`
type here?
> 
>
https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/2526473005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:473:
KURL(ParsedURLString, parentSecurityOrigin->toString()));
> On 2016/11/28 at 13:08:02, Mike West (slow) wrote:
> > On 2016/11/28 at 11:56:22, amalika wrote:
> > > Not sure if there should be other conversions applied on the string. In
the comments of KURL it mentions something about `KURL::string()`.
> > 
> > What should this do in a sandboxed document (where the origin serializes to
`null`)?
> 
> Oh... hmm. Is there any other way we can learn the origin't kurl? Otherwise,
should we require that Embedding-CSP does not contain `self`?

We just talked about this, but I think we're doing the wrong thing for `'self'`
in general. :)

[amalika, 2016-11-30 12:23:22]

Patchset #4 (id:60001) has been deleted

[amalika, 2016-11-30 12:24:01]

Making changes to address meaning of `self`.
(only changes in the LayoutTests for `self`)

[Mike West, 2016-11-30 21:58:09]

LGTM. :)

[amalika, 2016-12-02 10:45:16]

The CQ bit was checked by amalika@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 10:45:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 13:06:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 13:06:02]

Dry run: This issue passed the CQ dry run.

[amalika, 2016-12-02 13:17:12]

The CQ bit was checked by amalika@google.com

[amalika, 2016-12-02 13:17:13]

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/2526473005/#ps100001
(title: "Rebasing on master")

[commit-bot: I haz the power, 2016-12-02 13:17:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 13:21:29]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480684632743100,
"parent_rev": "8f9eb026b84da52fc51841bc1806f3c513066cdb", "commit_rev":
"bd3debae4af78df46aa64b2b2d966e2c1e6711db"}

[commit-bot: I haz the power, 2016-12-02 13:22:02]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-02 13:25:52]

Description was changed from

==========
Part 4.1:  Is policy list subsumed under subsuming policy?

This CL is part of an experimental feature Embedding-CSP. In particular,
we add support for CSP level and layout tests. Main points:
- Filter returned CSP by `enforce` type
- Add check for subsumption in DocumentLoader
- Set `self` for Embedding-CSP.

BUG=647588
==========

to

==========
Part 4.1:  Is policy list subsumed under subsuming policy?

This CL is part of an experimental feature Embedding-CSP. In particular,
we add support for CSP level and layout tests. Main points:
- Filter returned CSP by `enforce` type
- Add check for subsumption in DocumentLoader
- Set `self` for Embedding-CSP.

BUG=647588

Committed: https://crrev.com/761fe2ef2f851eb9980dd1064fef65d42ef7628b
Cr-Commit-Position: refs/heads/master@{#435927}
==========

[commit-bot: I haz the power, 2016-12-02 13:25:53]

Patchset 5 (id:??) landed as
https://crrev.com/761fe2ef2f851eb9980dd1064fef65d42ef7628b
Cr-Commit-Position: refs/heads/master@{#435927}