Implementation of W3C compliant CSP script-src nonce.

Implementation of W3C compliant CSP script-src nonce. Removes the old 'script-nonce' implementation.

See section 3.2.2 of https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html

BUG=236669
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150541

[abarth-chromium, 2013-05-14 05:58:16]

Great start!  Just a few minor comments below.  Thanks for the CL.

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
Source/core/loader/cache/CachedResourceLoader.cpp:382: const String& nonce =
(request.initiatorElement() != NULL) ?
request.initiatorElement()->fastGetAttribute(HTMLNames::nonceAttr) :
AtomicString();
Can we put this in the CachedResourceRequest instead?  Then we can pass the
CachedResourceRequest directly to canRequest instead of exploding out the nonce
and "forPreload" parts of the request.

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
File Source/core/loader/cache/CachedResourceLoader.h (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
Source/core/loader/cache/CachedResourceLoader.h:128: bool
canRequest(CachedResource::Type, const KURL&, const String&, bool forPreload =
false);
I'd probably name this parameter here because it's not obvious what String to
pass.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:338: void addSourceNonce(String&
nonce);
const String&   <-- we generally use const references unless the parameter is an
out parameter.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:346: // Set of nonces that indicate
whitelisted scripts
I'd skip this comment.  CSPSourceList isn't specific to scripts, so this comment
is going to be wrong once we add nonce support to style-src.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:452: const UChar* position;
There's no reason to have this variable declared here.  We should move it down
to line 486 so that you can declare and initialize it at the same time.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:455: const int noncePrefixLen =
strlen(noncePrefix);
It's better if you use a String that's an ASCIILiteral.  Then the compiler will
compute the length for you.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:457: if
(equalIgnoringCase(noncePrefix, begin, noncePrefixLen) && (*(end - 1) == '\''))
{
There's no reason to check the end - 1 condition.  If the ' isn't in the right
place, we want to error out on line 479 rather than try to consume the the
directive another way.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:459: const UChar* nonceBegin =
position = begin + noncePrefixLen;
We generally don't use compound statements like this one.  It's more readable if
you use two lines.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:466: if (nonceBegin <= position) {
There's no way this can fail to be true.  I'd just make this an ASSERT and not
worry about handling the case where it's not true.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:478: position = end;
Position is a local variable.  THere's no reason to do this assignment.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:704: m_nonces.add(String(nonce));
There's no reason to call the String constructor again here.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/DOMSecurityP...
File Source/core/page/DOMSecurityPolicy.cpp (left):

https://codereview.chromium.org/14949017/diff/1/Source/core/page/DOMSecurityP...
Source/core/page/DOMSecurityPolicy.cpp:81: 
You should leave this blank line.

[Mike West, 2013-05-14 08:07:38]

Just a few bits in addition to Adam's comments. Thanks for working on this!

https://codereview.chromium.org/14949017/diff/1/Source/core/dom/ScriptElement...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/dom/ScriptElement...
Source/core/dom/ScriptElement.cpp:296: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy &&
!document->contentSecurityPolicy()->allowInlineScript(nonce, document->url(),
m_startLineNumber)))
Nit: You probably don't need the temp variable here.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:127: static const char scriptNonce[]
= "script-nonce";
You can kill this now, I believe.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:146: || equalIgnoringCase(name,
scriptNonce)
And this.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:457: if
(equalIgnoringCase(noncePrefix, begin, noncePrefixLen) && (*(end - 1) == '\''))
{
Given the length, it might also be reasonable to split this block out into
'parseNonce()', similar to the 'parseHost()', etc. we use in other places in
this function.

[jww, 2013-05-14 20:49:30]

Response to comments. I'll upload the listed fixes.

https://codereview.chromium.org/14949017/diff/1/Source/core/dom/ScriptElement...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/dom/ScriptElement...
Source/core/dom/ScriptElement.cpp:296: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy &&
!document->contentSecurityPolicy()->allowInlineScript(nonce, document->url(),
m_startLineNumber)))
On 2013/05/14 08:07:38, Mike West (chromium) wrote:
> Nit: You probably don't need the temp variable here.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
Source/core/loader/cache/CachedResourceLoader.cpp:382: const String& nonce =
(request.initiatorElement() != NULL) ?
request.initiatorElement()->fastGetAttribute(HTMLNames::nonceAttr) :
AtomicString();
This is tough because there are 3 call sites for canRequest, and one of them has
a ResourceRequest, but not a CachedResourceRequest. A not-quite as elegant
version of this would be to pass in an Element* instead. It won't get rid of the
forPreload argument, but it would move the fastGetAttribute into canRequest,
which I think does make sense to do.

On 2013/05/14 05:58:16, abarth wrote:
> Can we put this in the CachedResourceRequest instead?  Then we can pass the
> CachedResourceRequest directly to canRequest instead of exploding out the
nonce
> and "forPreload" parts of the request.

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
File Source/core/loader/cache/CachedResourceLoader.h (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/loader/cache/Cach...
Source/core/loader/cache/CachedResourceLoader.h:128: bool
canRequest(CachedResource::Type, const KURL&, const String&, bool forPreload =
false);
Assuming the change to passing in an Element (see my reply in
CacheResourceLoader.cpp), I'll change this to be named "initiatorElement".
On 2013/05/14 05:58:16, abarth wrote:
> I'd probably name this parameter here because it's not obvious what String to
> pass.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:127: static const char scriptNonce[]
= "script-nonce";
On 2013/05/14 08:07:38, Mike West (chromium) wrote:
> You can kill this now, I believe.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:146: || equalIgnoringCase(name,
scriptNonce)
On 2013/05/14 08:07:38, Mike West (chromium) wrote:
> And this.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:338: void addSourceNonce(String&
nonce);
On 2013/05/14 05:58:16, abarth wrote:
> const String&   <-- we generally use const references unless the parameter is
an
> out parameter.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:346: // Set of nonces that indicate
whitelisted scripts
On 2013/05/14 05:58:16, abarth wrote:
> I'd skip this comment.  CSPSourceList isn't specific to scripts, so this
comment
> is going to be wrong once we add nonce support to style-src.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:452: const UChar* position;
I declared it up here because I actually use it in the 'if' statement directly
below it. Is that okay, or do you think I should use a new variable instead
within the 'if' statement?

EDIT: I like Mike's suggestion of factoring all this code into its own
parseNonce function, so I believe my question above is now irrelevant.
On 2013/05/14 05:58:16, abarth wrote:
> There's no reason to have this variable declared here.  We should move it down
> to line 486 so that you can declare and initialize it at the same time.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:455: const int noncePrefixLen =
strlen(noncePrefix);
I'm unclear on how to get the length in a compiler-friendly way if I declare an
ASCIILiteral. However, I noticed that there's a common use of
DEFINE_STATIC_LOCAL to make String objects from ASCIILiterals, so I changed it
to use that. Let me know if that's acceptable or unacceptable.
On 2013/05/14 05:58:16, abarth wrote:
> It's better if you use a String that's an ASCIILiteral.  Then the compiler
will
> compute the length for you.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:457: if
(equalIgnoringCase(noncePrefix, begin, noncePrefixLen) && (*(end - 1) == '\''))
{
On 2013/05/14 05:58:16, abarth wrote:
> There's no reason to check the end - 1 condition.  If the ' isn't in the right
> place, we want to error out on line 479 rather than try to consume the the
> directive another way.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:457: if
(equalIgnoringCase(noncePrefix, begin, noncePrefixLen) && (*(end - 1) == '\''))
{
On 2013/05/14 08:07:38, Mike West (chromium) wrote:
> Given the length, it might also be reasonable to split this block out into
> 'parseNonce()', similar to the 'parseHost()', etc. we use in other places in
> this function.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:459: const UChar* nonceBegin =
position = begin + noncePrefixLen;
On 2013/05/14 05:58:16, abarth wrote:
> We generally don't use compound statements like this one.  It's more readable
if
> you use two lines.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:466: if (nonceBegin <= position) {
On 2013/05/14 05:58:16, abarth wrote:
> There's no way this can fail to be true.  I'd just make this an ASSERT and not
> worry about handling the case where it's not true.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:478: position = end;
Whoops. Remnants of an old misunderstanding :-)
On 2013/05/14 05:58:16, abarth wrote:
> Position is a local variable.  THere's no reason to do this assignment.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:704: m_nonces.add(String(nonce));
On 2013/05/14 05:58:16, abarth wrote:
> There's no reason to call the String constructor again here.

Done.

https://codereview.chromium.org/14949017/diff/1/Source/core/page/DOMSecurityP...
File Source/core/page/DOMSecurityPolicy.cpp (left):

https://codereview.chromium.org/14949017/diff/1/Source/core/page/DOMSecurityP...
Source/core/page/DOMSecurityPolicy.cpp:81: 
On 2013/05/14 05:58:16, abarth wrote:
> You should leave this blank line.

Done.

[abarth-chromium, 2013-05-14 21:11:08]

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/Resour...
Source/core/loader/ResourceLoader.cpp:327:
(!m_documentLoader->cachedResourceLoader()->canRequest(m_resource->type(),
request.url(), NULL)) {
These two lines should be merged.

Also, we don't use NULL as a constant.  We use 0 instead.  :)

Why is 0 the right thing to pass here?  Does that mean we'll incorrectly block a
request if there's a redirect?  Specifically, if there's a <script nonce="..."
src="..."> and the we get a redirect, is the script still allowed?  In any case,
please add a test for this case.

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:149: if (requestURL.isValid()
&& canRequest(CachedResource::ImageResource, requestURL, NULL))
NULL -> 0

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:311: const String& nonce =
(initiatorElement != NULL) ?
initiatorElement->fastGetAttribute(HTMLNames::nonceAttr) : AtomicString();
(initiatorElement != NULL)   ->   initiatorElement

No need to compare against 0.  The default boolean operator does that for us.

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.h (right):

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.h:128: bool
canRequest(CachedResource::Type, const KURL&, PassRefPtr<Element>
initiatorElement, bool forPreload = false);
PassRefPtr<Element>   ->   Element*

We only need to use PassRefPtr when we're passing ownership of an object and we
want to save a ref/deref pair.

[jww, 2013-05-14 22:55:30]

Responded to comments from Adam.

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/Resour...
Source/core/loader/ResourceLoader.cpp:327:
(!m_documentLoader->cachedResourceLoader()->canRequest(m_resource->type(),
request.url(), NULL)) {
Can you elaborate on the example you're concerned about? I don't understand the
redirect issue you want me to test.

On 2013/05/14 21:11:08, abarth wrote:
> These two lines should be merged.
> 
> Also, we don't use NULL as a constant.  We use 0 instead.  :)
> 
> Why is 0 the right thing to pass here?  Does that mean we'll incorrectly block
a
> request if there's a redirect?  Specifically, if there's a <script nonce="..."
> src="..."> and the we get a redirect, is the script still allowed?  In any
case,
> please add a test for this case.

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:149: if (requestURL.isValid()
&& canRequest(CachedResource::ImageResource, requestURL, NULL))
On 2013/05/14 21:11:08, abarth wrote:
> NULL -> 0

Done.

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:311: const String& nonce =
(initiatorElement != NULL) ?
initiatorElement->fastGetAttribute(HTMLNames::nonceAttr) : AtomicString();
On 2013/05/14 21:11:08, abarth wrote:
> (initiatorElement != NULL)   ->   initiatorElement
> 
> No need to compare against 0.  The default boolean operator does that for us.

Done.

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.h (right):

https://codereview.chromium.org/14949017/diff/13001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.h:128: bool
canRequest(CachedResource::Type, const KURL&, PassRefPtr<Element>
initiatorElement, bool forPreload = false);
On 2013/05/14 21:11:08, abarth wrote:
> PassRefPtr<Element>   ->   Element*
> 
> We only need to use PassRefPtr when we're passing ownership of an object and
we
> want to save a ref/deref pair.

Done.

[abarth-chromium, 2013-05-14 23:03:08]

Sure.

Content-Security-Policy: script-src 'nonce-foo'

<script nonce="foo" src="bar.php">

where bar.php does an HTTP redirect to qux.js.

[jww, 2013-05-15 00:01:38]

Test written and confirmed that this is a problem. I think I have an idea for a
solution, but your opinion would be greatly appreciated.

I misunderstood what willSendRequest was for, and I thought it would never have
a nonce associated with it. Obviously, not the case. Unfortunately, it appears
that the "initiator element", if there is one, is pretty far back in the layers
of calls. So, the question really is, how to pass that initiator element forward
so that willSendRequest gets it.

We could store it in the ResourceRequest, but it seems odd to me to make the
platform networking code know about Elements. It seems like a more logical place
to put it is in the ResourceLoaderOptions. Does that make sense to you?
On 2013/05/14 23:03:08, abarth wrote:
> Sure.
> 
> Content-Security-Policy: script-src 'nonce-foo'
> 
> <script nonce="foo" src="bar.php">
> 
> where bar.php does an HTTP redirect to qux.js.

[abarth-chromium, 2013-05-15 00:07:37]

We only want to read the nonce off the element once.  If we grab the nonce off
the element again for each redirect, we'll have a strange behavior if the nonce
changes between the time the first load is issued and when the redirect occurs.

Instead, we should probably not use the initiator element at all.  Instead, the
code that prepares the CachedResourceRequest should probably put the nonce in
the CachedResourceRequest.  The trick will be recovering the
CachedResourceRequest from the ResourceRequest when handling the willSendRequest
call.

[abarth-chromium, 2013-05-15 00:08:57]

We can also ask japhet for help.

[jww, 2013-05-15 00:39:46]

On 2013/05/15 00:08:57, abarth wrote:
> We can also ask japhet for help.

Maybe japhet will correct me, but I don't think there's any way to get at the
CachedResourceRequest from the ResourceRequest. I think, though, that we can do
something similar to what you're suggesting in the ResourceLoaderOptions.

In all the cases that lead to willSendRequest, we have an options object that we
pass in. If we get  the nonce name from the CachedResourceRequest and store it
in the options struct, we can pass that nonce value into willSendRequest, which
can pass that value later on to canRequest.

Does that sound reasonable to you? I think it has the properties you suggested,
namely of extracting the nonce only when the call is initially made and not
checking it again later, but it doesn't require access to the
CachedResourceRequest near the willSendRequest call site.

[abarth-chromium, 2013-05-15 01:06:57]

Rather than putting the nonce here, what if we put an enum similar to

    enum ContentSecurityPolicyEnforcement {
        EnforceConnectSrcDirective,
        DoNotEnforceContentSecurityPolicy,
    };

from ThreadableLoaderOptions?  Basically, we'd tell the ResourceLoader to skip
further CSP checks because the request has already passed the necessary CSP
checks.

[do-not-use-jww, 2013-05-15 01:19:13]

I'm not sure we can do that because we can't do the rest of the CSP checks
until we get into willSendRequest anyway, where canRequest is called. And
in order to do that check there, we're back to square 1 and need the nonce
passed down to do the check.
On May 14, 2013 6:06 PM, <abarth@chromium.org> wrote:

> Rather than putting the nonce here, what if we put an enum similar to
>
>     enum ContentSecurityPolicyEnforceme**nt {
>         EnforceConnectSrcDirective,
>         DoNotEnforceContentSecurityPol**icy,
>     };
>
> from ThreadableLoaderOptions?  Basically, we'd tell the ResourceLoader to
> skip
> further CSP checks because the request has already passed the necessary CSP
> checks.
>
>
https://codereview.chromium.**org/14949017/<https://codereview.chromium.org/1...
>

[jww, 2013-05-16 00:44:31]

I just uploaded a patch that includes a test for nonce behavior of redirects in
a script load, as well as a fix for that problem. The approach I took is pretty
similar to what Adam suggested in his last comment. I added a new option in
RequestLoaderOptions that specifies in a nonce check has already been
(successfully) done. Then, at the point that willSendRequest is called, we know
if a nonce check has already passed and we can send this to canRequest.

This is all not quite as elegant as it previously was, so I'm open to any
suggestions on how to improve the feel of the code.

[abarth-chromium, 2013-05-16 00:59:27]

https://codereview.chromium.org/14949017/diff/25001/Source/core/dom/ScriptEle...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:295: bool validNonce =
document->contentSecurityPolicy()->allowNonce(m_element->fastGetAttribute(HTMLNames::nonceAttr));
Maybe isValidNonceForScript?  Different types of resources can have different
nonces.

https://codereview.chromium.org/14949017/diff/25001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:296: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy &&
!document->contentSecurityPolicy()->allowInlineScript(validNonce,
document->url(), m_startLineNumber)))
Is there a reason to call allowInlineScript if the nonce is valid?  It seems
like we could just bypass the call.  It seems related to
shouldBypassMainWorldContentSecurityPolicy in that way.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/Resour...
Source/core/loader/ResourceLoaderOptions.h:65: NonceCheckNotValid
Does this need to be specific to nonces?  Perhaps we should just say
CheckContentSecurityPolicy and DoNotCheckContentSecurityPolicy.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:149: if (requestURL.isValid()
&& canRequest(CachedResource::ImageResource, requestURL, false))
If you use the enum rather than a bool, then call sites like this one are more
readable.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:199: nonceCheck =
NonceCheckValid;
If this code isn't used, we shouldn't add it.  We can always add it later when
we implement that feature.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:278: return initiatorElement
&&
m_document->contentSecurityPolicy()->allowNonce(initiatorElement->fastGetAttribute(HTMLNames::nonceAttr));
This isn't right.  Different types have different nonces, so there isn't a
global way to check whether a nonce is valid.  The code that prepares the
CachedResourceRequest needs to do this check.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:392: if (!canRequest(type,
url, checkNonceFromInitiatorElement(request.initiatorElement().get()),
request.forPreload()))
This needs to be done in type-specific code because the nonces can be
type-specific.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.h (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.h:128: bool
canRequest(CachedResource::Type, const KURL&, bool validNonce, bool forPreload =
false);
Why not pass the enum here instead of the bool?

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/ContentS...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/ContentS...
Source/core/page/ContentSecurityPolicy.cpp:1199: return
checkNonce(operativeDirective(m_scriptSrc.get()), nonce);
Yeah, allowNonce is secretly script-src specific.

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/ContentS...
Source/core/page/ContentSecurityPolicy.cpp:1547: return validNonce ||
isAllowedByAllWithContext<&CSPDirectiveList::allowInlineScript>(m_policies,
contextURL, contextLine, reportingStatus);
Rather than pass this boolean value into this function, the callers should just
skip the call if they've found a valid nonce.

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/DOMSecur...
File Source/core/page/DOMSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/DOMSecur...
Source/core/page/DOMSecurityPolicy.cpp:93: }
I don't understand why this code is needed.  Can we remove it?

[jww, 2013-05-16 20:58:59]

I *think* I've made the changes you've requested, although I have a couple of
questions in response to your comments.

https://codereview.chromium.org/14949017/diff/25001/Source/core/dom/ScriptEle...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:295: bool validNonce =
document->contentSecurityPolicy()->allowNonce(m_element->fastGetAttribute(HTMLNames::nonceAttr));
On 2013/05/16 00:59:27, abarth wrote:
> Maybe isValidNonceForScript?  Different types of resources can have different
> nonces.

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:296: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy &&
!document->contentSecurityPolicy()->allowInlineScript(validNonce,
document->url(), m_startLineNumber)))
Sure, I can do this. I guess my thought was to keep as much contained within the
ContentSecurityPolicy class itself, but good point that
shouldBypassMainWorldContentSecurityPolicy already does this sort of thing.
On 2013/05/16 00:59:27, abarth wrote:
> Is there a reason to call allowInlineScript if the nonce is valid?  It seems
> like we could just bypass the call.  It seems related to
> shouldBypassMainWorldContentSecurityPolicy in that way.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/Resour...
Source/core/loader/ResourceLoaderOptions.h:65: NonceCheckNotValid
On 2013/05/16 00:59:27, abarth wrote:
> Does this need to be specific to nonces?  Perhaps we should just say
> CheckContentSecurityPolicy and DoNotCheckContentSecurityPolicy.

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:149: if (requestURL.isValid()
&& canRequest(CachedResource::ImageResource, requestURL, false))
On 2013/05/16 00:59:27, abarth wrote:
> If you use the enum rather than a bool, then call sites like this one are more
> readable.

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:199: nonceCheck =
NonceCheckValid;
On 2013/05/16 00:59:27, abarth wrote:
> If this code isn't used, we shouldn't add it.  We can always add it later when
> we implement that feature.

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:278: return initiatorElement
&&
m_document->contentSecurityPolicy()->allowNonce(initiatorElement->fastGetAttribute(HTMLNames::nonceAttr));
Okay, I think I've basically factored all of the checks into places where
CachedResourceRequests are created. However, I did have a confusion:

In HTMLResourcePreloader.cpp, there's a resourceRequest function that makes a
cachedResourceRequest. However, there doesn't seem to be an initiator element
(only an initiator name), therefore we can't extract a nonce to check. Thus, I'm
assuming that in this case, we will always assume no nonce. Is that right?

Similarly, it's unclear to me what the type of the documents being loaded in
DocumentThreadableLoader are. I suspect that it is HTML documents, not scripts,
but I haven't been able to figure it out.

On 2013/05/16 00:59:27, abarth wrote:
> This isn't right.  Different types have different nonces, so there isn't a
> global way to check whether a nonce is valid.  The code that prepares the
> CachedResourceRequest needs to do this check.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:392: if (!canRequest(type,
url, checkNonceFromInitiatorElement(request.initiatorElement().get()),
request.forPreload()))
On 2013/05/16 00:59:27, abarth wrote:
> This needs to be done in type-specific code because the nonces can be
> type-specific.

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.h (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.h:128: bool
canRequest(CachedResource::Type, const KURL&, bool validNonce, bool forPreload =
false);
On 2013/05/16 00:59:27, abarth wrote:
> Why not pass the enum here instead of the bool?

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/ContentS...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/ContentS...
Source/core/page/ContentSecurityPolicy.cpp:1199: return
checkNonce(operativeDirective(m_scriptSrc.get()), nonce);
I've renamed it to "allowScriptNonce", which should decrypt it, I think.
On 2013/05/16 00:59:27, abarth wrote:
> Yeah, allowNonce is secretly script-src specific.

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/ContentS...
Source/core/page/ContentSecurityPolicy.cpp:1547: return validNonce ||
isAllowedByAllWithContext<&CSPDirectiveList::allowInlineScript>(m_policies,
contextURL, contextLine, reportingStatus);
On 2013/05/16 00:59:27, abarth wrote:
> Rather than pass this boolean value into this function, the callers should
just
> skip the call if they've found a valid nonce.

Done.

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/DOMSecur...
File Source/core/page/DOMSecurityPolicy.cpp (right):

https://codereview.chromium.org/14949017/diff/25001/Source/core/page/DOMSecur...
Source/core/page/DOMSecurityPolicy.cpp:93: }
On 2013/05/16 00:59:27, abarth wrote:
> I don't understand why this code is needed.  Can we remove it?

Done.

[abarth-chromium, 2013-05-16 21:09:16]

LGTM modulo some style nits.  Thanks for working on this CL!

https://codereview.chromium.org/14949017/diff/40001/Source/core/dom/ScriptEle...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/40001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:271: bool isValidScriptNonce =
m_element->document()->contentSecurityPolicy()->allowScriptNonce(request.initiatorElement()->fastGetAttribute(HTMLNames::nonceAttr));
There's no reason to use the initiator element.  We can just use m_element. 
We're about to remove initiatorElement.

https://codereview.chromium.org/14949017/diff/40001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:300: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy && !(isValidScriptNonce ||
document->contentSecurityPolicy()->allowInlineScript(document->url(),
m_startLineNumber))))
We should treat shouldBypassMainWorldContentSecurityPolicy and
isValidScriptNonce the same typographically since they're treated the same
operationally.

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/Resour...
Source/core/loader/ResourceLoaderOptions.h:105: ContentSecurityPolicyCheck
cspCheck;
cspCheck -> contentSecurityPolicyOption

We prefer to use complete words in variable names.  Usually we'd call this sort
of thing a "policy", but that doesn't read so well because "content security
policy" ends with the word policy.  "Check" doesn't mean anything, so "option"
is probably the next best option.

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:276: bool
doNotCheckContentSecurityPolicy = contentSecurityPolicyCheck ==
DoNotCheckContentSecurityPolicy;
We should just fold this value into shouldBypassMainWorldContentSecurityPolicy

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:550: 
This change seems spurious.

[jww, 2013-05-16 21:37:46]

https://codereview.chromium.org/14949017/diff/40001/Source/core/dom/ScriptEle...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/40001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:271: bool isValidScriptNonce =
m_element->document()->contentSecurityPolicy()->allowScriptNonce(request.initiatorElement()->fastGetAttribute(HTMLNames::nonceAttr));
On 2013/05/16 21:09:16, abarth wrote:
> There's no reason to use the initiator element.  We can just use m_element. 
> We're about to remove initiatorElement.

Done.

https://codereview.chromium.org/14949017/diff/40001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:300: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy && !(isValidScriptNonce ||
document->contentSecurityPolicy()->allowInlineScript(document->url(),
m_startLineNumber))))
I'm sure what you mean here. I *think* what you're saying is merge the
isValidScriptNonce check into the shouldBypassMainWorldContentSecurityPolicy
value, similar to what you suggest in a later comment. This is what I put into
the patch, so let me know if I misunderstood.
On 2013/05/16 21:09:16, abarth wrote:
> We should treat shouldBypassMainWorldContentSecurityPolicy and
> isValidScriptNonce the same typographically since they're treated the same
> operationally.

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/Resour...
Source/core/loader/ResourceLoaderOptions.h:105: ContentSecurityPolicyCheck
cspCheck;
On 2013/05/16 21:09:16, abarth wrote:
> cspCheck -> contentSecurityPolicyOption
> 
> We prefer to use complete words in variable names.  Usually we'd call this
sort
> of thing a "policy", but that doesn't read so well because "content security
> policy" ends with the word policy.  "Check" doesn't mean anything, so "option"
> is probably the next best option.

Done.

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:276: bool
doNotCheckContentSecurityPolicy = contentSecurityPolicyCheck ==
DoNotCheckContentSecurityPolicy;
On 2013/05/16 21:09:16, abarth wrote:
> We should just fold this value into shouldBypassMainWorldContentSecurityPolicy

Done.

https://codereview.chromium.org/14949017/diff/40001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:550: 
On 2013/05/16 21:09:16, abarth wrote:
> This change seems spurious.

Done.

[abarth-chromium, 2013-05-16 22:01:20]

https://codereview.chromium.org/14949017/diff/48001/Source/core/dom/ScriptEle...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14949017/diff/48001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:299: if (!m_isExternalScript &&
(!shouldBypassMainWorldContentSecurityPolicy &&
!document->contentSecurityPolicy()->allowInlineScript(document->url(),
m_startLineNumber)))
I actually just meant

!shouldBypassMainWorldContentSecurityPolicy && !isValidNonce && !document...

but this is fine too.

Thanks!!

[commit-bot: I haz the power, 2013-05-16 22:01:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/14949017/48001

[commit-bot: I haz the power, 2013-05-16 22:43:07]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[abarth-chromium, 2013-05-16 22:44:25]

Those errors are in the tree, not with your CL.  The other boxes are green.

[commit-bot: I haz the power, 2013-05-16 22:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/14949017/48001

[commit-bot: I haz the power, 2013-05-16 23:22:27]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-16 23:24:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/14949017/48001

[commit-bot: I haz the power, 2013-05-16 23:24:42]

Change committed as 150541