Implement the `require-sri-for` CSP directive

Implement the `require-sri-for` CSP directive

As defined in [1], this CSP directive allows developers to block resource
requests that do not contain integrity metadata. This includes contexts
like external scripts, workers, shared workers, service workers, external
stylesheets, preload requests, and requests originated by CSS @import.

[1]: https://w3c.github.io/webappsec-subresource-integrity/#opt-in-require-sri-for

Intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jyCdW1dHyYA/YefRSKs1AQAJ

BUG=618924
R=mkwst@chromium.org

Committed: https://crrev.com/615df311feca979548acd4f73ce3d7ac34449e3a
Cr-Commit-Position: refs/heads/master@{#405530}

[Sergey Shekyan, 2016-06-10 07:01:10]

The CQ bit was checked by shekyan@gmail.com

[commit-bot: I haz the power, 2016-06-10 07:01:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056183002/1

[commit-bot: I haz the power, 2016-06-10 07:01:24]

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

[commit-bot: I haz the power, 2016-06-10 07:01:24]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Mike West, 2016-06-10 08:45:47]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-10 08:45:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056183002/1

[commit-bot: I haz the power, 2016-06-10 08:47:40]

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

[commit-bot: I haz the power, 2016-06-10 08:47:41]

Dry run: Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Mike West, 2016-06-10 09:25:13]

mkwst@chromium.org changed reviewers:
+ jww@chromium.org

[Mike West, 2016-06-10 09:25:15]

I added a first set of comments. The tests are a good start, but I've pointed
you at a few that I hope will be better models for you. I'd also like you to add
some tests for reporting, both to verify that the report data is correct, and
also that report-only mode does the right thing.

Adding jww@ who might have comments as well.

Thanks!

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html
(right):

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html:4:
<meta http-equiv="Content-Security-Policy" content="require-sri-for script;
script-src 'unsafe-inline' *">
Please write this as a `testharness.js` test. See
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-enforce-allowed.php`
as an example.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-blocked.html
(right):

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-blocked.html:4:
<meta http-equiv="Content-Security-Policy" content="require-sri-for script;
script-src 'unsafe-inline' *">
Please write this as a `testharness.js` test. See
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-enforce-blocked.php`
as an example.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-style-allowed.html
(right):

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-style-allowed.html:13:
document.write(document.styleSheets.length > 0 ? 'FAIL' : 'PASS');
Please write this as a `testharness.js` test. See
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/style-enforce-allowed.php`
as an example.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-style-blocked.html
(right):

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-style-blocked.html:4:
<meta http-equiv="Content-Security-Policy" content="require-sri-for style;
style-src *;;">
Nit: Extra ';'.

Double nit: Please write this as a `testharness.js` test. See
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/style-enforce-blocked.php`
as an example.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:209:
reportViolation(ContentSecurityPolicy::RequireSRIFor,
ContentSecurityPolicy::RequireSRIFor, "Refused to load the " + resourceType + "
'" + url.elidedString() + "' because 'require-sri-for' directive requires
integrity attribute be present for all " + resourceType + "s.", url,
ResourceRequest::RedirectStatus::NoRedirect);
`NoRedirect` probably isn't correct here, as I think the current code is going
to trigger this on every step through a redirect in `report-only` mode. You'll
either need to refactor the code so that we only do this check on the initial
step of a request, or pass in the redirect status here so that we properly strip
the report data cross-origin.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:929:
parseRequireSRIFor(name, value);
This functionality should be hidden behind the experimental features flag. You
could call `ContentSecurityPolicy::experimentalFeaturesEnabled()` before
parsing, for example.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:93:
FRIEND_TEST_ALL_PREFIXES(CSPDirectiveListTest, IsMatchingNoncePresent);
Please add parsing tests to CSPDirectiveListTest to verify the bits of state
that you're setting here.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:571: if
(!policy->allowRequestWithoutMetadata(context, url, integrityMetedata,
reportingStatus))
I don't understand the naming choices here. Why is is named one thing on
ContentSecurityPolicy and another on CSPDirectiveList? I think it makes sense to
name both `allowRequestWithoutIntegrity`, and only to call in if the
IntegrityMetadataSet is empty. See the comments in `::allowRequest`.

WDYT?

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:577: bool
ContentSecurityPolicy::allowRequest(WebURLRequest::RequestContext context, const
KURL& url, const String& nonce, const IntegrityMetadataSet& integrityMetedata,
RedirectStatus redirectStatus, ReportingStatus reportingStatus) const
Nit: s/Metedata/Metadata/

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:579: switch
(context) {
Rather than calling `checkIntegrityMetadataPresence` inline with the call to
`allowWhateverFromSource`, it might make things clearer to do something like the
following right at the top:

```
if (integrityMetadata.isEmpty() && !allowRequestWithoutIntegrity(context, url,
redirectStatus, reportingStatus))
    return false;
```

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:606: &&
allowScriptFromSource(url, nonce, redirectStatus, reportingStatus);
Please add tests for the kinds of script creation that SRI doesn't yet support.
I'm thinking specifically of https://twitter.com/Qab/status/741077446179618816
and imports, but please feel free to be creative. :)

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:614: return
allowWorkerContextFromSource(url, redirectStatus, reportingStatus);
Here too, along with layout tests for worker creation.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:211: bool
checkIntegrityMetadataPresence(WebURLRequest::RequestContext, const KURL&, const
IntegrityMetadataSet&, ReportingStatus = SendReport) const;
Nit: This should probably be private.

[jww, 2016-06-11 22:45:12]

Thanks for starting this, Sergey! I've added a few comments on top of Mike's.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html
(right):

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html:4:
<meta http-equiv="Content-Security-Policy" content="require-sri-for script;
script-src 'unsafe-inline' *">
On 2016/06/10 09:25:15, Mike West wrote:
> Please write this as a `testharness.js` test. See
>
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-enforce-allowed.php`
> as an example.

Just as an explanation, this is how we used to write tests, but testharness is
the new and improved testing framework (and more easily made compatible with the
cross-user agent Web Platform Tests).

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:205: if (context
== WebURLRequest::RequestContextScript || context ==
WebURLRequest::RequestContextImport)
Treating RequestContextImport as guaranteed to be a script resource needs an
explanatory comment, I think. Is the reasoning just that since integrity is only
enforced for scripts and stylesheets that any import leading to this code must
be a script import, and not an HTML import or something?

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:650: if
(!invalidTokensErrorMessage.isEmpty()) {
I think inavildTokensErrorMessage cannot be empty here. I'm happy for this to be
a DCHECK that it's not empty, but I don't think it should be a conditional.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:69: bool
allowRequestWithoutMetadata(WebURLRequest::RequestContext, const KURL&, const
IntegrityMetadataSet&, ContentSecurityPolicy::ReportingStatus) const;
nit: I'd prefer that this have 'Integrity' somewhere in the method name since I
expect that 'metadata' may be used elsewhere in CSP at a later point. So perhaps
named "allowRequestWithoutIntegrityMetadata"?

[Sergey Shekyan, 2016-06-11 23:32:28]

On 2016/06/11 22:45:12, jww wrote:
> Thanks for starting this, Sergey! I've added a few comments on top of Mike's.
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html
> (right):
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html:4:
> <meta http-equiv="Content-Security-Policy" content="require-sri-for script;
> script-src 'unsafe-inline' *">
> On 2016/06/10 09:25:15, Mike West wrote:
> > Please write this as a `testharness.js` test. See
> >
>
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-enforce-allowed.php`
> > as an example.
> 
> Just as an explanation, this is how we used to write tests, but testharness is
> the new and improved testing framework (and more easily made compatible with
the
> cross-user agent Web Platform Tests).
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:205: if (context
> == WebURLRequest::RequestContextScript || context ==
> WebURLRequest::RequestContextImport)
> Treating RequestContextImport as guaranteed to be a script resource needs an
> explanatory comment, I think. Is the reasoning just that since integrity is
only
> enforced for scripts and stylesheets that any import leading to this code must
> be a script import, and not an HTML import or something?
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:650: if
> (!invalidTokensErrorMessage.isEmpty()) {
> I think inavildTokensErrorMessage cannot be empty here. I'm happy for this to
be
> a DCHECK that it's not empty, but I don't think it should be a conditional.
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):
> 
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:69: bool
> allowRequestWithoutMetadata(WebURLRequest::RequestContext, const KURL&, const
> IntegrityMetadataSet&, ContentSecurityPolicy::ReportingStatus) const;
> nit: I'd prefer that this have 'Integrity' somewhere in the method name since
I
> expect that 'metadata' may be used elsewhere in CSP at a later point. So
perhaps
> named "allowRequestWithoutIntegrityMetadata"?

Thanks! Agree for all the comments. All address all of then on 18th, when i
return from vacation.

[jww, 2016-06-12 16:38:22]

On 2016/06/11 23:32:28, shekyan wrote:
> On 2016/06/11 22:45:12, jww wrote:
> > Thanks for starting this, Sergey! I've added a few comments on top of
Mike's.
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
> > File
> >
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html:4:
> > <meta http-equiv="Content-Security-Policy" content="require-sri-for script;
> > script-src 'unsafe-inline' *">
> > On 2016/06/10 09:25:15, Mike West wrote:
> > > Please write this as a `testharness.js` test. See
> > >
> >
>
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-enforce-allowed.php`
> > > as an example.
> > 
> > Just as an explanation, this is how we used to write tests, but testharness
is
> > the new and improved testing framework (and more easily made compatible with
> the
> > cross-user agent Web Platform Tests).
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:205: if
(context
> > == WebURLRequest::RequestContextScript || context ==
> > WebURLRequest::RequestContextImport)
> > Treating RequestContextImport as guaranteed to be a script resource needs an
> > explanatory comment, I think. Is the reasoning just that since integrity is
> only
> > enforced for scripts and stylesheets that any import leading to this code
must
> > be a script import, and not an HTML import or something?
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:650: if
> > (!invalidTokensErrorMessage.isEmpty()) {
> > I think inavildTokensErrorMessage cannot be empty here. I'm happy for this
to
> be
> > a DCHECK that it's not empty, but I don't think it should be a conditional.
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:69: bool
> > allowRequestWithoutMetadata(WebURLRequest::RequestContext, const KURL&,
const
> > IntegrityMetadataSet&, ContentSecurityPolicy::ReportingStatus) const;
> > nit: I'd prefer that this have 'Integrity' somewhere in the method name
since
> I
> > expect that 'metadata' may be used elsewhere in CSP at a later point. So
> perhaps
> > named "allowRequestWithoutIntegrityMetadata"?
> 
> Thanks! Agree for all the comments. All address all of then on 18th, when i
> return from vacation.

Sounds great. FYI, there's a good chance I'll be OOO at that point for a few
weeks, so once you get the go ahead from Mike, don't wait for me.

[Sergey Shekyan, 2016-06-20 05:43:00]

On 2016/06/12 16:38:22, jww wrote:
> On 2016/06/11 23:32:28, shekyan wrote:
> > On 2016/06/11 22:45:12, jww wrote:
> > > Thanks for starting this, Sergey! I've added a few comments on top of
> Mike's.
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
> > > File
> > >
> >
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/LayoutTe...
> > >
> >
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html:4:
> > > <meta http-equiv="Content-Security-Policy" content="require-sri-for
script;
> > > script-src 'unsafe-inline' *">
> > > On 2016/06/10 09:25:15, Mike West wrote:
> > > > Please write this as a `testharness.js` test. See
> > > >
> > >
> >
>
`LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-enforce-allowed.php`
> > > > as an example.
> > > 
> > > Just as an explanation, this is how we used to write tests, but
testharness
> is
> > > the new and improved testing framework (and more easily made compatible
with
> > the
> > > cross-user agent Web Platform Tests).
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:205: if
> (context
> > > == WebURLRequest::RequestContextScript || context ==
> > > WebURLRequest::RequestContextImport)
> > > Treating RequestContextImport as guaranteed to be a script resource needs
an
> > > explanatory comment, I think. Is the reasoning just that since integrity
is
> > only
> > > enforced for scripts and stylesheets that any import leading to this code
> must
> > > be a script import, and not an HTML import or something?
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:650: if
> > > (!invalidTokensErrorMessage.isEmpty()) {
> > > I think inavildTokensErrorMessage cannot be empty here. I'm happy for this
> to
> > be
> > > a DCHECK that it's not empty, but I don't think it should be a
conditional.
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:69: bool
> > > allowRequestWithoutMetadata(WebURLRequest::RequestContext, const KURL&,
> const
> > > IntegrityMetadataSet&, ContentSecurityPolicy::ReportingStatus) const;
> > > nit: I'd prefer that this have 'Integrity' somewhere in the method name
> since
> > I
> > > expect that 'metadata' may be used elsewhere in CSP at a later point. So
> > perhaps
> > > named "allowRequestWithoutIntegrityMetadata"?
> > 
> > Thanks! Agree for all the comments. All address all of then on 18th, when i
> > return from vacation.
> 
> Sounds great. FYI, there's a good chance I'll be OOO at that point for a few
> weeks, so once you get the go ahead from Mike, don't wait for me.

Folks, how do I run testharness tests?

[Sergey Shekyan, 2016-06-20 07:12:00]

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:205: if (context
== WebURLRequest::RequestContextScript || context ==
WebURLRequest::RequestContextImport)
On 2016/06/11 22:45:12, jww wrote:
> Treating RequestContextImport as guaranteed to be a script resource needs an
> explanatory comment, I think. Is the reasoning just that since integrity is
only
> enforced for scripts and stylesheets that any import leading to this code must
> be a script import, and not an HTML import or something?
I inherited the logic that RequestContextImport is enforced for scripts from
ContentSecurityPolicy::allowRequest, which is a little inaccurate.
RequestContext is the closest property of the request that can be aligned with
spec'ed algorithm, which says to use request's destination
(https://fetch.spec.whatwg.org/#concept-request-destination).

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:209:
reportViolation(ContentSecurityPolicy::RequireSRIFor,
ContentSecurityPolicy::RequireSRIFor, "Refused to load the " + resourceType + "
'" + url.elidedString() + "' because 'require-sri-for' directive requires
integrity attribute be present for all " + resourceType + "s.", url,
ResourceRequest::RedirectStatus::NoRedirect);
On 2016/06/10 09:25:15, Mike West (OOO - BlinkOn) wrote:
> `NoRedirect` probably isn't correct here, as I think the current code is going
> to trigger this on every step through a redirect in `report-only` mode. You'll
> either need to refactor the code so that we only do this check on the initial
> step of a request, or pass in the redirect status here so that we properly
strip
> the report data cross-origin.

Acknowledged.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:650: if
(!invalidTokensErrorMessage.isEmpty()) {
On 2016/06/11 22:45:12, jww wrote:
> I think inavildTokensErrorMessage cannot be empty here. I'm happy for this to
be
> a DCHECK that it's not empty, but I don't think it should be a conditional.

Acknowledged.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:929:
parseRequireSRIFor(name, value);
On 2016/06/10 09:25:15, Mike West (OOO - BlinkOn) wrote:
> This functionality should be hidden behind the experimental features flag. You
> could call `ContentSecurityPolicy::experimentalFeaturesEnabled()` before
> parsing, for example.

Acknowledged.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:69: bool
allowRequestWithoutMetadata(WebURLRequest::RequestContext, const KURL&, const
IntegrityMetadataSet&, ContentSecurityPolicy::ReportingStatus) const;
On 2016/06/11 22:45:12, jww wrote:
> nit: I'd prefer that this have 'Integrity' somewhere in the method name since
I
> expect that 'metadata' may be used elsewhere in CSP at a later point. So
perhaps
> named "allowRequestWithoutIntegrityMetadata"?

I renamed it to "allowRequestWithoutIntegrity". Trying to commit a code right
before vacation flight was not a good idea.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:571: if
(!policy->allowRequestWithoutMetadata(context, url, integrityMetedata,
reportingStatus))
On 2016/06/10 09:25:15, Mike West (OOO - BlinkOn) wrote:
> I don't understand the naming choices here. Why is is named one thing on
> ContentSecurityPolicy and another on CSPDirectiveList? I think it makes sense
to
> name both `allowRequestWithoutIntegrity`, and only to call in if the
> IntegrityMetadataSet is empty. See the comments in `::allowRequest`.
> 
> WDYT?

totally agree. Much clearer.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:577: bool
ContentSecurityPolicy::allowRequest(WebURLRequest::RequestContext context, const
KURL& url, const String& nonce, const IntegrityMetadataSet& integrityMetedata,
RedirectStatus redirectStatus, ReportingStatus reportingStatus) const
On 2016/06/10 09:25:15, Mike West (OOO - BlinkOn) wrote:
> Nit: s/Metedata/Metadata/

Acknowledged.

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:579: switch
(context) {
On 2016/06/10 09:25:15, Mike West (OOO - BlinkOn) wrote:
> Rather than calling `checkIntegrityMetadataPresence` inline with the call to
> `allowWhateverFromSource`, it might make things clearer to do something like
the
> following right at the top:
> 
> ```
> if (integrityMetadata.isEmpty() && !allowRequestWithoutIntegrity(context, url,
> redirectStatus, reportingStatus))
>     return false;
> ```

Acknowledged.

[Mike West, 2016-06-20 08:00:25]

On 2016/06/20 at 05:43:00, shekyan wrote:
> Folks, how do I run testharness tests?

Just like you run non-testharness layout tests:
`third_party/WebKit/Tools/Scripts/run-webkit-tests path/to/test.html` (or
`out/Release/content_shell --run-layout-test
http://127.0.0.1:8080/path/to/test.html` to run a single test from the command
line)?

[Mike West, 2016-06-20 08:11:37]

Looking pretty good. Comments inline, and there's still outstanding work from
the last round of comments about layout tests. I hope the comments above gave
you enough info to work on those.

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

https://codereview.chromium.org/2056183002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:205: if (context
== WebURLRequest::RequestContextScript || context ==
WebURLRequest::RequestContextImport)
On 2016/06/20 at 07:12:00, shekyan wrote:
> I inherited the logic that RequestContextImport is enforced for scripts from
ContentSecurityPolicy::allowRequest, which is a little inaccurate.
> RequestContext is the closest property of the request that can be aligned with
spec'ed algorithm, which says to use request's destination
(https://fetch.spec.whatwg.org/#concept-request-destination).

Right. Contexts are from an earlier version of Fetch, before it was refactored
into the initiator/type/destination tuple. Using the context here is the right
thing to do until such time as we refactor WebURLRequest. You'll also need to
add in the Worker types.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:179: if
(m_requireSRIFor == 0)
Nit: Prefer `if (!m_requireSRIFor)` or `if (m_requireSRIFor ==
RequireSRIForToken::None)`.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:210: }
Nit: No {} for single-line bodies.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:97: Script = 1 << 1,
Nit: Start with 0.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:106: //
https://w3c.github.io/webappsec-subresource-integrity/#require-sri-for
Nit: This link doesn't work.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:223:
csp->didReceiveHeader("require-sri-for script style;",
ContentSecurityPolicyHeaderTypeEnforce, ContentSecurityPolicyHeaderSourceMeta);
Please add some report-only tests as well.

[Sergey Shekyan, 2016-06-21 06:34:51]

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:179: if
(m_requireSRIFor == 0)
On 2016/06/20 08:11:37, Mike West (OOO - BlinkOn) wrote:
> Nit: Prefer `if (!m_requireSRIFor)` or `if (m_requireSRIFor ==
> RequireSRIForToken::None)`.

Acknowledged.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:210: }
On 2016/06/20 08:11:37, Mike West (OOO - BlinkOn) wrote:
> Nit: No {} for single-line bodies.

Acknowledged.

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2056183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:106: //
https://w3c.github.io/webappsec-subresource-integrity/#require-sri-for
On 2016/06/20 08:11:37, Mike West (OOO - BlinkOn) wrote:
> Nit: This link doesn't work.

It will once they regenerate the HTML file.

[Sergey Shekyan, 2016-06-21 06:48:17]

There are couple of questions:
- when requesting styles,  `allowRequest()` receives empty integrityMetadataSet.
Is loading styles using Fetch in some hacky way? Any hints to where to look?
- because of above problem, I think I should mark my test as `should pass`, or
remove it until things are fixed. Is there an established way of doing it? 
- I didn't get if I should also commit expectations file for testharness-based
tests? I can only do it for the platform that I am using, e.g. mac-retina..
- should I write LayoutTests for preload, which should be covered by
require-sri-for?
- writing worker tests is hard:(, working on them.

[Mike West, 2016-06-21 07:15:28]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 07:15:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056183002/40001

[Mike West, 2016-06-21 07:29:37]

On 2016/06/21 at 06:48:17, shekyan wrote:
> There are couple of questions:
> - when requesting styles,  `allowRequest()` receives empty
integrityMetadataSet. Is loading styles using Fetch in some hacky way? Any hints
to where to look?

What test is failing?

> - because of above problem, I think I should mark my test as `should pass`, or
remove it until things are fixed. Is there an established way of doing it?

You should either fix the code so that the tests that should pass do pass, or
drop the 'style' portion of the patch for a subsequent patch. It would be bad to
land known-broken code (we land enough unknown-broken code as it is... :) ).

> - I didn't get if I should also commit expectations file for testharness-based
tests? I can only do it for the platform that I am using, e.g. mac-retina..

Nope. As long as all the assertions pass, you don't need a separate `.txt` file.
One of the many advantages of testharness...

> - should I write LayoutTests for preload, which should be covered by
require-sri-for?

You should write layout tests for every behavioral change you'd like to ensure
someone doesn't regress in the future. If it's not tested, it's going to break.

> - writing worker tests is hard:(, working on them.

80% of a patch is figuring out how to write the patch; the other 80% is writing
the tests. :)

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed-meta.html
(right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed-meta.html:5:
<script>
Same comments apply.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed.php
(right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed.php:7:
<script>
More of a question for the spec than this patch, but Apple was asking about
extending SRI to include inline script blocks at the top of our last
face-to-face meeting: minutes at
https://www.w3.org/2016/05/16-webappsec-minutes.html#item02. Would you mind
filing a spec bug and CCing the relevant folks to make sure this is taken care
of one way or another? If it's something we'd like to do, we should be blocking
this script block.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed.php:13:
executed_test.done();
assert_equals(z, 13);

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-blocked.php
(right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-blocked.php:9:
var watcher = new EventWatcher(t, document,
['securitypolicyviolation','securitypolicyviolation']);
Do you expect two securitypoicyviolation events?

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:186: || context ==
WebURLRequest::RequestContextServiceWorker
Is this what `git cl format` came up with? Because it's pretty strangely
formatted. :)

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:189: return false;
Please add {} for multi-line clauses.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:234:
EXPECT_FALSE(csp->allowRequest(WebURLRequest::RequestContextScript, url,
String(), IntegrityMetadataSet(), ResourceRequest::RedirectStatus::NoRedirect,
ContentSecurityPolicy::SuppressReport));
Right now, you're applying both report-only and enforce to the same
ContentSecurityPolicy object, so you're not actually testing that the
report-only header in isolation. Please split them up.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:261:
csp->didReceiveHeader("require-sri-for script style;",
ContentSecurityPolicyHeaderTypeReport, ContentSecurityPolicyHeaderSourceMeta);
Ditto.

[commit-bot: I haz the power, 2016-06-21 08:45:43]

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

[commit-bot: I haz the power, 2016-06-21 08:45:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sergey Shekyan, 2016-06-22 06:25:14]

- Do I need to enable experimental flag somewhere to have LayoutTests by trybot
pass?
- regarding style requests missing integrity metadata: I finally setup Xcode to
load sources and can debug things now, so trying to pass integrity to style
request..
- meanwhile, require-sri-for-style-allowed.php should fail, because of missing
integrity we block all style requests.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:186: || context ==
WebURLRequest::RequestContextServiceWorker
On 2016/06/21 07:29:36, Mike West wrote:
> Is this what `git cl format` came up with? Because it's pretty strangely
> formatted. :)

Acknowledged.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:189: return false;
On 2016/06/21 07:29:36, Mike West wrote:
> Please add {} for multi-line clauses.

Acknowledged.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:234:
EXPECT_FALSE(csp->allowRequest(WebURLRequest::RequestContextScript, url,
String(), IntegrityMetadataSet(), ResourceRequest::RedirectStatus::NoRedirect,
ContentSecurityPolicy::SuppressReport));
On 2016/06/21 07:29:36, Mike West wrote:
> Right now, you're applying both report-only and enforce to the same
> ContentSecurityPolicy object, so you're not actually testing that the
> report-only header in isolation. Please split them up.

I split them and added another set of tests for policy delivered through meta.
But I am not sure what report-only tests here, because allowRequest behaves the
same way both for enforce and report.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:261:
csp->didReceiveHeader("require-sri-for script style;",
ContentSecurityPolicyHeaderTypeReport, ContentSecurityPolicyHeaderSourceMeta);
On 2016/06/21 07:29:36, Mike West wrote:
> Ditto.

Acknowledged.

[Mike West, 2016-06-24 09:25:07]

On 2016/06/22 at 06:25:14, shekyan wrote:
> - Do I need to enable experimental flag somewhere to have LayoutTests by
trybot pass?

No. The flag should be set for layout tests
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Runti...).

> - regarding style requests missing integrity metadata: I finally setup Xcode
to load sources and can debug things now, so trying to pass integrity to style
request..

Looks reasonable.

> - meanwhile, require-sri-for-style-allowed.php should fail, because of missing
integrity we block all style requests.

I've thrown the patch at the bots. Let's see what happens.

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:234:
EXPECT_FALSE(csp->allowRequest(WebURLRequest::RequestContextScript, url,
String(), IntegrityMetadataSet(), ResourceRequest::RedirectStatus::NoRedirect,
ContentSecurityPolicy::SuppressReport));
On 2016/06/22 at 06:25:14, shekyan wrote:
> I split them and added another set of tests for policy delivered through meta.
But I am not sure what report-only tests here, because allowRequest behaves the
same way both for enforce and report.

That shouldn't be the case. That is, `script-src 'none'` has no effect in
report-only mode beyond sending the report: it should return `true` for
`allowRequest`, whereas it should return `false` if in enforce-mode. Similarly,
`allowRequest` should basically always return `true` when `require-sri-for` is
in report-only mode.

https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:211: bool
allowRequestWithoutIntegrity(WebURLRequest::RequestContext, const KURL&,
RedirectStatus = RedirectStatus::NoRedirect, ReportingStatus = SendReport)
const;
Nit: I think this will be simpler to understand if you name it something like
`requireIntegrityMetadataForRequest`. As is, it seems like a variant of
`allowRequest`, when it's really a piece of how `allowRequest` works.

Likewise, does this need to be exposed as part of the public API? Could it be
private?

https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:236:
EXPECT_FALSE(policy->allowRequest(WebURLRequest::RequestContextScript, url,
String(), IntegrityMetadataSet(), ResourceRequest::RedirectStatus::NoRedirect,
ContentSecurityPolicy::SuppressReport));
As noted above, this seems incorrect. Why do we deny the request in report-only
mode?

[Mike West, 2016-06-24 09:25:15]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 09:25:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056183002/100001

[commit-bot: I haz the power, 2016-06-24 10:46:51]

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

[commit-bot: I haz the power, 2016-06-24 10:46:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Shekyan, 2016-06-24 15:46:45]

On 2016/06/24 09:25:07, Mike West wrote:
> On 2016/06/22 at 06:25:14, shekyan wrote:
> > - Do I need to enable experimental flag somewhere to have LayoutTests by
> trybot pass?
> 
> No. The flag should be set for layout tests
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Runti...).
> 
> > - regarding style requests missing integrity metadata: I finally setup Xcode
> to load sources and can debug things now, so trying to pass integrity to style
> request..
> 
> Looks reasonable.
> 
> > - meanwhile, require-sri-for-style-allowed.php should fail, because of
missing
> integrity we block all style requests.
> 
> I've thrown the patch at the bots. Let's see what happens.

My tests fail with:
Regressions: Unexpected missing results (3)
 
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed-meta.html
[ Missing ]
 
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed.php
[ Missing ]
 
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-blocked.php
[ Missing ]

what does this mean?


> 
>
https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2056183002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:234:
> EXPECT_FALSE(csp->allowRequest(WebURLRequest::RequestContextScript, url,
> String(), IntegrityMetadataSet(), ResourceRequest::RedirectStatus::NoRedirect,
> ContentSecurityPolicy::SuppressReport));
> On 2016/06/22 at 06:25:14, shekyan wrote:
> > I split them and added another set of tests for policy delivered through
meta.
> But I am not sure what report-only tests here, because allowRequest behaves
the
> same way both for enforce and report.
> 
> That shouldn't be the case. That is, `script-src 'none'` has no effect in
> report-only mode beyond sending the report: it should return `true` for
> `allowRequest`, whereas it should return `false` if in enforce-mode.
Similarly,
> `allowRequest` should basically always return `true` when `require-sri-for` is
> in report-only mode.
> 

Well, I would expect so too, but probably some piece of information is not set
in the unit test, as allowRequest ignores that header is of type Report. Not
only for my tests, but, for example, ObjectSrc test. I set heder to come from
HTTP, changed it to Report, and still allowRequest returns false when it should
not. 

>
https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):
> 
>
https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:211: bool
> allowRequestWithoutIntegrity(WebURLRequest::RequestContext, const KURL&,
> RedirectStatus = RedirectStatus::NoRedirect, ReportingStatus = SendReport)
> const;
> Nit: I think this will be simpler to understand if you name it something like
> `requireIntegrityMetadataForRequest`. As is, it seems like a variant of
> `allowRequest`, when it's really a piece of how `allowRequest` works.
> 
> Likewise, does this need to be exposed as part of the public API? Could it be
> private?
> 
>
https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:236:
> EXPECT_FALSE(policy->allowRequest(WebURLRequest::RequestContextScript, url,
> String(), IntegrityMetadataSet(), ResourceRequest::RedirectStatus::NoRedirect,
> ContentSecurityPolicy::SuppressReport));
> As noted above, this seems incorrect. Why do we deny the request in
report-only
> mode?

[Mike West, 2016-06-24 17:50:49]

On 2016/06/24 at 15:46:45, shekyan wrote:
> My tests fail with:
> Regressions: Unexpected missing results (3)
>  
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed-meta.html
[ Missing ]
>  
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed.php
[ Missing ]
>  
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-blocked.php
[ Missing ]
> 
> what does this mean?

If you click through to the results
(https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...,
for example), you'll see that the integrity metadata for the test harness is
incorrect. Fixing that should fix your tests.


> Well, I would expect so too, but probably some piece of information is not set
in the unit test, as allowRequest ignores that header is of type Report. Not
only for my tests, but, for example, ObjectSrc test. I set heder to come from
HTTP, changed it to Report, and still allowRequest returns false when it should
not. 

That's a bug. You should fix it. :)

It looks like you're returning the wrong value from
CSPDirectiveList::checkRequestWithoutIntegrity, as noted below. That might fix
the issue.

https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:190: return false;
Here and on #193, you should be returning `denyIfEnforcingPolicy()`.

[Sergey Shekyan, 2016-06-27 08:13:39]

On 2016/06/24 17:50:49, Mike West wrote:
> On 2016/06/24 at 15:46:45, shekyan wrote:
> > My tests fail with:
> > Regressions: Unexpected missing results (3)
> >  
>
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed-meta.html
> [ Missing ]
> >  
>
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-allowed.php
> [ Missing ]
> >  
>
http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-blocked.php
> [ Missing ]
> > 
> > what does this mean?
> 
> If you click through to the results
>
(https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...,
> for example), you'll see that the integrity metadata for the test harness is
> incorrect. Fixing that should fix your tests.
> 
Somehow my local tests use different testharnessreport.js, so I whitelisted both
local and trybot's hashes..
> 
> > Well, I would expect so too, but probably some piece of information is not
set
> in the unit test, as allowRequest ignores that header is of type Report. Not
> only for my tests, but, for example, ObjectSrc test. I set heder to come from
> HTTP, changed it to Report, and still allowRequest returns false when it
should
> not. 
> 
> That's a bug. You should fix it. :)
> 
> It looks like you're returning the wrong value from
> CSPDirectiveList::checkRequestWithoutIntegrity, as noted below. That might fix
> the issue.
> 
>
https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):
> 
>
https://codereview.chromium.org/2056183002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:190: return
false;
> Here and on #193, you should be returning `denyIfEnforcingPolicy()`.

Thanks!

[Mike West, 2016-06-28 05:34:54]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 05:35:12]

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-06-28 06:54:21]

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

[commit-bot: I haz the power, 2016-06-28 06:54:22]

Dry run: 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_...)

[Sergey Shekyan, 2016-06-28 07:36:14]

On 2016/06/28 06:54:22, commit-bot: I haz the power wrote:
> Dry run: 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_...)

Mike, sorry, I forgot to update CSPDirectiveListTest, will push it in a bit.
Some questions:
- how do I get the link you pointed in one of the previous comments ? 
>If you click through to the results
>(https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...,
>for example), you'll see that the integrity metadata for the test harness is
>incorrect. Fixing that should fix your tests.
- should blocking worker generate a SecurityPolicyViolationEvent? It does not
now...
- sharedWorker is different and is not actually blocked by main document's
policy, ContentSecurityPolicy's m_policies is empty..Will try to figure out
tomorrow.

[Sergey Shekyan, 2016-06-30 04:09:55]

Can you please try to build this?

[Sergey Shekyan, 2016-06-30 04:53:31]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 04:53:52]

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-06-30 04:53:53]

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

[commit-bot: I haz the power, 2016-06-30 04:53:55]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Mike West, 2016-07-04 08:52:18]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 08:52:28]

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-07-04 10:07:47]

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

[commit-bot: I haz the power, 2016-07-04 10:07:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sergey Shekyan, 2016-07-04 22:38:03]

On 2016/07/04 10:07:49, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

Mike, is this my fault? Failing test passes on my mac, and I don't think I
touched anything there. 
Also, it is still a mystery for me on how to look for failed test details:)

[Sergey Shekyan, 2016-07-06 04:59:48]

On 2016/07/04 22:38:03, shekyan wrote:
> On 2016/07/04 10:07:49, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> 
> Mike, is this my fault? Failing test passes on my mac, and I don't think I
> touched anything there. 
> Also, it is still a mystery for me on how to look for failed test details:)


Looked again and pretty sure tests are flaky. Do I need to rebase before
uploading (don't think so but clarifying just in case)?

[Mike West, 2016-07-06 06:26:20]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 06:26:29]

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

[Mike West, 2016-07-06 06:32:16]

LGTM with a nit. Thanks for taking so many passes at this.

https://codereview.chromium.org/2056183002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right):

https://codereview.chromium.org/2056183002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:49: KURL
scriptURL = resolveURL(url, exceptionState,
WebURLRequest::RequestContextUnspecified);
These aren't well defined yet. I'd suggest running with `RequestContextScript`,
as these can't have their own policy (at the moment).

Would you mind adding `// TODO(mkwst): Revisit the context as
https://drafts.css-houdini.org/worklets/ evolves.`?

[Sergey Shekyan, 2016-07-06 07:16:58]

https://codereview.chromium.org/2056183002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right):

https://codereview.chromium.org/2056183002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:49: KURL
scriptURL = resolveURL(url, exceptionState,
WebURLRequest::RequestContextUnspecified);
On 2016/07/06 06:32:16, Mike West wrote:
> These aren't well defined yet. I'd suggest running with
`RequestContextScript`,
> as these can't have their own policy (at the moment).
> 
> Would you mind adding `// TODO(mkwst): Revisit the context as
> https://drafts.css-houdini.org/worklets/ evolves.`?

Acknowledged.

[Sergey Shekyan, 2016-07-06 07:18:04]

The CQ bit was checked by shekyan@gmail.com

[Sergey Shekyan, 2016-07-06 07:18:05]

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/2056183002/#ps180001
(title: "Addressing comments")

[commit-bot: I haz the power, 2016-07-06 07:18:18]

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-07-06 08:50:16]

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

[commit-bot: I haz the power, 2016-07-06 08:50:19]

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_...)

[Mike West, 2016-07-06 10:31:47]

Before landing, please update the CL description to reflect what you've actually
done in this patch. :)

[Sergey Shekyan, 2016-07-06 17:00:51]

Description was changed from

==========
Expreriment with 'require-sri-for'

This commit is an attempt to implement 'require-sri-for' CSP directive
that is described in
https://github.com/w3c/webappsec-subresource-integrity/pull/32.
This directive allows to opt-in to require integrity metadata be present on the
request
for the resource. Few tests are added and comments on areas where to increase
test
coverage are welcome. Parsing of the directive could've been better, will
rewrite
once the approach is approved.

BUG=618924
R=mkwst@chromium.org
==========

to

==========
As defined in
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows to block any resource requests that do not contain
integrity metadata. This covers external scripts, workers, shared workers,
service workers, external stylesheets, as well as  preload requests, requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-06 17:01:22]

On 2016/07/06 10:31:47, Mike West wrote:
> Before landing, please update the CL description to reflect what you've
actually
> done in this patch. :)

Is this description looks ok?

[Sergey Shekyan, 2016-07-06 17:11:08]

Description was changed from

==========
As defined in
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows to block any resource requests that do not contain
integrity metadata. This covers external scripts, workers, shared workers,
service workers, external stylesheets, as well as  preload requests, requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

to

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows to block any resource requests
that do not contain integrity metadata. This covers external
scripts, workers, shared workers, service workers,
external stylesheets, as well as  preload requests, requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-06 17:13:00]

Description was changed from

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows to block any resource requests
that do not contain integrity metadata. This covers external
scripts, workers, shared workers, service workers,
external stylesheets, as well as  preload requests, requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

to

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows to block resource requests
that do not contain integrity metadata. This covers contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, as well as  preload requests and requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-06 20:33:47]

Description was changed from

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows to block resource requests
that do not contain integrity metadata. This covers contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, as well as  preload requests and requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

to

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows developer to block resource requests
that do not contain integrity metadata. This includes contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, preload requests, and requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-06 21:39:29]

Description was changed from

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows developer to block resource requests
that do not contain integrity metadata. This includes contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, preload requests, and requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

to

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows developers to block resource requests
that do not contain integrity metadata. This includes contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, preload requests, and requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-07 06:40:52]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 06:41:10]

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-07-07 09:41:14]

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

[commit-bot: I haz the power, 2016-07-07 09:41:15]

Dry run: 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_...)

[Sergey Shekyan, 2016-07-07 15:35:52]

The CQ bit was checked by shekyan@gmail.com

[Sergey Shekyan, 2016-07-07 15:35:52]

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/2056183002/#ps220001
(title: "Added SVG script test")

[commit-bot: I haz the power, 2016-07-07 15:36:34]

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-07-07 18:33:10]

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

[commit-bot: I haz the power, 2016-07-07 18:33:13]

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_...)

[Sergey Shekyan, 2016-07-07 19:49:51]

The CQ bit was checked by shekyan@gmail.com

[commit-bot: I haz the power, 2016-07-07 19:50:32]

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-07-07 21:48:08]

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

[commit-bot: I haz the power, 2016-07-07 21:48:10]

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_...)

[Sergey Shekyan, 2016-07-08 00:07:34]

The CQ bit was checked by shekyan@gmail.com

[commit-bot: I haz the power, 2016-07-08 00:08:17]

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-07-08 01:46:08]

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

[commit-bot: I haz the power, 2016-07-08 01:46:10]

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_...)

[Sergey Shekyan, 2016-07-08 03:53:50]

The CQ bit was checked by shekyan@gmail.com

[commit-bot: I haz the power, 2016-07-08 03:54:21]

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

[Mike West, 2016-07-08 04:42:26]

On 2016/07/08 at 03:54:21, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

The failures at
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...
look real. I don't think hitting the CQ button again is going to help. :)

It looks like we're generating the wrong integrity information for
`testharnessreport.js` on Windows, which is strange indeed (especially since
we're not failing for `testharness.js` which is loaded first. I'll try to look
into this today.

[Sergey Shekyan, 2016-07-08 04:52:44]

On 2016/07/08 04:42:26, Mike West wrote:
> On 2016/07/08 at 03:54:21, commit-bot wrote:
> > CQ is trying da patch. Follow status at
> >  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> The failures at
>
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...
> look real. I don't think hitting the CQ button again is going to help. :)
> 
> It looks like we're generating the wrong integrity information for
> `testharnessreport.js` on Windows, which is strange indeed (especially since
> we're not failing for `testharness.js` which is loaded first. I'll try to look
> into this today.

Haha, thanks! windows failures started after I removed php tags where are not
needed, I think...

[commit-bot: I haz the power, 2016-07-08 06:09:13]

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

[commit-bot: I haz the power, 2016-07-08 06:09:16]

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_...)

[Sergey Shekyan, 2016-07-13 05:53:11]

Any luck on Windows?

[Mike West, 2016-07-13 07:14:54]

On 2016/07/13 at 05:53:11, shekyan wrote:
> Any luck on Windows?

Nope, sorry. I haven't had time to get a windows box set up for myself.

I'd suggest that you file a bug and assign it to me, and unblock this patch by
moving the CSP declaration under the `<script>` tags. That is:

```
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<meta http-equiv="Content-Security-Policy" content="require-sri-for script;
script-src 'self' 'unsafe-inline'"">

```

Also, I notice that we never sent an "intent to implement" for this feature.
Please do that before landing it by copy/pasting the template at
https://docs.google.com/document/d/1vlTlsQKThwaX0-lj_iZbVTzyqY7LioqERU8DK3u3X...
and sending it to blink-dev@. I've created a `chromestatus.com` entry for you at
https://www.chromestatus.com/features/5635811978510336.

[Sergey Shekyan, 2016-07-13 22:58:33]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[Sergey Shekyan, 2016-07-13 22:59:09]

On 2016/07/13 07:14:54, Mike West wrote:
> On 2016/07/13 at 05:53:11, shekyan wrote:
> > Any luck on Windows?
> 
> Nope, sorry. I haven't had time to get a windows box set up for myself.
> 
> I'd suggest that you file a bug and assign it to me, and unblock this patch by
> moving the CSP declaration under the `<script>` tags. That is:
> 
> ```
> <script src="/resources/testharness.js"></script>
> <script src="/resources/testharnessreport.js"></script>
> <meta http-equiv="Content-Security-Policy" content="require-sri-for script;
> script-src 'self' 'unsafe-inline'"">
> 
> ```
> 
> Also, I notice that we never sent an "intent to implement" for this feature.
> Please do that before landing it by copy/pasting the template at
>
https://docs.google.com/document/d/1vlTlsQKThwaX0-lj_iZbVTzyqY7LioqERU8DK3u3X...
> and sending it to blink-dev@. I've created a `chromestatus.com` entry for you
at
> https://www.chromestatus.com/features/5635811978510336.

Thanks for making this moving. 
Updated tests, report-only tests kind of become useless, commented accordingly.

Reported a bug https://crbug.com/627966 on windows issue. Intent to implement is
sent out.

[commit-bot: I haz the power, 2016-07-13 22:59:15]

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-07-14 00:33:39]

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

[commit-bot: I haz the power, 2016-07-14 00:33:41]

Dry run: This issue passed the CQ dry run.

[Sergey Shekyan, 2016-07-14 00:41:33]

Dry run passed. I'll commit after you say so:)

[Mike West, 2016-07-14 05:25:07]

On 2016/07/14 at 00:41:33, shekyan wrote:
> Dry run passed. I'll commit after you say so:)

One test nit (I'd like you to change the report-only test to expect the
additional failure rather than commenting it out entirely).

I'd also like you to change the CL description to include a link to the intent
to implement
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/jyCdW1dHyYA/YefRSKs1...),
and to have a title like "Implement the 'require-sri-for' CSP directive". Just
put that on a line by itself with a blank line under it. Wrapping the commit
message to ~70 characters would also be lovely. :)

Otherwise LGTM.

https://codereview.chromium.org/2056183002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php
(right):

https://codereview.chromium.org/2056183002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:4:
?> -->
I'd prefer for you to adjust the test to expect the `testharnessreport.js`
failure, rather than gutting it entirely.

[Sergey Shekyan, 2016-07-14 06:34:45]

Description was changed from

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html#opt...,
this CSP directive allows developers to block resource requests
that do not contain integrity metadata. This includes contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, preload requests, and requests
originated by CSS @import.

BUG=618924
R=mkwst@chromium.org
==========

to

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/#opt-in-require-sri-for,
this CSP directive allows developers to block resource requests
that do not contain integrity metadata. This includes contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, preload requests, and requests
originated by CSS @import.
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jyCdW1dHyYA/Ye...

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-14 06:52:39]

Updated the CL description and addressed nits.

https://codereview.chromium.org/2056183002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php
(right):

https://codereview.chromium.org/2056183002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:4:
?> -->
On 2016/07/14 05:25:07, Mike West wrote:
> I'd prefer for you to adjust the test to expect the `testharnessreport.js`
> failure, rather than gutting it entirely.

I cannot figure out how to expect the failure without changing the test much,
except introducing .expected file :( Let me know if I can make it better.

[Mike West, 2016-07-14 07:29:50]

https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php
(right):

https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:4:
?> -->
Drop the `<!--/-->`.

https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:17:
}, "Script without integrity generates reports.");
I don't think you'll have to change this, actually, as the `testharness.js` and
`testharnessreport.js` bits are already processed and fired by the time you set
up the listener. If you drop the comment above, you should get a passing test.

[Mike West, 2016-07-14 07:33:40]

Description was changed from

==========
As defined in 
https://w3c.github.io/webappsec-subresource-integrity/#opt-in-require-sri-for,
this CSP directive allows developers to block resource requests
that do not contain integrity metadata. This includes contexts like external
scripts, workers, shared workers, service workers,
external stylesheets, preload requests, and requests
originated by CSS @import.
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jyCdW1dHyYA/Ye...

BUG=618924
R=mkwst@chromium.org
==========

to

==========
Implement the `require-sri-for` CSP directive

As defined in [1], this CSP directive allows developers to block resource
requests that do not contain integrity metadata. This includes contexts
like external scripts, workers, shared workers, service workers, external
stylesheets, preload requests, and requests originated by CSS @import.

[1]:
https://w3c.github.io/webappsec-subresource-integrity/#opt-in-require-sri-for

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jyCdW1dHyYA/Ye...

BUG=618924
R=mkwst@chromium.org
==========

[Sergey Shekyan, 2016-07-14 08:01:33]

https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php
(right):

https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:17:
}, "Script without integrity generates reports.");
On 2016/07/14 07:29:49, Mike West wrote:
> I don't think you'll have to change this, actually, as the `testharness.js`
and
> `testharnessreport.js` bits are already processed and fired by the time you
set
> up the listener. If you drop the comment above, you should get a passing test.

Indeed, there is almost nothing to change. `testharnessreport.js` violation is
catched by the EventWatcher though. I'll look tomorrow, if you think in can
flaky.

[Sergey Shekyan, 2016-07-14 08:02:16]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 08:02:40]

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

[Mike West, 2016-07-14 08:46:38]

On 2016/07/14 at 08:01:33, shekyan wrote:
>
https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php
(right):
> 
>
https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:17:
}, "Script without integrity generates reports.");
> On 2016/07/14 07:29:49, Mike West wrote:
> > I don't think you'll have to change this, actually, as the `testharness.js`
and
> > `testharnessreport.js` bits are already processed and fired by the time you
set
> > up the listener. If you drop the comment above, you should get a passing
test.
> 
> Indeed, there is almost nothing to change. `testharnessreport.js` violation is
catched by the EventWatcher though. I'll look tomorrow, if you think in can
flaky.

If it's caught by eventwatcher, that's fine. Just add another expectation. See
`//LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-multiple-blocked.php`
for an example of how that might look (basically, `return
watcher.wait_for('securitypolicyviolation')` from the end of the first handler).

[commit-bot: I haz the power, 2016-07-14 09:26:30]

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

[commit-bot: I haz the power, 2016-07-14 09:26:31]

Dry run: This issue passed the CQ dry run.

[Sergey Shekyan, 2016-07-14 17:35:30]

On 2016/07/14 08:46:38, Mike West wrote:
> On 2016/07/14 at 08:01:33, shekyan wrote:
> >
>
https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
> > File
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php
> (right):
> > 
> >
>
https://codereview.chromium.org/2056183002/diff/280001/third_party/WebKit/Lay...
> >
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for/require-sri-for-script-reportonly-blocked.php:17:
> }, "Script without integrity generates reports.");
> > On 2016/07/14 07:29:49, Mike West wrote:
> > > I don't think you'll have to change this, actually, as the
`testharness.js`
> and
> > > `testharnessreport.js` bits are already processed and fired by the time
you
> set
> > > up the listener. If you drop the comment above, you should get a passing
> test.
> > 
> > Indeed, there is almost nothing to change. `testharnessreport.js` violation
is
> catched by the EventWatcher though. I'll look tomorrow, if you think in can
> flaky.
> 
> If it's caught by eventwatcher, that's fine. Just add another expectation. See
>
`//LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-multiple-blocked.php`
> for an example of how that might look (basically, `return
> watcher.wait_for('securitypolicyviolation')` from the end of the first
handler).


Done. Will try to commit now.

[Sergey Shekyan, 2016-07-14 17:35:38]

The CQ bit was checked by shekyan@gmail.com

[Sergey Shekyan, 2016-07-14 17:35:39]

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/2056183002/#ps310001
(title: "Updated tests")

[commit-bot: I haz the power, 2016-07-14 17:35:51]

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-07-14 19:10:50]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-07-14 19:11:14]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 19:12:46]

Description was changed from

==========
Implement the `require-sri-for` CSP directive

As defined in [1], this CSP directive allows developers to block resource
requests that do not contain integrity metadata. This includes contexts
like external scripts, workers, shared workers, service workers, external
stylesheets, preload requests, and requests originated by CSS @import.

[1]:
https://w3c.github.io/webappsec-subresource-integrity/#opt-in-require-sri-for

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jyCdW1dHyYA/Ye...

BUG=618924
R=mkwst@chromium.org
==========

to

==========
Implement the `require-sri-for` CSP directive

As defined in [1], this CSP directive allows developers to block resource
requests that do not contain integrity metadata. This includes contexts
like external scripts, workers, shared workers, service workers, external
stylesheets, preload requests, and requests originated by CSS @import.

[1]:
https://w3c.github.io/webappsec-subresource-integrity/#opt-in-require-sri-for

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jyCdW1dHyYA/Ye...

BUG=618924
R=mkwst@chromium.org

Committed: https://crrev.com/615df311feca979548acd4f73ce3d7ac34449e3a
Cr-Commit-Position: refs/heads/master@{#405530}
==========

[commit-bot: I haz the power, 2016-07-14 19:12:49]

Patchset 17 (id:??) landed as
https://crrev.com/615df311feca979548acd4f73ce3d7ac34449e3a
Cr-Commit-Position: refs/heads/master@{#405530}