Adding Embedding-CSP HTTP header

Adding and sending Embedding-CSP HTTP header

Also, setting requiredCSP on FrameLoader to ensure
that we record the CSP value we used when sending the FrameLoadRequest

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

[amalika, 2016-09-27 09:25:11]

Description was changed from

==========
Adding Embedder-CSP HTTP header

BUG=
==========

to

==========
Adding Embedding-CSP HTTP header

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

[amalika, 2016-09-27 09:25:11]

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

[amalika, 2016-09-27 10:05:57]

Patchset #4 (id:60001) has been deleted

[amalika, 2016-09-27 10:06:09]

Patchset #3 (id:40001) has been deleted

[amalika, 2016-09-27 11:13:08]

Patchset #4 (id:100001) has been deleted

[amalika, 2016-09-27 11:13:31]

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

[commit-bot: I haz the power, 2016-09-27 11:13:44]

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

[amalika, 2016-09-27 11:16:15]

The CQ bit was unchecked by amalika@google.com

[amalika, 2016-09-27 11:19:21]

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

[commit-bot: I haz the power, 2016-09-27 11:19:28]

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

[amalika, 2016-09-27 11:22:59]

The CQ bit was unchecked by amalika@google.com

[Mike West, 2016-09-27 12:14:50]

This is a good start! I think we're going to need a slightly different approach,
though. I've added some notes below.

https://codereview.chromium.org/2372563002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2372563002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:334: }
I think this is going to end up being the wrong place to put this logic. Despite
the name, `loadOrRedirectSubframe` doesn't actually appear to be called during
redirects (which suggests another set of tests to add :) ).

I think you'll have more luck adding the header in `FrameLoader`, which ends up
being responsible for creating a document and stuffing things into it.
`upgradeInsecureRequest()` is ~similar, and called in `FrameLoader::startLoad`.
It might be worth looking into that area of the code to see what you'd need to
do to move this logic around a bit to ensure that it covers the cases you care
about.

I think you'll need to replicate the `csp` value up to `FrameOwner` (that is,
both `HTMLFrameOwnerElement` and `RemoteFrameOwner`) in order to access it in
the loader. Look at things like `WebFrame::setFrameOwnerProperties` to see how
that works, as it's a bit complicated. Basically, the call to
`frameOwnerPropertiesChanged()` in the previous patch needs to grab the new CSP
value, replicate it up to the browser process from the renderer, and then push
it back down to the renderers it creates during navigation. It's probably best
to split that out into a separate patch.

[Nico, 2016-09-27 15:47:08]

Please don't pick all the trybots you can find. This patch ran on the bot we use
to build and upload new compilers
(https://build.chromium.org/p/tryserver.chromium.win/builders/win_upload_clang...).
Just the default set from `git cl try` should usually be a good selection of
bots. Don't add bots where you don't know what they do.

(Nothing bad happened by this patch running on the compiler upload bot, but it's
still not a good practice.)

[blink-reviews, 2016-09-27 17:45:43]

I am sorry about this. That was indeed a bad idea to manually choose some
try bots.
Thank you for the email!

On Tue, Sep 27, 2016 at 5:47 PM, <thakis@chromium.org> wrote:

> Please don't pick all the trybots you can find. This patch ran on the bot
> we use
> to build and upload new compilers
> (https://build.chromium.org/p/tryserver.chromium.win/
> builders/win_upload_clang/builds/112).
> Just the default set from `git cl try` should usually be a good selection
> of
> bots. Don't add bots where you don't know what they do.
>
> (Nothing bad happened by this patch running on the compiler upload bot,
> but it's
> still not a good practice.)
>
> https://codereview.chromium.org/2372563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-27 17:45:43]

I am sorry about this. That was indeed a bad idea to manually choose some
try bots.
Thank you for the email!

On Tue, Sep 27, 2016 at 5:47 PM, <thakis@chromium.org> wrote:

> Please don't pick all the trybots you can find. This patch ran on the bot
> we use
> to build and upload new compilers
> (https://build.chromium.org/p/tryserver.chromium.win/
> builders/win_upload_clang/builds/112).
> Just the default set from `git cl try` should usually be a good selection
> of
> bots. Don't add bots where you don't know what they do.
>
> (Nothing bad happened by this patch running on the compiler upload bot,
> but it's
> still not a good practice.)
>
> https://codereview.chromium.org/2372563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[amalika, 2016-09-28 12:53:21]

Preliminary version, rebased on Issue 2378643002 Patch 20001:
It currently lacks validation of csp value.

[amalika, 2016-09-29 08:44:11]

Description was changed from

==========
Adding Embedding-CSP HTTP header

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

to

==========
Adding and sending Embedding-CSP HTTP header

Also, setting requiredCSP on LocalDOMWindow to ensure
that we record the CSP value we used when sending the FrameLoadRequest

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

[amalika, 2016-09-29 12:57:51]

Decided that the validation of the csp value would be done in a separate patch

[amalika, 2016-09-29 13:01:14]

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php
(right):

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php:4:
header("Embedding-CSP: " + $value);
From my understanding header() function sets a response header?
I thought we could use this file later to reflect Content-Security-Policy
header. 
So it shouldn't say "embedding-csp: " + $value

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:988: // Record the latest
requiredCSP value that was used when sending this request.
This seems to be necessary to make sure we correctly check against the CSP
response even if csp attribute has changed.

[Mike West, 2016-09-29 13:33:13]

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php
(right):

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php:4:
header("Embedding-CSP: " + $value);
On 2016/09/29 at 13:01:14, amalika wrote:
> From my understanding header() function sets a response header?
> I thought we could use this file later to reflect Content-Security-Policy
header. 
> So it shouldn't say "embedding-csp: " + $value

Sounds good (though it would need to say `Content-Security-Policy: ${value}`,
right?)!

That said, you're not actually exercising this functionality. Perhaps
introducing this file would make more sense when you actually have a blocking
implementation in place?

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:124: for (size_t i =
0; i < value.length(); ++i) {
You can replace this loop with `value.containsOnlyASCII()` (defined in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/WTFSt...,
but you can cast an `AtomicString` to `WTF::String` without copying, so it's
fairly cheap).

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:988: // Record the latest
requiredCSP value that was used when sending this request.
On 2016/09/29 at 13:01:14, amalika wrote:
> This seems to be necessary to make sure we correctly check against the CSP
response even if csp attribute has changed.

I agree that we need to bind the required CSP so that it's locked in at
load-time, but I don't think `Window` is the right place to put it. In
particular, we certainly don't need to replicate the value out to remote
anythings, as it's a local cache for this process.

Where are you planning on doing the blocking checks? I'd guess we'd end up doing
those somewhere near
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do...
in the short term (and somewhere in `//content` in the long term, similar to
what we're doing for `X-Frame-Options`). We have access to the `FrameLoader`
there, so why not just cache the value here? That's vaguely similar to what we
do for `Upgrade-Insecure-Requests` (but we just reach up to the parent for that
getter, because we don't have the same problem of changing values).

https://codereview.chromium.org/2372563002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:990:
request.resourceRequest().setHTTPHeaderField(HTTPNames::Embedding_CSP,
requiredCSP);
I think I'd prefer to push this logic out to a separate function. Perhaps we
could combine this and
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
from `FrameLoader::upgradeInsecureRequest` into something like
`FrameLoader::addOutgoingSecurityHeaders` (or whatever)?

Note also that you'll need to deduplicate the header in case of redirects (as
you'll hit `load` again). The code in `upgradeInsecureRequest` is a good model.

[amalika, 2016-09-30 11:02:31]

In this patch requiredCSP is moved from LocalDOMWindow to FrameLoader. A few
concerns:

- AddingOutgoingSecurityHeaders should be coupled with getting the latest CSP
value (which in this patch has to be called right before). Now we have two
places that call the function so we have to record the latest required CSP in
both places. The concern is if in the future, there will become more dependents
on this addingOutgoingSecurityHeaders that would forget to update the
requiredCSP. 

- There seem to be various places that cancel the request. However, is it
possible that multiple requests were sent and in between those CSP value has
changed? If the latest request gets cancelled, it is possible we would be
checking against wrong CSP value for the response on the first request?

Validation of policies as well as actual check of the response against
requiredCSP is to come in next patches.

[Mike West, 2016-09-30 13:11:49]

https://codereview.chromium.org/2372563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2372563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:125:
document().addConsoleMessage(ConsoleMessage::create(OtherMessageSource,
ErrorMessageLevel, "'csp' attribute contains non-ASCII characters: " + value));
This has the side-effect of putting `m_csp` and the attribute's value out of
sync. I'd suggest we clear out `m_csp` if the content attribute is invalid (and
update the spec accordingly).

https://codereview.chromium.org/2372563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2372563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:704:
frame()->loader().addOutgoingSecurityHeaders(resourceRequest, m_document);
This seems strange. I don't know why we call into upgradeInsecureRequest from
here and frameloader. Let me dig into that a moment.

https://codereview.chromium.org/2372563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1630: void
FrameLoader::addOutgoingSecurityHeaders(ResourceRequest& resourceRequest,
Document* document) const
Hrm. I think it makes more sense to pull out the header-setting bits into a new
function. The upgrading bits from 1646 on don't have anything to do with setting
security headers.

[amalika, 2016-10-04 08:31:59]

The CQ bit was checked by amalika@google.com

[amalika, 2016-10-04 08:31:59]

I separated into two functions but not sure that's what you wanted because
addOutgoingSecurityHeaders still calls on upgradeInsecureRequest if the request
has not been upgraded prior. 

Also, Pending why FrameFetchContext calls on upgradeInsecureRequest.

[commit-bot: I haz the power, 2016-10-04 08:32:08]

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

[commit-bot: I haz the power, 2016-10-04 08:32:09]

This CL has an open dependency (Issue 2378643002 Patch 140001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Mike West, 2016-10-06 08:00:51]

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/embedding_csp-header.html
(right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/embedding_csp-header.html:79:
i.src = '../../resources/redir.php?url=' + redirect_url;
Since you do this a few times, it's probably worth pulling out into a helper
function (`function generateRedirect(url) { return
'/security/resources/redir.php?url=' + url; }`).

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php
(right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php:8:
response['embedding_csp'] = "<?php echo $embedding_csp_header; ?>";
Nit: It's confusing to use a string "null" here when JavaScript has a `null`
value that's probably more like what you want. Perhaps you could change this to
output a string when there's a header, and the bareword `null` when there's not.
Then you'd `assert_equals(null, ...)` in the tests to verify that no header was
sent (as opposed to a header whose value was the string "null").

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:128: }
Can you add a TODO here to make the validation a bit more robust? We need to
take care of things like `\r` and `\n` at least.

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:187:
fetchContext->addOutgoingSecurityHeadersAndUpgradeRequest(fetchRequest.mutableResourceRequest());
Please add tests in this file for the header you're adding. You could do it in
the same class (possibly renaming it to something like
`FrameFetchContextModifyRequestTest` if you accept the naming suggestion in the
other comment), just adding something like
`TEST_F(FrameFetchContextModifyRequestTest, SendExpectedCSPHeader)`, similar to
`SendHTTPSHeader`.

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:197: void
expectHTTPSHeader(const char* input, WebURLRequest::FrameType frameType, bool
shouldPrefer)
Nit: While you're here, could you rename this to
`expectUpgradeInsecureRequestHeader`, and the associated test to
`SendUpgradeInsecureRequestHeader`? Doing that in a separate patch would be
fine, if you don't want to clutter this diff.

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:177: ,
m_requiredCSP(nullAtom)
Nit: I think this is the default, isn't it? You shouldn't need to initialize
this.

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1633:
resourceRequest.setHTTPHeaderField(HTTPNames::Embedding_CSP, requiredCSP());
Can you add a DCHECK here that the 'requiredCSP()' value is sane?

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.h:151: void
addOutgoingSecurityHeadersAndUpgradeRequest(ResourceRequest&, Document*) const;
How about `modifyRequestForCSP`?

[amalika, 2016-10-06 13:06:02]

- Renaming older tests in FrameFetchContext
- Adding a test for EmbeddingCSP header
- Adding a comment to HTMLIFrameElement that more validation needs to be done

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php
(right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php:8:
response['embedding_csp'] = "<?php echo $embedding_csp_header; ?>";
On 2016/10/06 at 08:00:50, Mike West wrote:
> Nit: It's confusing to use a string "null" here when JavaScript has a `null`
value that's probably more like what you want. Perhaps you could change this to
output a string when there's a header, and the bareword `null` when there's not.
Then you'd `assert_equals(null, ...)` in the tests to verify that no header was
sent (as opposed to a header whose value was the string "null").

I just didnt know how to convert php NULL value to be 'null' in javascript
without ugly php if statements inside <script> since the value is echoed inside
a string. I changed it to NULL in php but now response['embedding_csp'] is an
empty string.

[Mike West, 2016-10-06 13:30:00]

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

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

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

[Mike West, 2016-10-06 13:30:21]

Looking pretty good!

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php
(right):

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/get-embedding-csp-header-and-respond.php:8:
response['embedding_csp'] = "<?php echo $embedding_csp_header; ?>";
How about something like the following in both files:

```
response['embedding_csp'] = <?php echo is_null($embedding_csp_header) ?
"\"${embedding_csp_header}\"" : "null"; ?>
```

Also, you'll want to call `addslashes()` at some point (maybe when doing the
assignment?) to deal with policies that would otherwise break out of the string.

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:167: class
FrameFetchContextModifyRequestTest : public FrameFetchContextTest {
I would have expected you to need to set the runtime enabled flag for this
feature for these tests to pass... Do they pass? If so, what am I missing? :)

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:255: if
(frameType == WebURLRequest::FrameTypeNested) {
Nit: We generally try to do the quick exit first. So, perhaps reverse this into:

```
if (frameType != WebURLRequest::FrameTypeNested) {
  document->frame()->setOwner(nullptr);
  return nullAtom;
}

iframe->setAttribute(...);
...
return ...;
```

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:261:
document->frame()->setOwner(0);
Nit: s/0/nullptr/

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:414: // the
tests both before and after providing a document to the context.
I don't think this comment is accurate for this header.
`Upgrade-Insecure-Requests: 1` is sent for all navigational requests, top-level
or nested. This new header is sent only for navigational requests in a nested
context. I agree that the behavior should be the same regardless of whether or
not a `Document` is present, but I'm not sure we can ever do a nested load
without one. :)

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:418:
expectSetEmbeddingCSPRequestHeader(test.toRequest, test.frameType,
Using the return value to set the expectation is somewhat more magical than I
think is comprehensible. :) Would you consider something like:

```
expectSetEmbeddingCSPRequestHeader(test.toRequest, test.frameType,
    test.frameType == WebURLRequest::FrameTypeNested ? requiredCSP : nullAtom);

```

?

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1828:
!requiredCSP().isEmpty())
Nit: {}.

[commit-bot: I haz the power, 2016-10-06 15:53:32]

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

[commit-bot: I haz the power, 2016-10-06 15:53:33]

Dry run: This issue passed the CQ dry run.

[amalika, 2016-10-06 18:54:57]

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1633:
resourceRequest.setHTTPHeaderField(HTTPNames::Embedding_CSP, requiredCSP());
On 2016/10/06 at 08:00:51, Mike West wrote:
> Can you add a DCHECK here that the 'requiredCSP()' value is sane?

Not sure if that's what you wanted?

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2372563002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:167: class
FrameFetchContextModifyRequestTest : public FrameFetchContextTest {
On 2016/10/06 at 13:30:21, Mike West wrote:
> I would have expected you to need to set the runtime enabled flag for this
feature for these tests to pass... Do they pass? If so, what am I missing? :)

From my understanding this might be because site_per_proces_browsertests
explicitly overrides SetUpCommandLine in 
https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser...
? 
But not too sure. I tried looking for confirmation but couldnt find it in 15
minutes or so I stopped... But I double checked with printf statement and the
flag is set to true in FrameFetchContext test. (Also in that file
setCorsRFC1918Enabled flag is explicitly set to false first so I am guessing all
RuntimeEnabledFeatures are set by default for tests?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...)

[Mike West, 2016-10-07 12:08:47]

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

[commit-bot: I haz the power, 2016-10-07 12:08:54]

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

[Mike West, 2016-10-07 12:11:59]

LGTM if you change the DCHECK to something that verifies that the value doesn't
contain non-ascii characters (with a TODO about strengthening that check, in
line with the other TODO when parsing the value).

Thanks!

https://codereview.chromium.org/2372563002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2372563002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1828:
DCHECK(requiredCSP());
I was going for something more like `DCHECK(requiredCSP() does not contain crazy
characters.)`.

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

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

[commit-bot: I haz the power, 2016-10-07 12:19:51]

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

[amalika, 2016-10-10 08:39:35]

Description was changed from

==========
Adding and sending Embedding-CSP HTTP header

Also, setting requiredCSP on LocalDOMWindow to ensure
that we record the CSP value we used when sending the FrameLoadRequest

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

to

==========
Adding and sending Embedding-CSP HTTP header

Also, setting requiredCSP on FrameLoader to ensure
that we record the CSP value we used when sending the FrameLoadRequest

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

[amalika, 2016-10-10 08:40:50]

The CQ bit was checked by amalika@google.com

[amalika, 2016-10-10 08:40:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2372563002/#ps300001
(title: "ASCII DCHECK and a comment")

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

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

[commit-bot: I haz the power, 2016-10-10 09:54:46]

Committed patchset #13 (id:300001)

[commit-bot: I haz the power, 2016-10-10 09:57:02]

Description was changed from

==========
Adding and sending Embedding-CSP HTTP header

Also, setting requiredCSP on FrameLoader to ensure
that we record the CSP value we used when sending the FrameLoadRequest

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

to

==========
Adding and sending Embedding-CSP HTTP header

Also, setting requiredCSP on FrameLoader to ensure
that we record the CSP value we used when sending the FrameLoadRequest

BUG=647588

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

[commit-bot: I haz the power, 2016-10-10 09:57:04]

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