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.

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/LayoutTests/http/tests/security/contentSecurityPolicy/require-sri-for-script-allowed.html ...

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.

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

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

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

Looking pretty good. Comments inline, and there's still outstanding work from the last round of ...

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.

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?

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?

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()`.

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!

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.

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.

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

On 2016/07/04 22:38:03, shekyan wrote: > On 2016/07/04 10:07:49, commit-bot: I haz the power wrote: ...

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)?

LGTM with a nit. Thanks for taking so many passes at this. https://codereview.chromium.org/2056183002/diff/160001/third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp ...

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

On 2016/07/06 10:31:47, Mike West wrote: > Before landing, please update the CL description to ...

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.

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.

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.

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.

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

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.