WIP: Move 'X-Frame-Options' checking to the browser.

WIP: Move 'X-Frame-Options' checking to the browser.

First pass at a structure that folks can help me evaluate.

BUG=555418

[Mike West, 2015-12-17 13:04:44]

mkwst@chromium.org changed reviewers:
+ clamy@chromium.org

[Mike West, 2015-12-17 13:09:15]

clamy@: Would you mind taking a look at this from a structural perspective? If
you're ok with it from a high level, I'll start polishing and adding tests. If
not, I'd love suggestions for improvement. :)

https://codereview.chromium.org/1530393003/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1530393003/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:2740:
scoped_ptr<content::NavigationThrottle> xfo_throttle =
I think this probably needs to happen in //content, as I don't want to make
every embedder implement it themselves. I don't know whether it's a good idea to
do it directly in ContentBrowserClient, though...

https://codereview.chromium.org/1530393003/diff/1/chrome/browser/security/xfo...
File chrome/browser/security/xfo_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/1/chrome/browser/security/xfo...
chrome/browser/security/xfo_throttle.h:22: class XFOThrottle : public
NavigationThrottle {
As above, this should move to //content somewhere.

https://codereview.chromium.org/1530393003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1530393003/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:300:
ReadyToCommitNavigation(render_frame_host, response_headers);
This feels a bit hacky. I'd appreciate suggestions if you agree.

https://codereview.chromium.org/1530393003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (left):

https://codereview.chromium.org/1530393003/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:109: const
net::HttpResponseHeaders* GetResponseHeaders();
This moved up to 'NavigationHandle'.

[Mike West, 2015-12-17 13:31:29]

https://codereview.chromium.org/1530393003/diff/1/chrome/browser/security/xfo...
File chrome/browser/security/xfo_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/1/chrome/browser/security/xfo...
chrome/browser/security/xfo_throttle.h:22: class XFOThrottle : public
NavigationThrottle {
On 2015/12/17 at 13:09:15, Mike West wrote:
> As above, this should move to //content somewhere.

Or a component.

[Mike West, 2015-12-17 13:37:55]

https://codereview.chromium.org/1530393003/diff/20001/chrome/browser/security...
File chrome/browser/security/xfo_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/20001/chrome/browser/security...
chrome/browser/security/xfo_throttle.cc:39: 
Here, I need to grab the frame tree in order to check the ancestors against the
XFO rule. I can do this by exposing the `frame_tree_node_` on NavigationHandle,
but there's probably a more elegant mechanism... Halp? :)

[Mike West, 2015-12-17 13:39:49]

Emily, Jochen, and Charlie, FYI.

[nasko, 2015-12-17 15:02:04]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2015-12-17 15:02:04]

Haven't looked at the code, just the files, but have a fundamental question -
why is this in chrome/? The spirit of content/ is to implement a base for
multiprocess browser and everything that is part of the web platform should live
there. XFO is part of the web platform, so I don't see a reason why this would
be in chrome/.

[Mike West, 2015-12-17 16:51:06]

On 2015/12/17 at 15:02:04, nasko wrote:
> Haven't looked at the code, just the files, but have a fundamental question -
why is this in chrome/? The spirit of content/ is to implement a base for
multiprocess browser and everything that is part of the web platform should live
there. XFO is part of the web platform, so I don't see a reason why this would
be in chrome/.

I agree. I mentioned that in https://codereview.chromium.org/1530393003#msg2.
This was just the fastest thing I could implement to give clamy@ something to
evaluate so I'm not going in the completely wrong direction. :)

You'll also note that I'm not actually checking the frame bits and pieces yet,
so. Yeah. WIP.

[kinuko, 2015-12-18 04:25:52]

Driving-by, just noticed this, interesting!  Haven't really looked into the code
either but +1 having this in somewhere more common for web platform.

[clamy, 2015-12-21 10:13:29]

Thanks! The general direction seems right, with the caveat that the xfo checks
should really be in content. I think content/browser/frame_host/ is a good place
to put them (since it already handles security checks on the frame tree &
navigation). This will also make the code easier, and require to expose less
things in the content/ public API.

https://codereview.chromium.org/1530393003/diff/20001/chrome/browser/security...
File chrome/browser/security/xfo_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/20001/chrome/browser/security...
chrome/browser/security/xfo_throttle.cc:39: 
On 2015/12/17 13:37:54, Mike West (OOO) wrote:
> Here, I need to grab the frame tree in order to check the ancestors against
the
> XFO rule. I can do this by exposing the `frame_tree_node_` on
NavigationHandle,
> but there's probably a more elegant mechanism... Halp? :)

I think you should move this to content/ (the frame tree is not accessible
outside of content). Then you can cast the NavigationHandle to a
NavigationHandleImpl, an get the FrameTreeNode from there.

https://codereview.chromium.org/1530393003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1530393003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:235: //
RegisterThrottleForTesting.
If you move the XFOThrottle to content/, you can just add it to the
ScopedVector<NavigationThrottle> here (or below depending on when it should
execute).

https://codereview.chromium.org/1530393003/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:217: render_process_id,
render_frame_id, response_headers));
You need to set *defer=true somewhere here, otherwise the navigation will
proceed.

[Mike West, 2015-12-21 13:01:08]

Patchset #3 (id:40001) has been deleted

[Mike West, 2016-01-15 15:37:10]

Here's another pass that I think is closer to what might actually work. Would
y'all mind evaluating it for sanity? Tests will fail because we're producing an
extra console message, and we're not taking `frame-ancestors` into account, but
I think this should be good enough to evaluate the approach.

Thanks!

https://codereview.chromium.org/1530393003/diff/80001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/80001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:233:
controller()->CancelWithError(net::ERR_BLOCKED_BY_CLIENT);
This is a bit of a hack. I need a mechanism by which I can communicate something
other than ERR_ABORTED to Blink, as Blink treats that code as something it
should pretty much ignore entirely. We don't want to ignore the cancellation
here; we want to load a (blank :( ) error page! And trigger `load` events on the
frame!

This seems to work, but has side-effects I don't like (one extra console error).

Ideas welcome!

[Mike West, 2016-01-18 13:26:00]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2016-01-18 13:26:01]

Jochen, you had an idea about who I should be talking to regarding the error
page at lunch. Mind reminding me who you suggested? :)

https://codereview.chromium.org/1530393003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny.html
(right):

https://codereview.chromium.org/1530393003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny.html:22:
<iframe style="width:500px; height:500px"
src="http://127.0.0.1:8000/security/XFrameOptions/resources/x-frame-options-deny.cgi"
onload="checkIfDone()" onerror="checkIfDone()"></iframe>
I don't understand why this isn't triggering; if I load it up in
`content_shell`, the test passes. If I load it with `--run-layout-test`, the
`load` event never fires (neither does the `error` event, but it shouldn't...).
~4 tests are failing because of this. Halp!

[Mike West, 2016-01-18 14:52:41]

mkwst@chromium.org changed reviewers:
+ ellyjones@chromium.org

[Mike West, 2016-01-18 14:52:41]

+ellyjones@: Hello! I am reliably informed that you can help me figure out a
sane way of working with error pages in Blink layout tests. :) If so, mind
taking a look at this patch to help me determine why `load` events aren't being
fired for layout tests under `content_shell` when `--run-layout-test` is
present?

[clamy, 2016-01-18 17:45:32]

Thanks! I think it's looking better, and have a few comments below.

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:291: void
NavigationHandleImpl::WillProcessResponse(
You also need to call this method in the PlzNavigate code (see the callsites for
NavigationHandleImpl::ReadyToCommitNavigation in NavigatorImpl).

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:166: // will be called.
Well the callback should also be called in the current case so that the code is
restarted right?

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:182: void
ReadyToCommitNavigation(
This should probably become private, and only be called after
WillProcessResponse has determined that the navigation is safe.

https://codereview.chromium.org/1530393003/diff/80001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/80001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:233:
controller()->CancelWithError(net::ERR_BLOCKED_BY_CLIENT);
On 2016/01/15 15:37:10, Mike West wrote:
> This is a bit of a hack. I need a mechanism by which I can communicate
something
> other than ERR_ABORTED to Blink, as Blink treats that code as something it
> should pretty much ignore entirely. We don't want to ignore the cancellation
> here; we want to load a (blank :( ) error page! And trigger `load` events on
the
> frame!
> 
> This seems to work, but has side-effects I don't like (one extra console
error).
> 
> Ideas welcome!

I'm less knowledgeable about the inner workings of the net stack. davidben@
would be a good person to ping, since he also has knowledge of some of the
PlzNavigate code.

https://codereview.chromium.org/1530393003/diff/80001/content/public/browser/...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/80001/content/public/browser/...
content/public/browser/navigation_throttle.h:36: // Block the navigation.
nit: maybe add something like "this will inform the requester of the navigation
that the navigation was blocked." to make the difference with CANCEL clearer.

Also, several places have switch cases on this type, they should be updated to
take the new value into account.

[Mike West, 2016-01-20 18:45:39]

mkwst@chromium.org changed reviewers:
+ davidben@chromium.org
- jochen@chromium.org, nasko@chromium.org

[Mike West, 2016-01-20 18:45:39]

clamy@: Thanks! I've started poking at the PlzNavigate bits, but it's turning
out to be ugly. Maybe we could chat about it later this week?

davidben@: Would you mind taking a look at this and explaining to me that it's
not as horrible or hacky as I think it is? Or, you know, giving me a hint about
how to make it less horrible and hacky? :)

ellyjones@: If you can find time to do the same, specifically with regard to
ideas about why the blocked request generates an error page (and load events!)
in content_shell proper, but not in content_shell under `--run-layout-test`?

Thanks!

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1530393003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:166: // will be called.
On 2016/01/18 at 17:45:32, clamy wrote:
> Well the callback should also be called in the current case so that the code
is restarted right?

Yup. Comment is stale, hopefully this update is better.

[nasko, 2016-01-20 23:15:06]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-01-20 23:15:07]

Some drive-by comments.

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:238: if
(scoped_ptr<content::NavigationThrottle> xfo_throttle =
nit: scoped_ptr on a separate line outside of the if() check.

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:347: // TODO(clamy):
distinguish between CANCEL and CANCEL_AND_IGNORE.
Do we need to distinguish BLOCK as well?

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
File content/browser/frame_host/xfo_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.cc:61: url::Origin
current_origin(navigation_handle()->GetURL());
Shouldn't we be using the GetOrigin() from the current RenderFrameHost for the
FrameTreeNode?

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
File content/browser/frame_host/xfo_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016 :)

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:21: class CONTENT_EXPORT XFOThrottle :
public NavigationThrottle {
XfoThrottle, as per semi-recent chromium-dev@ discussions on capitalization.

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:26: SAMEORIGIN,
Why use _ in NOT_PRESENT, but not in SAME_ORITIN and ALLOW_ALL?

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:36: static
scoped_ptr<NavigationThrottle> MaybeCreateThrottleFor(
Static methods go first in the class, especially creation related ones.

https://codereview.chromium.org/1530393003/diff/120001/content/public/browser...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/120001/content/public/browser...
content/public/browser/navigation_throttle.h:68: // must not return DEFER, as it
is the last step before committing.
Should we be adding BLOCK to the lists of possible results in these comments?

[Mike West, 2016-01-21 14:51:24]

Thanks Nasko. I'm closing out this CL, splitting it into
https://codereview.chromium.org/1616943003 which introduces some infrastructure,
and https://codereview.chromium.org/1617043002 for the throttles. Please feel
encouraged to take a look at those CLs. :)

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
File content/browser/frame_host/xfo_throttle.cc (right):

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.cc:61: url::Origin
current_origin(navigation_handle()->GetURL());
On 2016/01/20 at 23:15:07, nasko wrote:
> Shouldn't we be using the GetOrigin() from the current RenderFrameHost for the
FrameTreeNode?

XFO looks at the origin of the URL, not the origin of the document. That is,
`example.com` -> `example.com` -> `example.com` matches "sameorigin" even if the
middle frame is sandboxed. I guess we could consider changing that behavior?
Probably wouldn't impact many pages, but it would be a behavioral change.

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
File content/browser/frame_host/xfo_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/01/20 at 23:15:07, nasko wrote:
> 2016 :)

It was 2015 when I started. :)

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:21: class CONTENT_EXPORT XFOThrottle :
public NavigationThrottle {
On 2016/01/20 at 23:15:07, nasko wrote:
> XfoThrottle, as per semi-recent chromium-dev@ discussions on capitalization.

...

Style rules never get better, do they? They just trend slowly and inexorably
towards terribleness. :(

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:26: SAMEORIGIN,
On 2016/01/20 at 23:15:07, nasko wrote:
> Why use _ in NOT_PRESENT, but not in SAME_ORITIN and ALLOW_ALL?

Changed to `NONE` to avoid the question.

https://codereview.chromium.org/1530393003/diff/120001/content/browser/frame_...
content/browser/frame_host/xfo_throttle.h:36: static
scoped_ptr<NavigationThrottle> MaybeCreateThrottleFor(
On 2016/01/20 at 23:15:07, nasko wrote:
> Static methods go first in the class, especially creation related ones.

Done.

https://codereview.chromium.org/1530393003/diff/120001/content/public/browser...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1530393003/diff/120001/content/public/browser...
content/public/browser/navigation_throttle.h:68: // must not return DEFER, as it
is the last step before committing.
On 2016/01/20 at 23:15:07, nasko wrote:
> Should we be adding BLOCK to the lists of possible results in these comments?

We should, and have. Thanks!