Refactor nonce support.

Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in 'ScriptLoader', and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This patch
moves nonce checking into 'FrameFetchContext::canRequest' by attaching
it to 'ResourceLoaderOptions', and using that new data inside the
'ContentSecurityPolicy::allow*' checks to ensure that each active policy
gets a crack at reporting violations.

To prevent regression, this patch adds a number of unit tests, moves
the existing nonce layout tests to a separate directory, and adds a
few layout tests as well.

BUG=614416, 611652, 614802
Committed: https://crrev.com/cb18719cbfb2652bd1f0c89ff2b43fc431d9a3a7
Cr-Commit-Position: refs/heads/master@{#398036}

[Mike West, 2016-06-02 07:11:53]

Description was changed from

==========
WIP: Refactor nonce support entirely.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in ScriptLoader, and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms).

This patch is most of the way there; uploading for safe-keeping.

BUG=614416,611652,614802
==========

to

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in ScriptLoader, and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This is
a fairly small change in and of itself, but ends up touching more of
the platform than I expected (because we have to pipe the nonce value
around through the lifetime of a request).

WIP: This patch doesn't yet handle redirects correctly.

BUG=614416,611652,614802
==========

[Mike West, 2016-06-02 09:59:15]

Description was changed from

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in ScriptLoader, and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This is
a fairly small change in and of itself, but ends up touching more of
the platform than I expected (because we have to pipe the nonce value
around through the lifetime of a request).

WIP: This patch doesn't yet handle redirects correctly.

BUG=614416,611652,614802
==========

to

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in 'ScriptLoader', and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This patch
moves nonce checking into 'FrameFetchContext::canRequest' by attaching
it to 'ResourceLoaderOptions', and using that new data inside the
'ContentSecurityPolicy::allow*' checks to ensure that each active policy
gets a crack at reporting violations.

BUG=614416,611652,614802
==========

[Mike West, 2016-06-02 10:58:38]

Patchset #2 (id:20001) has been deleted

[Mike West, 2016-06-02 10:59:32]

Description was changed from

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in 'ScriptLoader', and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This patch
moves nonce checking into 'FrameFetchContext::canRequest' by attaching
it to 'ResourceLoaderOptions', and using that new data inside the
'ContentSecurityPolicy::allow*' checks to ensure that each active policy
gets a crack at reporting violations.

BUG=614416,611652,614802
==========

to

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in 'ScriptLoader', and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This patch
moves nonce checking into 'FrameFetchContext::canRequest' by attaching
it to 'ResourceLoaderOptions', and using that new data inside the
'ContentSecurityPolicy::allow*' checks to ensure that each active policy
gets a crack at reporting violations.

To prevent regression, this patch adds a number of unit tests, moves
the existing nonce layout tests to a separate directory, and adds a
few layout tests as well.

BUG=614416,611652,614802
==========

[Mike West, 2016-06-02 11:01:05]

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

[Mike West, 2016-06-02 11:01:06]

Emily, Joel, do you have some time to look at this patch? It's not as big as it
looks; most of the real changes are tests.

Thanks!

[estark, 2016-06-02 18:57:32]

Mostly l-g-t-m with some nits/questions, will come back and look at the layout
tests after lunch

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:58: {
"style-src 'nonce-yay'", "boo", false },
Maybe a test case or two for empty and null strings? e.g.
{ "script-src 'nonce-yay'", "", false }
{ "script-src 'nonce-yay'", String(), false }

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:106:
SCOPED_TRACE(testing::Message() << "List: `" << test.list << "`, URL: `" <<
test.url << "`");
Did you intend to leave this in or was it just for debugging?

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:127: //
Doesn't effect lists without nonces:
the nittiest of nits (here and below): affect

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:131: {
"script-src https://example.com", "https://not.example.com/script.js", "boo",
false },
same suggestion about/empty null strings here perhaps (testing that an empty
nonce doesn't let you bypass the whitelist)

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:143:
SCOPED_TRACE(testing::Message() << "List: `" << test.list << "`, URL: `" <<
test.url << "`");
ditto, did you mean to leave this in?

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:155: 
Is there a reason you mostly test scripts in this file, and not styles too? You
could omit the "script-" in each test case |list|, and run each test case twice,
once prepending "script-" to the |list| and once prepending "style-".

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

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:883:
didSendViolationReport(stringifiedReport);
Why did this move up here? Seems odd to call something called didX() immediately
*before* doing X, and especially because we might not end up actually sending
the report (if there is no frame() or no report endpoints).

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:887: if
(LocalFrame* frame = document->frame()) {
nit: early return would be more consistent with chromium style

if (!frame)
  return;
frame->...

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:154: bool
allowInlineScript(const String& contextURL, const String& nonce, const
WTF::OrdinalNumber& contextLine, const String& scriptContent, ReportingStatus =
SendReport) const;
Might be worth a comment to say that you should pass String() to indicate the
resource is being loaded with no nonce.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:201: // as
we've done with nonces.
crbug #?

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

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:231:
SCOPED_TRACE(testing::Message() << "Policy: `" << test.policy << "`, URL: `" <<
test.url << "`, Nonce: `" << test.nonce << "`");
ditto

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:272:
SCOPED_TRACE(testing::Message() << "Policy: `" << test.policy << "`, Nonce: `"
<< test.nonce << "`");
ditto

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:274:
unsigned expectedReports = test.allowed ? 0u : 1u;
nit: maybe do this in the test above too instead of computing inline in the
expectation (just for consistency)

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:338:
SCOPED_TRACE(testing::Message() << "Policy: `" << test.policy1 << "`/`" <<
test.policy2 << "`, URL: `" << test.url << "`, Nonce: `" << test.nonce << "`");
ditto... I'm starting to think maybe you meant to have these here after all...

[estark, 2016-06-02 19:52:37]

lgtm with another test suggestion

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-reportonly-allowed.php
(right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-reportonly-allowed.php:20:
</script>
Perhaps this should also test that "mis-nonced" script blocks also generate
reports? (i.e. a similar test but for a script block that has a nonce that
doesn't appear in the policy)

[jww, 2016-06-02 23:45:29]

lgtm, with one nit/question. estark's comments basically map to mine.

As a thought, it does seem like this approach is going to be harder to do for
hashes, since we can't really just take one hash of the resource and pass it
around. I suppose we can take all the hashes and pass that set around? I don't
know what's reasonable there.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ScriptResource.h (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.h:90: AtomicString
contentSecurityPolicyNonce() const { return m_contentSecurityPolicyNonce; }
Why are these specific to ScriptResource and not Resource (or TextResource,
perhaps)? At the very least, you'll have to repeat this for style, right?

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:106:
SCOPED_TRACE(testing::Message() << "List: `" << test.list << "`, URL: `" <<
test.url << "`");
On 2016/06/02 18:57:31, estark wrote:
> Did you intend to leave this in or was it just for debugging?

tl;dr I'm pretty sure this is on purpose. I believe it is standard unit testing
messaging that allows you to effectively have a different message for each run
so that if ASSERTS or EXPECTS fail mid-run, you get this message so that you
know where you failed.

[estark, 2016-06-02 23:49:34]

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:106:
SCOPED_TRACE(testing::Message() << "List: `" << test.list << "`, URL: `" <<
test.url << "`");
On 2016/06/02 23:45:29, jww wrote:
> On 2016/06/02 18:57:31, estark wrote:
> > Did you intend to leave this in or was it just for debugging?
> 
> tl;dr I'm pretty sure this is on purpose. I believe it is standard unit
testing
> messaging that allows you to effectively have a different message for each run
> so that if ASSERTS or EXPECTS fail mid-run, you get this message so that you
> know where you failed.

Oh! Cool! TIL. Thanks.

[Mike West, 2016-06-04 06:30:56]

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-reportonly-allowed.php
(right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/nonces/script-reportonly-allowed.php:20:
</script>
On 2016/06/02 at 19:52:37, estark wrote:
> Perhaps this should also test that "mis-nonced" script blocks also generate
reports? (i.e. a similar test but for a script block that has a nonce that
doesn't appear in the policy)

I added a number of new tests for scripts and styles. Thanks!

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ScriptResource.h (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.h:90: AtomicString
contentSecurityPolicyNonce() const { return m_contentSecurityPolicyNonce; }
On 2016/06/02 at 23:45:28, jww wrote:
> Why are these specific to ScriptResource and not Resource (or TextResource,
perhaps)? At the very least, you'll have to repeat this for style, right?

Thanks for catching this! It's dead code leftover from a previous iteration at
this patch, before I moved things to ResourceLoaderOptions (style is taken care
of in HTMLLinkElement.cpp).

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:58: {
"style-src 'nonce-yay'", "boo", false },
On 2016/06/02 at 18:57:31, estark wrote:
> Maybe a test case or two for empty and null strings? e.g.
> { "script-src 'nonce-yay'", "", false }
> { "script-src 'nonce-yay'", String(), false }

Added it into the `for` loop, as these should always return false.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:106:
SCOPED_TRACE(testing::Message() << "List: `" << test.list << "`, URL: `" <<
test.url << "`");
On 2016/06/02 at 23:49:34, estark wrote:
> On 2016/06/02 23:45:29, jww wrote:
> > On 2016/06/02 18:57:31, estark wrote:
> > > Did you intend to leave this in or was it just for debugging?
> > 
> > tl;dr I'm pretty sure this is on purpose. I believe it is standard unit
testing
> > messaging that allows you to effectively have a different message for each
run
> > so that if ASSERTS or EXPECTS fail mid-run, you get this message so that you
> > know where you failed.
> 
> Oh! Cool! TIL. Thanks.

Yeah, if you're combining a bunch of test cases into one test like I'm doing
here, then SCOPED_TRACE is super helpful 3 weeks from now when something breaks,
as it'll dump detail about what's going on, which is nice!

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:127: //
Doesn't effect lists without nonces:
On 2016/06/02 at 18:57:31, estark wrote:
> the nittiest of nits (here and below): affect

These words are my nemeses. :/

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:131: {
"script-src https://example.com", "https://not.example.com/script.js", "boo",
false },
On 2016/06/02 at 18:57:31, estark wrote:
> same suggestion about/empty null strings here perhaps (testing that an empty
nonce doesn't let you bypass the whitelist)

Done.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:155: 
On 2016/06/02 at 18:57:31, estark wrote:
> Is there a reason you mostly test scripts in this file, and not styles too?
You could omit the "script-" in each test case |list|, and run each test case
twice, once prepending "script-" to the |list| and once prepending "style-".

I've added style tests, thanks!

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

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:883:
didSendViolationReport(stringifiedReport);
On 2016/06/02 at 18:57:31, estark wrote:
> Why did this move up here? Seems odd to call something called didX()
immediately *before* doing X, and especially because we might not end up
actually sending the report (if there is no frame() or no report endpoints).

This moved because I wanted to write unit tests that verified that reports were
being sent, and ensuring that the HashSet of report data is populated seemed
like the lowest-friction approach to getting that done. I agree that it's not
terribly elegant, though. I'll look into doing this more cleanly.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:887: if
(LocalFrame* frame = document->frame()) {
On 2016/06/02 at 18:57:31, estark wrote:
> nit: early return would be more consistent with chromium style
> 
> if (!frame)
>   return;
> frame->...

Done.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:154: bool
allowInlineScript(const String& contextURL, const String& nonce, const
WTF::OrdinalNumber& contextLine, const String& scriptContent, ReportingStatus =
SendReport) const;
On 2016/06/02 at 18:57:32, estark wrote:
> Might be worth a comment to say that you should pass String() to indicate the
resource is being loaded with no nonce.

Done.

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:201: // as
we've done with nonces.
On 2016/06/02 at 18:57:31, estark wrote:
> crbug #?

Done.

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

https://codereview.chromium.org/2020223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:274:
unsigned expectedReports = test.allowed ? 0u : 1u;
On 2016/06/02 at 18:57:32, estark wrote:
> nit: maybe do this in the test above too instead of computing inline in the
expectation (just for consistency)

Done!

[Mike West, 2016-06-06 11:58:49]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-06-06 11:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2020223002/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-06-06 11:58:58]

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

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

Committed patchset #5 (id:100001)

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

Description was changed from

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in 'ScriptLoader', and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This patch
moves nonce checking into 'FrameFetchContext::canRequest' by attaching
it to 'ResourceLoaderOptions', and using that new data inside the
'ContentSecurityPolicy::allow*' checks to ensure that each active policy
gets a crack at reporting violations.

To prevent regression, this patch adds a number of unit tests, moves
the existing nonce layout tests to a separate directory, and adds a
few layout tests as well.

BUG=614416,611652,614802
==========

to

==========
Refactor nonce support to correctly handle report-only policy.

In order to correctly handle report-only, we need to stop thinking of
nonces as a one-time bypass in 'ScriptLoader', and start thinking of it
as an ongoing check associated with a request (as it's specced in
https://w3c.github.io/webappsec-csp/#script-src-algorithms). This patch
moves nonce checking into 'FrameFetchContext::canRequest' by attaching
it to 'ResourceLoaderOptions', and using that new data inside the
'ContentSecurityPolicy::allow*' checks to ensure that each active policy
gets a crack at reporting violations.

To prevent regression, this patch adds a number of unit tests, moves
the existing nonce layout tests to a separate directory, and adds a
few layout tests as well.

BUG=614416,611652,614802

Committed: https://crrev.com/cb18719cbfb2652bd1f0c89ff2b43fc431d9a3a7
Cr-Commit-Position: refs/heads/master@{#398036}
==========

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

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