Allow view-source of pages fully-blocked by Blink's XSS filter.

Allow view-source of pages fully blocked by blinks XSS filter.

Unlike the other kinds of errors which are detected earlier in navigation
(SSL certs, etc), when the Blink reflected XSS filter encounters an
XSS and the page needs to be blocked (per the server's request), we 
already are have a commited navigation, and are well past the point
where interstitials and the like would do us any good.

Consequently, blink just aborts the load, and schedules a navigation to
data:, with history replacement enabled, so that the offending entry
is lost (note https://codereview.chromium.org/301163006/ changes this
behaviour blink-side to add to the back-forward list).

This is less than ideal when a webmaster would like to do a view-source on
the offending page so as to diagnose the cause, so what I've done is to set
up a way to flag the offending entry when the reflection is detected. 
I'd really like to just continue with navigating to data:, rather than
trying to deal with the UX issue -- there's nothing to be done, and 
screaming about XSS isn't helpful to the user -- and we aren't going
to ever add a "revisit the page with protection disabled" option neither.

So, when a block is detected, we make an IPC call to flag the current entry
in the navigation controller.  The navigation then continues to data:,. 

When we encounter a view-source on the data:, page URL, we check if the 
previous page was explicitly flagged prior to the block. If so, show its
source instead.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283728

[Tom Sepez, 2014-05-30 22:51:15]

Sky, here's a patch for a problem I've been scratching my head over for quite a
while.  Let me know what you think of the approach.

[sky, 2014-06-01 19:55:11]

https://codereview.chromium.org/304313003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/304313003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:1149: if (entry->GetURL() ==
GURL("data:,Blocked")) {
Is "data:,Blocked" not a legal url that could be encountered? For example, what
if someone typed it in the omnibox?

[Tom Sepez, 2014-06-03 22:00:27]

> Is "data:,Blocked" not a legal url that could be encountered? For example,
what
> if someone typed it in the omnibox?
Ok, I've handled this situation by marking the entry when the XSS occurs.

[sky, 2014-06-03 22:12:16]

https://codereview.chromium.org/304313003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/304313003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2555: NavigationEntry* entry =
controller_.GetLastCommittedEntry();
How do you know lastcommittedentry is the right entry? Would it be safer to pass
in the page id?

[Tom Sepez, 2014-06-06 20:01:03]

+nate, nasko for suggestions about how to make this cleaner.  Thanks.

[Tom Sepez, 2014-06-10 22:39:05]

Reviewers, please review. I'd like to try to land this.

[sky, 2014-06-10 22:44:27]

LGTM

[nasko, 2014-06-10 23:21:30]

Just a few minor comments.

https://codereview.chromium.org/304313003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/304313003/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.h:348:
linked_ptr<NavigationEntryImpl> blocked_page_entry_;
Why not use scoped_ptr here?

https://codereview.chromium.org/304313003/diff/140001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/304313003/diff/140001/content/common/view_mes...
content/common/view_messages.h:1152:
IPC_MESSAGE_ROUTED3(ViewHostMsg_DidDetectXSS,
I'd rather see this IPC be routed through RenderFrameHost. If it is added here,
we need to do work to move it later, as this file should mostly disappear by the
time site isolation is done :).

https://codereview.chromium.org/304313003/diff/140001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/304313003/diff/140001/content/public/browser/...
content/public/browser/navigation_controller.h:296: virtual void
SetBlockedPageEntry(NavigationEntry* entry) = 0;
This is not called outside of content/, so it shouldn't be in the pubic
interface.

https://codereview.chromium.org/304313003/diff/140001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/304313003/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:2645: render_view_->Send(
Just send it through the RenderFrameImpl Send with the frame routing id. It will
get delivered to WebContents just fine.

[Tom Sepez, 2014-06-10 23:45:14]

On 2014/06/10 23:21:30, nasko wrote:
> Just a few minor comments.
> 
>
https://codereview.chromium.org/304313003/diff/140001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl.h (right):
> 
>
https://codereview.chromium.org/304313003/diff/140001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl.h:348:
> linked_ptr<NavigationEntryImpl> blocked_page_entry_;
> Why not use scoped_ptr here?
> 
Because the lifetime of this beast is managed through linked_ptr's in other
places. Should there be some other spot that's got a link to it, we don't want
to blow it away when we clear this member variable.

>
https://codereview.chromium.org/304313003/diff/140001/content/common/view_mes...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/304313003/diff/140001/content/common/view_mes...
> content/common/view_messages.h:1152:
> IPC_MESSAGE_ROUTED3(ViewHostMsg_DidDetectXSS,
> I'd rather see this IPC be routed through RenderFrameHost. If it is added
here,
> we need to do work to move it later, as this file should mostly disappear by
the
> time site isolation is done :).
> 
Will do.

>
https://codereview.chromium.org/304313003/diff/140001/content/public/browser/...
> File content/public/browser/navigation_controller.h (right):
> 
>
https://codereview.chromium.org/304313003/diff/140001/content/public/browser/...
> content/public/browser/navigation_controller.h:296: virtual void
> SetBlockedPageEntry(NavigationEntry* entry) = 0;
> This is not called outside of content/, so it shouldn't be in the pubic
> interface.
> 
Will do.

>
https://codereview.chromium.org/304313003/diff/140001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/304313003/diff/140001/content/renderer/render...
> content/renderer/render_frame_impl.cc:2645: render_view_->Send(
> Just send it through the RenderFrameImpl Send with the frame routing id. It
will
> get delivered to WebContents just fine.
Will do.

[nasko, 2014-06-11 16:28:12]

LGTM

[Tom Sepez, 2014-06-11 17:10:36]

+brettw for OWNERS review of content/public and content/renderer.

[Tom Sepez, 2014-06-17 21:05:18]

@brettw - ping?

[brettw, 2014-06-18 05:16:57]

Oh sorry. I was looking at this before and I was trying to think of a way to
make it nicer, but couldn't, and then got distracted...

This is sort of a nice-to-have feature for an edge case of developers that
didn't quite square with my general terror of adding more complication to the
nav controller. I was further concerned by my lack of ability to understand the
rules for when we clear or don't clear the blockend entry in NavControllerImpl.
It actually seems like you would want to clear it when renavigating to an
existing page, but wouldn't want to clear it for many of the other ones
(although they shouldn't theoretically come up).

Maybe we should discuss this in-person tomorrow. One thing in particular I'm
wondering is if we can log something to the developer console that might be as
as useful (or maybe even more useful) for diagnosing the problem than viewing
source on the page? Or is there anything else we can do to help webmasters in
this case without adding more special cases to the navigation logic?

[brettw, 2014-06-18 18:00:34]

Looking at the history of this, I liked the general approach of Patch Set 4
better. The code is much simpler and doesn't have to touch any of the super
complicate and delicate navigation logic.

I do agree that it would be desirable to not mess up the back/forward list in
this case, but it doesn't seem that bad. This will very rarely happen, and when
it does, it probably means the page you were on was evil, so making it easier to
get back to doesn't necessarily seem like an important goal.

If we go back to that approach, it should either work one of two ways:
- Content sends some XSS notification to the chrome/browser layer which sets the
XSS extra data (we should probably have a constant for this somewhere).
- Make this a really boolean value on NavigationEntry that content sets and the
browser queries.

The extra data this is really for other components to persist random data so we
don't have to make content aware of every feature people add to the browser that
need to save data. If the content layer is involved at all, we should add a real
value with documentation and such so the contract is clear. Given the current
design of the code, the second approach (just adding a boolean) seems better to
me. If we're planning on adding more elaborate XSS stuff in the chrome layer and
no more to content, the first might be better.

[Tom Sepez, 2014-06-18 21:41:49]

@brettw - is Patch Set #10 what you had in mind?  Thanks.

[brettw, 2014-06-18 21:51:41]

Yeah. LGTM

https://codereview.chromium.org/304313003/diff/180001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/304313003/diff/180001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:1149: // If blink sent us to its
blocked-page URL, show the source of the previous
Can this explain in more detail what happens when Blink blocks the page? This is
kind of subtle...

[nasko, 2014-06-18 21:54:49]

https://codereview.chromium.org/304313003/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/304313003/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:333: bool xss_detected_;
Is this supposed to be serialized for session restore purposes?

[Tom Sepez, 2014-06-18 22:13:22]

https://codereview.chromium.org/304313003/diff/180001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/304313003/diff/180001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:1149: // If blink sent us to its
blocked-page URL, show the source of the previous
On 2014/06/18 21:51:41, brettw wrote:
> Can this explain in more detail what happens when Blink blocks the page? This
is
> kind of subtle...

Done.

https://codereview.chromium.org/304313003/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/304313003/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:333: bool xss_detected_;
On 2014/06/18 21:54:48, nasko wrote:
> Is this supposed to be serialized for session restore purposes?
Probably.  Got a pointer to what should be done?

[nasko, 2014-06-18 22:17:24]

https://codereview.chromium.org/304313003/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/304313003/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:333: bool xss_detected_;
On 2014/06/18 22:13:22, Tom Sepez wrote:
> On 2014/06/18 21:54:48, nasko wrote:
> > Is this supposed to be serialized for session restore purposes?
> Probably.  Got a pointer to what should be done?

Look up in this file for a line with lots of WARNING strings : ).

[Tom Sepez, 2014-06-18 22:30:56]

> > Is this supposed to be serialized for session restore purposes?
> > Probably.  Got a pointer to what should be done?
> 
> Look up in this file for a line with lots of WARNING strings : ).
Added a TODO() to avoid making this CL more complicated (along the lines of
charlie's precedent at line 239).
I don't think there's a lot of downside to missing out on this since its a rare
case.

[nasko, 2014-06-19 22:55:54]

On 2014/06/18 22:30:56, Tom Sepez wrote:
> > > Is this supposed to be serialized for session restore purposes?
> > > Probably.  Got a pointer to what should be done?
> > 
> > Look up in this file for a line with lots of WARNING strings : ).
> Added a TODO() to avoid making this CL more complicated (along the lines of
> charlie's precedent at line 239).
> I don't think there's a lot of downside to missing out on this since its a
rare
> case.

In case it wasn't clear, still LGTM.

[Tom Sepez, 2014-07-16 17:39:25]

Blink side has finally landed, now this can proceed.

[Tom Sepez, 2014-07-16 17:52:22]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-07-16 17:55:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tsepez@chromium.org/304313003/240001

[commit-bot: I haz the power, 2014-07-17 09:43:47]

Change committed as 283728