CSP 1.1: Implement the 'frame-ancestors' directive.

CSP 1.1: Implement the 'frame-ancestors' directive.

As defined at [1]. This patch will have no web-visible impact, as the directive
remains trapped behind the runtime flag that's governing all CSP 1.1 hotness.

[1]: http://w3c.github.io/webappsec/specs/content-security-policy/csp-specification.dev.html#frame-ancestors
BUG=129139, 335489
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165629

[Mike West, 2014-01-17 14:15:39]

Hello Nate, Adam, and Jochen!

This patch implements the Content Security Policy version of X-Frame-Options. It
obviously needs more tests, but I'd like feedback on the mechanism now before I
get too much further.

I'd _like_ to have done this in DocumentLoader, next to the X-Frame-Options
header parsing. We don't have a Document at that point, however, and therefore
no Content Security Policy.

Is it reasonable to put this into FrameLoader? If not, do you have suggestions
for alternate points at which I could hook into the loader framework?

Thanks!

-mike

[abarth-chromium, 2014-01-18 08:48:25]

Seems reasonable, but we should have japhet take a look.

[Nate Chapin, 2014-01-21 18:24:22]

LGTM with nits/questions.

I only skimmed the ContentSecurityPolicy.cpp changes, since both abarth and
jochen are better qualified to review those.

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:365: // load event and stopAllLoaders can
detach the Frame, so protect it.
The contents of this if() should probably be a helper function?

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:377: stopAllLoaders();
If a navigation was triggered during the load event, this will cancel it. Is
that desirable?

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:378: clear();
Do you really need clear() here? It was just called before we entered
didBeginDocument().

[Nate Chapin, 2014-01-21 18:27:46]

On 2014/01/17 14:15:39, Mike West wrote:
> Hello Nate, Adam, and Jochen!
> 
> This patch implements the Content Security Policy version of X-Frame-Options.
It
> obviously needs more tests, but I'd like feedback on the mechanism now before
I
> get too much further.
> 
> I'd _like_ to have done this in DocumentLoader, next to the X-Frame-Options
> header parsing. We don't have a Document at that point, however, and therefore
> no Content Security Policy.
> 
> Is it reasonable to put this into FrameLoader? If not, do you have suggestions
> for alternate points at which I could hook into the loader framework?
> 
> Thanks!
> 
> -mike

You could hook DocumentLoader::dataReceived, but doing that misses the
javascript url and empty document cases. I don't know if that's ok.

DocumentLoader::commitIfReady could also work, but that's so small and simple
and seems wrong to add this much code to it :)

[Mike West, 2014-01-22 10:08:46]

Thanks for taking a look japhet.

I've added a bunch of tests, and I think this is ready to go. Jochen, would you
mind sanity-checking the CSP bits?

-mike

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:365: // load event and stopAllLoaders can
detach the Frame, so protect it.
On 2014/01/21 18:24:22, Nate Chapin wrote:
> The contents of this if() should probably be a helper function?

Done.

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:377: stopAllLoaders();
On 2014/01/21 18:24:22, Nate Chapin wrote:
> If a navigation was triggered during the load event, this will cancel it. Is
> that desirable?

Hrm. Probably not. I'll reverse the order.

https://codereview.chromium.org/91353002/diff/70001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:378: clear();
On 2014/01/21 18:24:22, Nate Chapin wrote:
> Do you really need clear() here? It was just called before we entered
> didBeginDocument().

Done.

[Nate Chapin, 2014-01-22 17:33:58]

Thanks! FrameLoader bits LGTM

[commit-bot: I haz the power, 2014-01-23 09:56:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/91353002/190001

[commit-bot: I haz the power, 2014-01-23 11:17:38]

Change committed as 165629