CSP: 'frame-ancestors' should override 'x-frame-options'.

CSP: 'frame-ancestors' should override 'x-frame-options'.

As specified in [1], the 'frame-ancestors' CSP directive should take
control of the access checks when loading a document. In particular,
the 'x-frame-options' header should be ignored if a 'frame-ancestors'
directive is present and enforced.

[1]: https://w3c.github.io/webappsec/specs/content-security-policy/#frame-ancestors-and-frame-options

BUG=510423
R=estark@chromium.org

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

[Mike West, 2015-09-08 08:23:11]

Emily, you like reviewing patches, right? :)

-mike

[estark, 2015-09-08 18:15:47]

LGTM with one mild confusion inline

https://codereview.chromium.org/1326823003/diff/1/LayoutTests/http/tests/secu...
File
LayoutTests/http/tests/security/contentSecurityPolicy/resources/frame-ancestors-and-x-frame-options.pl
(right):

https://codereview.chromium.org/1326823003/diff/1/LayoutTests/http/tests/secu...
LayoutTests/http/tests/security/contentSecurityPolicy/resources/frame-ancestors-and-x-frame-options.pl:9:
print "X-Frame-Options: " . $cgi->param("xfo") . "\n\n";
Confusion: when $cgi->param("xfo") is empty, how does this not hit the
ASSERT_NOT_REACHED here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Won't an empty X-Frame-Options value cause parseXFrameOptionsDisposition()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
to return XFrameOptionsNone which will hit the default case of the switch
statement in shouldInterruptLoadForXFrameOptions()?

[estark, 2015-09-08 18:18:21]

https://codereview.chromium.org/1326823003/diff/1/LayoutTests/http/tests/secu...
File
LayoutTests/http/tests/security/contentSecurityPolicy/resources/frame-ancestors-and-x-frame-options.pl
(right):

https://codereview.chromium.org/1326823003/diff/1/LayoutTests/http/tests/secu...
LayoutTests/http/tests/security/contentSecurityPolicy/resources/frame-ancestors-and-x-frame-options.pl:9:
print "X-Frame-Options: " . $cgi->param("xfo") . "\n\n";
On 2015/09/08 18:15:47, estark wrote:
> Confusion: when $cgi->param("xfo") is empty, how does this not hit the
> ASSERT_NOT_REACHED here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Won't an empty X-Frame-Options value cause parseXFrameOptionsDisposition()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> to return XFrameOptionsNone which will hit the default case of the switch
> statement in shouldInterruptLoadForXFrameOptions()?

Oh wait, duh, it will only hit the ASSERT_NOT_REACHED if frame-ancestors is not
enforced, so I guess this is fine unless the served CSP doesn't parse or
something like that. So, confusion resolved, sorry for the noise. :)

[Mike West, 2015-09-09 08:12:27]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-09-09 08:12:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326823003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326823003/1

[commit-bot: I haz the power, 2015-09-09 08:16:56]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201959