Move 'frame-src' CSP checks into FrameFetchContext.

Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910
Committed: https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119
Cr-Commit-Position: refs/heads/master@{#398685}

[Mike West, 2016-05-31 14:18:44]

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

[Mike West, 2016-05-31 14:18:44]

WDYT, Jochen?

[Mike West, 2016-05-31 14:20:20]

Patchset #1 (id:1) has been deleted

[Mike West, 2016-05-31 14:25:29]

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin-expected.txt
(right):

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin-expected.txt:8:

This is the only behavioral change: if we navigate to a blocked page, we'll now
end up with a blocked page rather than staying on the page that attempted the
navigation. Eventually, we can fill this blocked page with a friendly error
page, which I think is actually an improvement over the status quo.

[Yoav Weiss, 2016-05-31 14:37:14]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2016-05-31 14:37:15]

Drive-by nit and question :)

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:529: // If we're
loading a frame, grab it's parent's policy for 'frame-src' checks:
s/it's/its/

Also, would've been neat to confine that logic inside a helper function, in
order to avoid canRequestInternal from becoming gigantic (or more gigantic than
it already is :|)

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:530: if (!csp &&
type == Resource::MainResource) {
Why shouldn't that be enforced when m_document has CSP?

[Mike West, 2016-06-01 06:53:29]

mkwst@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[Mike West, 2016-06-01 06:53:30]

creis@, nasko@: There are some content browsertest failures that I don't have
enough context to understand. Would you mind taking a look and helping me work
out what invariant the DCHECK intends to protect? :)

yoav@: The context is a bit strange here. We're dealing with a navigational
request, which occurs before we actually have a document. We're grabbing the
resource that's going to become a document once we've gotten it, so we need to
look to its parent to determine whether or not the request is to be allowed.

[jochen (gone - plz use gerrit), 2016-06-01 15:16:36]

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

[jochen (gone - plz use gerrit), 2016-06-01 15:16:37]

+dcheng who might remember this very issue

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:531: if (Frame*
parentFrame = frame()->tree().parent()) {
is that the right thing to do as opposed to using the frame that caused the
navigation to be initiated?

[Charlie Reis, 2016-06-01 19:47:31]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org

[Charlie Reis, 2016-06-01 19:47:32]

On 2016/06/01 06:53:30, Mike West (OOO until 30th) wrote:
> creis@, nasko@: There are some content browsertest failures that I don't have
> enough context to understand. Would you mind taking a look and helping me work
> out what invariant the DCHECK intends to protect? :)

Yeah, I'm super curious how you're triggering that failure in UpdateState.  :) 
I've been trying to switch us over to finding NavigationEntries by nav_entry_id
and not page_id, but we haven't been able to track down and reason about the
cases they differ.  (I had to revert https://crrev.com/395396 due to crashes in
https://crbug.com/614310, and I've been hunting for repro steps.)

I'm not entirely clear on what your CL is doing, or why it affects that logic. 
Here's what might be happening, though.

In UpdateTitle, |entry| is found using RenderViewHostImpl::page_id_, which gets
updated via OnDidAssignPageId when a navigation commits.  Specifically, the
DidAssignPageId IPC is sent from RenderFrameImpl::didCommitProvisionalLoad,
shortly before we send the DidCommitProvisionalLoad IPC.  In contrast,
|new_entry| is found using RenderFrameHostImpl::nav_entry_id_, which gets
updated for each frame at the end of
NavigationControllerImpl::RendererDidNavigate.  That happens in response to the
DidCommitProvisionalLoad IPC.

In the tests where you're hitting the failure, we create OOPIFs and navigate to
a URL blocked by CSP.  For example, CrossSiteIframeBlockedByParentCSPFromMeta is
hitting the failure when navigating to a blocked URL and expecting a load event
to fire:

  // The blocked frame should still fire a load event in its parent's process.
  EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());

We apparently do get an UpdateTitle IPC, but somehow the page_id approach and
nav_entry_id approach end up pointing to different NavigationEntries, which is
strange to me.  Maybe the renderer is committing a page but
NavigationControllerImpl::RendererDidNavigate ends up ignoring it and not
updating nav_entry_id?  (Ignored commits are scary, so I'm glad we're catching
it if so.)

I might need to patch in your CL to see what's going on.  Adding alexmos@, since
he'll likely be able to shed light on the OOPIF CSP behavior here.

[Charlie Reis, 2016-06-01 20:40:37]

On 2016/06/01 19:47:32, Charlie Reis wrote:
> On 2016/06/01 06:53:30, Mike West (OOO until 30th) wrote:
> > creis@, nasko@: There are some content browsertest failures that I don't
have
> > enough context to understand. Would you mind taking a look and helping me
work
> > out what invariant the DCHECK intends to protect? :)
> 
> Yeah, I'm super curious how you're triggering that failure in UpdateState.  :)

> I've been trying to switch us over to finding NavigationEntries by
nav_entry_id
> and not page_id, but we haven't been able to track down and reason about the
> cases they differ.  (I had to revert https://crrev.com/395396 due to crashes
in
> https://crbug.com/614310, and I've been hunting for repro steps.)
> 
> I'm not entirely clear on what your CL is doing, or why it affects that logic.

> Here's what might be happening, though.
> 
> In UpdateTitle, |entry| is found using RenderViewHostImpl::page_id_, which
gets
> updated via OnDidAssignPageId when a navigation commits.  Specifically, the
> DidAssignPageId IPC is sent from RenderFrameImpl::didCommitProvisionalLoad,
> shortly before we send the DidCommitProvisionalLoad IPC.  In contrast,
> |new_entry| is found using RenderFrameHostImpl::nav_entry_id_, which gets
> updated for each frame at the end of
> NavigationControllerImpl::RendererDidNavigate.  That happens in response to
the
> DidCommitProvisionalLoad IPC.
> 
> In the tests where you're hitting the failure, we create OOPIFs and navigate
to
> a URL blocked by CSP.  For example, CrossSiteIframeBlockedByParentCSPFromMeta
is
> hitting the failure when navigating to a blocked URL and expecting a load
event
> to fire:
> 
>   // The blocked frame should still fire a load event in its parent's process.
>   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
> 
> We apparently do get an UpdateTitle IPC, but somehow the page_id approach and
> nav_entry_id approach end up pointing to different NavigationEntries, which is
> strange to me.  Maybe the renderer is committing a page but
> NavigationControllerImpl::RendererDidNavigate ends up ignoring it and not
> updating nav_entry_id?  (Ignored commits are scary, so I'm glad we're catching
> it if so.)
> 
> I might need to patch in your CL to see what's going on.  Adding alexmos@,
since
> he'll likely be able to shed light on the OOPIF CSP behavior here.

Ha.  Good news, you've tripped across some repro steps that are blindingly
obvious in retrospect.  :)  (The page ID can't possibly be right after we do a
navigation in an OOPIF, so it incorrectly tells us to update the title on the
old entry.)  I still have some open questions (like why the test doesn't hit
this failure without your change, and why I saw UpdateTitle crashes in non-OOPIF
modes as well), but I'll put up a CL that should unblock you here momentarily. 
I'll post a link to the bug and CL once I get them up.

[Charlie Reis, 2016-06-01 20:47:56]

On 2016/06/01 20:40:37, Charlie Reis wrote:
> On 2016/06/01 19:47:32, Charlie Reis wrote:
> > On 2016/06/01 06:53:30, Mike West (OOO until 30th) wrote:
> > > creis@, nasko@: There are some content browsertest failures that I don't
> have
> > > enough context to understand. Would you mind taking a look and helping me
> work
> > > out what invariant the DCHECK intends to protect? :)
> > 
> > Yeah, I'm super curious how you're triggering that failure in UpdateState. 
:)
> 
> > I've been trying to switch us over to finding NavigationEntries by
> nav_entry_id
> > and not page_id, but we haven't been able to track down and reason about the
> > cases they differ.  (I had to revert https://crrev.com/395396 due to crashes
> in
> > https://crbug.com/614310, and I've been hunting for repro steps.)
> > 
> > I'm not entirely clear on what your CL is doing, or why it affects that
logic.
> 
> > Here's what might be happening, though.
> > 
> > In UpdateTitle, |entry| is found using RenderViewHostImpl::page_id_, which
> gets
> > updated via OnDidAssignPageId when a navigation commits.  Specifically, the
> > DidAssignPageId IPC is sent from RenderFrameImpl::didCommitProvisionalLoad,
> > shortly before we send the DidCommitProvisionalLoad IPC.  In contrast,
> > |new_entry| is found using RenderFrameHostImpl::nav_entry_id_, which gets
> > updated for each frame at the end of
> > NavigationControllerImpl::RendererDidNavigate.  That happens in response to
> the
> > DidCommitProvisionalLoad IPC.
> > 
> > In the tests where you're hitting the failure, we create OOPIFs and navigate
> to
> > a URL blocked by CSP.  For example,
CrossSiteIframeBlockedByParentCSPFromMeta
> is
> > hitting the failure when navigating to a blocked URL and expecting a load
> event
> > to fire:
> > 
> >   // The blocked frame should still fire a load event in its parent's
process.
> >   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
> > 
> > We apparently do get an UpdateTitle IPC, but somehow the page_id approach
and
> > nav_entry_id approach end up pointing to different NavigationEntries, which
is
> > strange to me.  Maybe the renderer is committing a page but
> > NavigationControllerImpl::RendererDidNavigate ends up ignoring it and not
> > updating nav_entry_id?  (Ignored commits are scary, so I'm glad we're
catching
> > it if so.)
> > 
> > I might need to patch in your CL to see what's going on.  Adding alexmos@,
> since
> > he'll likely be able to shed light on the OOPIF CSP behavior here.
> 
> Ha.  Good news, you've tripped across some repro steps that are blindingly
> obvious in retrospect.  :)  (The page ID can't possibly be right after we do a
> navigation in an OOPIF, so it incorrectly tells us to update the title on the
> old entry.)  I still have some open questions (like why the test doesn't hit
> this failure without your change, and why I saw UpdateTitle crashes in
non-OOPIF
> modes as well), but I'll put up a CL that should unblock you here momentarily.

> I'll post a link to the bug and CL once I get them up.

Oh, ok.  Before your CL we never committed anything for the blocked navigation,
and now we commit about:blank.  That explains how you triggered the bug.

One concern for your change that I noticed while debugging: in
didCommitProvisionalLoad, we still set FrameNavigateParams::redirects to
{"http://b.com:59998/title2.html", "http://c.com:59998/title3.html"}, but
FrameNavigateParams::url is "about:blank".  We probably shouldn't be putting the
blocked URL (http://c.com:59998/title3.html) in the redirects array, since that
might get misinterpreted as the destination URL by some logic.  (At a glance,
I'm guessing we should verify that the last url in |redirects| matches
params.url, but who knows how many corner cases there are.)

[Charlie Reis, 2016-06-01 22:14:05]

On 2016/06/01 20:40:37, Charlie Reis wrote:
> I'll post a link to the bug and CL once I get them up.

https://crbug.com/616609 and https://codereview.chromium.org/2036463002/ should
get you unblocked.

[alexmos, 2016-06-01 22:21:01]

alexmos@chromium.org changed reviewers:
+ lukasza@chromium.org

[alexmos, 2016-06-01 22:21:02]

On 2016/06/01 19:47:32, Charlie Reis wrote:
> On 2016/06/01 06:53:30, Mike West (OOO until 30th) wrote:
> > creis@, nasko@: There are some content browsertest failures that I don't
have
> > enough context to understand. Would you mind taking a look and helping me
work
> > out what invariant the DCHECK intends to protect? :)
> 
> Yeah, I'm super curious how you're triggering that failure in UpdateState.  :)

> I've been trying to switch us over to finding NavigationEntries by
nav_entry_id
> and not page_id, but we haven't been able to track down and reason about the
> cases they differ.  (I had to revert https://crrev.com/395396 due to crashes
in
> https://crbug.com/614310, and I've been hunting for repro steps.)
> 
> I'm not entirely clear on what your CL is doing, or why it affects that logic.

> Here's what might be happening, though.
> 
> In UpdateTitle, |entry| is found using RenderViewHostImpl::page_id_, which
gets
> updated via OnDidAssignPageId when a navigation commits.  Specifically, the
> DidAssignPageId IPC is sent from RenderFrameImpl::didCommitProvisionalLoad,
> shortly before we send the DidCommitProvisionalLoad IPC.  In contrast,
> |new_entry| is found using RenderFrameHostImpl::nav_entry_id_, which gets
> updated for each frame at the end of
> NavigationControllerImpl::RendererDidNavigate.  That happens in response to
the
> DidCommitProvisionalLoad IPC.
> 
> In the tests where you're hitting the failure, we create OOPIFs and navigate
to
> a URL blocked by CSP.  For example, CrossSiteIframeBlockedByParentCSPFromMeta
is
> hitting the failure when navigating to a blocked URL and expecting a load
event
> to fire:
> 
>   // The blocked frame should still fire a load event in its parent's process.
>   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
> 
> We apparently do get an UpdateTitle IPC, but somehow the page_id approach and
> nav_entry_id approach end up pointing to different NavigationEntries, which is
> strange to me.  Maybe the renderer is committing a page but
> NavigationControllerImpl::RendererDidNavigate ends up ignoring it and not
> updating nav_entry_id?  (Ignored commits are scary, so I'm glad we're catching
> it if so.)
> 
> I might need to patch in your CL to see what's going on.  Adding alexmos@,
since
> he'll likely be able to shed light on the OOPIF CSP behavior here.

Charlie already figured this out, but I'll just add that at some point I
similarly switched frame-ancestors to navigate to a blocked (blank) page instead
of staying at the old page, which simplified OOPIF scenarios (see issue 584845),
so this change makes frame-src more consistent with that, which is great.  Note
that the three failing tests currently assume that the last committed URL and
document title stay the same after frame-src blocks the navigation, so those
expectations will need to be updated in addition to Charlie's fix to use
new_entry in UpdateTitle.  Also adding lukasza@, who recently added those tests
while making frame-src work with OOPIFs.

Also, one additional question below.

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:534:
frame()->loader().forceSandboxFlags(SandboxOrigin);
Why use forceSandboxFlags here?  Doesn't that mean that if you first load a
blocked URL into the frame and then load an allowed URL, the document with the
allowed URL but end up being sandboxed incorrectly?

[alexmos, 2016-06-01 22:22:07]

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:534:
frame()->loader().forceSandboxFlags(SandboxOrigin);
On 2016/06/01 22:21:02, alexmos wrote:
> Why use forceSandboxFlags here?  Doesn't that mean that if you first load a
> blocked URL into the frame and then load an allowed URL, the document with the
> allowed URL but end up being sandboxed incorrectly?

sorry, s/but/will/

[Mike West, 2016-06-02 08:58:02]

Thanks folks! I've taken another stab at the blink-side implementation, and
fixed up the //content-side tests as alexmos@ suggested.

WDYT?

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:531: if (Frame*
parentFrame = frame()->tree().parent()) {
On 2016/06/01 at 15:16:37, jochen wrote:
> is that the right thing to do as opposed to using the frame that caused the
navigation to be initiated?

1. I don't think we know what frame caused the navigation at this point, do we?

2. I think it is correct, as a page's policy governs what resources are loaded
inside of it. I agree that there's some strangeness here with regard to the
request's initiator, but if we made any changes, I think they'd probably be to
check both the initiator _and_ the frame owner.

https://codereview.chromium.org/2022083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:534:
frame()->loader().forceSandboxFlags(SandboxOrigin);
On 2016/06/01 at 22:22:07, alexmos wrote:
> On 2016/06/01 22:21:02, alexmos wrote:
> > Why use forceSandboxFlags here?  Doesn't that mean that if you first load a
> > blocked URL into the frame and then load an allowed URL, the document with
the
> > allowed URL but end up being sandboxed incorrectly?
> 
> sorry, s/but/will/

You're right, thanks! I wasn't thinking, obviously. I've adjusted the unique
origin bit to happen inside DocumentLoader rather than here in
FrameFetchContext, as that seems like a more reasonable spot to deal with the
result of the blocked load.

[Yoav Weiss, 2016-06-02 09:15:15]

https://codereview.chromium.org/2022083002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:530: if (!csp &&
type == Resource::MainResource) {
I understand that the assumption here is that csp is NULL (because m_document
is) when we're navigating to a frame.
Can you make that assumption explicit (comment, etc)?

https://codereview.chromium.org/2022083002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:542: }
should we return here? Or nullify csp? Otherwise, we've change csp to the
parent's, which may change code paths further down the function? (e.g. we'd call
csp->allowRequest when we previously didn't) I'm not sure that outcome would be
different, but still..

Alternatively, isolating the frame logic in a helper function would help making
sure it doesn't have such side effects on the rest of the function.

[Mike West, 2016-06-02 13:39:30]

Fixed the remaining broken tests; WDYT, yoav@, alexmos@, et al?

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:517: if
(contentSecurityPolicyBlocksRequest(type, resourceRequest, url, options,
forPreload, redirectStatus))
Does this extraction make you happier, Yoav? :)

[Yoav Weiss, 2016-06-02 14:13:38]

core/ LGTM % a question

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:639: loadUnique();
What's the reason for this change?

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:517: if
(contentSecurityPolicyBlocksRequest(type, resourceRequest, url, options,
forPreload, redirectStatus))
On 2016/06/02 13:39:30, Mike West (OOO until 30th) wrote:
> Does this extraction make you happier, Yoav? :)

way happier :D

[Mike West, 2016-06-02 14:25:45]

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:639: loadUnique();
On 2016/06/02 at 14:13:37, Yoav Weiss wrote:
> What's the reason for this change?

Blocking the response ought to leave us with a document that appears
cross-origin to its embedder. We do this in the status quo by calling
`m_frame->document()->enforceSandboxFlags(SandboxOrigin)` in
`FrameLoader::shouldContinueForNavigationPolicy`. Since we're now actually
committing a navigation when a page is blocked, enforcing sandbox flags in
FrameFetchContext wouldn't be effective (as there's no Document yet). The
original patch forced sandboxing flags onto the FrameLoader, but alexmos@
correctly noted that that was a terrible idea.

This approach changes DocumentLoader to commit a navigation to a URL we'll treat
as unique. This is the same thing we do for the XSS auditor and XFO, so this
just aligns blockage in general with that mechanism.

[Yoav Weiss, 2016-06-02 14:28:07]

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:639: loadUnique();
On 2016/06/02 14:25:45, Mike West (OOO until 30th) wrote:
> On 2016/06/02 at 14:13:37, Yoav Weiss wrote:
> > What's the reason for this change?
> 
> Blocking the response ought to leave us with a document that appears
> cross-origin to its embedder. We do this in the status quo by calling
> `m_frame->document()->enforceSandboxFlags(SandboxOrigin)` in
> `FrameLoader::shouldContinueForNavigationPolicy`. Since we're now actually
> committing a navigation when a page is blocked, enforcing sandbox flags in
> FrameFetchContext wouldn't be effective (as there's no Document yet). The
> original patch forced sandboxing flags onto the FrameLoader, but alexmos@
> correctly noted that that was a terrible idea.
> 
> This approach changes DocumentLoader to commit a navigation to a URL we'll
treat
> as unique. This is the same thing we do for the XSS auditor and XFO, so this
> just aligns blockage in general with that mechanism.

OK, cool!

[Charlie Reis, 2016-06-02 16:48:19]

On 2016/06/01 20:47:56, Charlie Reis wrote:
> One concern for your change that I noticed while debugging: in
> didCommitProvisionalLoad, we still set FrameNavigateParams::redirects to
> {"http://b.com:59998/title2.html", "http://c.com:59998/title3.html"}, but
> FrameNavigateParams::url is "about:blank".  We probably shouldn't be putting
the
> blocked URL (http://c.com:59998/title3.html) in the redirects array, since
that
> might get misinterpreted as the destination URL by some logic.  (At a glance,
> I'm guessing we should verify that the last url in |redirects| matches
> params.url, but who knows how many corner cases there are.)

Can you look at this concern about the redirects array?  (Sorry, I'm sure it got
buried in my other comments.)

[Mike West, 2016-06-02 17:03:43]

On 2016/06/02 at 16:48:19, creis wrote:
> On 2016/06/01 20:47:56, Charlie Reis wrote:
> > One concern for your change that I noticed while debugging: in
> > didCommitProvisionalLoad, we still set FrameNavigateParams::redirects to
> > {"http://b.com:59998/title2.html", "http://c.com:59998/title3.html"}, but
> > FrameNavigateParams::url is "about:blank".  We probably shouldn't be putting
the
> > blocked URL (http://c.com:59998/title3.html) in the redirects array, since
that
> > might get misinterpreted as the destination URL by some logic.  (At a
glance,
> > I'm guessing we should verify that the last url in |redirects| matches
> > params.url, but who knows how many corner cases there are.)
> 
> Can you look at this concern about the redirects array?  (Sorry, I'm sure it
got buried in my other comments.)

Sure. Um. Where would I look at that? :) Is there a test somewhere I can/should
extend?

[Charlie Reis, 2016-06-02 17:22:12]

On 2016/06/02 17:03:43, Mike West (OOO until 30th) wrote:
> On 2016/06/02 at 16:48:19, creis wrote:
> > On 2016/06/01 20:47:56, Charlie Reis wrote:
> > > One concern for your change that I noticed while debugging: in
> > > didCommitProvisionalLoad, we still set FrameNavigateParams::redirects to
> > > {"http://b.com:59998/title2.html", "http://c.com:59998/title3.html"}, but
> > > FrameNavigateParams::url is "about:blank".  We probably shouldn't be
putting
> the
> > > blocked URL (http://c.com:59998/title3.html) in the redirects array, since
> that
> > > might get misinterpreted as the destination URL by some logic.  (At a
> glance,
> > > I'm guessing we should verify that the last url in |redirects| matches
> > > params.url, but who knows how many corner cases there are.)
> > 
> > Can you look at this concern about the redirects array?  (Sorry, I'm sure it
> got buried in my other comments.)
> 
> Sure. Um. Where would I look at that? :) Is there a test somewhere I
can/should
> extend?

RenderFrameImpl gets the data from WebDataSource::redirectChain, so it might be
possible to write a WebFrameTest?  Or one of the SitePerProcessBrowserTests that
you're modifying would be an easy place to test it.  Just grab the
FrameNavigateParams at commit time (using
WebContentsObserver::DidNavigateAnyFrame) and verify the blocked URL isn't in
params.redirects.  There's an example in FrameNavigateParamsCapturer in
navigation_controller_impl_browsertest.cc.  (I'm not opposed to abstracting that
out into content_browser_test_utils_internal.h if you just want to share it.)

Thanks!

https://codereview.chromium.org/2022083002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2022083002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6435: EXPECT_EQ(GURL("data:,"),
root->child_at(0)->current_url());
Is the comment wrong?  about:blank and data:, aren't the same, are they?  (Same
below.)

[dcheng, 2016-06-02 21:48:56]

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:577: if (Frame*
parentFrame = frame()->tree().parent()) {
How does CSP inheritance work? Is it always strictly from the parent? Are there
any edge cases where it would not be inherited from the parent? I'm thinking of
something like https://crbug.com/583445, where document.open() changes the URL
to 'about:blank', yet the SecurityOrigin should not be inherited from the
parent.

[alexmos, 2016-06-02 22:21:07]

Great, I like the loadUnique() approach much better than tinkering with sandbox
flags, so that part LGTM.  Just a couple of nits, and I'm also curious about
Charlie's redirect question.

https://codereview.chromium.org/2022083002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2022083002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6435: EXPECT_EQ(GURL("data:,"),
root->child_at(0)->current_url());
nit: maybe we could just define a kBlockedPageURL and use that everywhere in
this file?  Also +1 to Charlie's comment about the comment :)

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:639: loadUnique();
Probably not for this CL, but I'm curious if we should also use loadUnique() in
cancelLoadAfterXFrameOptionsOrCSPDenied().  It probably doesn't matter much
though, since you're moving that to the browser process.

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:575: // yet (so
|csp| will be nullptr). We instead need to grab the frame's
nit: I don't see |csp| defined anywhere in this function :)

[Mike West, 2016-06-06 08:40:10]

creis@, alexmos@: Would y'all mind taking a look at the changes to WebFrameTest
and DocumentLoader in the latest patchset? I think it's a reasonable approach,
but I don't know the clients of WebDataSource well enough to judge.

https://codereview.chromium.org/2022083002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2022083002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6435: EXPECT_EQ(GURL("data:,"),
root->child_at(0)->current_url());
On 2016/06/02 at 22:21:06, alexmos wrote:
> nit: maybe we could just define a kBlockedPageURL and use that everywhere in
this file?  Also +1 to Charlie's comment about the comment :)

Done and done.

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:639: loadUnique();
On 2016/06/02 at 22:21:07, alexmos wrote:
> Probably not for this CL, but I'm curious if we should also use loadUnique()
in cancelLoadAfterXFrameOptionsOrCSPDenied().  It probably doesn't matter much
though, since you're moving that to the browser process.

Makes sense. I'll reevaluate that once we re-land XFO in the browser.

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:575: // yet (so
|csp| will be nullptr). We instead need to grab the frame's
On 2016/06/02 at 22:21:07, alexmos wrote:
> nit: I don't see |csp| defined anywhere in this function :)

Done. Too much refactoring. :)

https://codereview.chromium.org/2022083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:577: if (Frame*
parentFrame = frame()->tree().parent()) {
On 2016/06/02 at 21:48:56, dcheng wrote:
> How does CSP inheritance work? Is it always strictly from the parent? Are
there any edge cases where it would not be inherited from the parent? I'm
thinking of something like https://crbug.com/583445, where document.open()
changes the URL to 'about:blank', yet the SecurityOrigin should not be inherited
from the parent.

In this case, I think pulling the policy from the parent is fine. The goal of
`frame-src` is to allow a page to control the resources which it embeds. That
is, it controls the things which can be loaded into an `<iframe>`.

We only hit this case for requests which are navigating a frame, and since the
embedder's policy is the thing we need to look to when determining what it
allows itself to embed, I think we're correct in grabbing the parent frame's
policy if it exists.

https://codereview.chromium.org/2022083002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:643:
appendRedirect(SecurityOrigin::urlWithUniqueSecurityOrigin());
Charlie: It seems reasonable to me to treat blocking as a redirect of its own (I
can imagine that clients of the redirects array might actually want the URL that
we would have loaded if it hadn't been blocked). That approach also lends itself
to the current implementation (as we append a redirect in FrameLoader before we
have any idea whether or not we'll end up blocking the request.

Does this seem like a reasonable approach, or should we refactor the append
entirely?

[Charlie Reis, 2016-06-06 20:43:06]

creis@chromium.org changed reviewers:
+ japhet@chromium.org

[Charlie Reis, 2016-06-06 20:43:07]

[+japhet]

On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> creis@, alexmos@: Would y'all mind taking a look at the changes to
WebFrameTest
> and DocumentLoader in the latest patchset? I think it's a reasonable approach,
> but I don't know the clients of WebDataSource well enough to judge.

I'll have to defer to japhet@, since I don't know that area well enough.

https://codereview.chromium.org/2022083002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2022083002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6530: // The blocked frame
should commit to |kBlockedURL|
nit: Keep the period.  (Same above.)

https://codereview.chromium.org/2022083002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2022083002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:643:
appendRedirect(SecurityOrigin::urlWithUniqueSecurityOrigin());
On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> Charlie: It seems reasonable to me to treat blocking as a redirect of its own
(I
> can imagine that clients of the redirects array might actually want the URL
that
> we would have loaded if it hadn't been blocked). That approach also lends
itself
> to the current implementation (as we append a redirect in FrameLoader before
we
> have any idea whether or not we'll end up blocking the request.
> 
> Does this seem like a reasonable approach, or should we refactor the append
> entirely?

Hmm, that feels weird/unfortunate to me, but maybe it works.  I thought the goal
was to act like the renderer never got to the blocked URL, but here we're making
it look like it went through the blocked URL to end up at about:blank.

That said, I don't know if anything looks at the intermediate hops of the
redirects array and infers that the renderer had access to them, and I don't
know this area well enough to recommend a better plan.  It certainly seems
better to do this than leave the blocked URL as the last item of the redirects
array, which we do assume to be equal to the committed URL in some places.  I'll
defer unless Nate has a better idea.

[Mike West, 2016-06-07 08:08:41]

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

[Mike West, 2016-06-07 08:08:42]

On 2016/06/06 at 20:43:07, creis wrote:
> On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> > Charlie: It seems reasonable to me to treat blocking as a redirect of its
own (I
> > can imagine that clients of the redirects array might actually want the URL
that
> > we would have loaded if it hadn't been blocked). That approach also lends
itself
> > to the current implementation (as we append a redirect in FrameLoader before
we
> > have any idea whether or not we'll end up blocking the request.
> > 
> > Does this seem like a reasonable approach, or should we refactor the append
> > entirely?
> 
> Hmm, that feels weird/unfortunate to me, but maybe it works.  I thought the
goal was to act like the renderer never got to the blocked URL, but here we're
making it look like it went through the blocked URL to end up at about:blank.
> 
> That said, I don't know if anything looks at the intermediate hops of the
redirects array and infers that the renderer had access to them, and I don't
know this area well enough to recommend a better plan.  It certainly seems
better to do this than leave the blocked URL as the last item of the redirects
array, which we do assume to be equal to the committed URL in some places.  I'll
defer unless Nate has a better idea.

So far as I can tell, SafeBrowsing is the only client of this data (it ends up
in the `BrowseInfo` struct in `browser_feature_extractor.h`). It seems like that
will end up being more helpful than harmful, as it gives SB a signal that a
particular page was blocked. I imagine that could be useful for their analysis.

+nparker@, who might have opinions.

If neither nparker@ nor japhet@ object, this seems like the simplest approach,
and I'd like to run with it. It's certainly an improvement over the status quo.

[Charlie Reis, 2016-06-07 22:22:34]

On 2016/06/07 08:08:42, Mike West (OOO until 30th) wrote:
> On 2016/06/06 at 20:43:07, creis wrote:
> > On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> > > Charlie: It seems reasonable to me to treat blocking as a redirect of its
> own (I
> > > can imagine that clients of the redirects array might actually want the
URL
> that
> > > we would have loaded if it hadn't been blocked). That approach also lends
> itself
> > > to the current implementation (as we append a redirect in FrameLoader
before
> we
> > > have any idea whether or not we'll end up blocking the request.
> > > 
> > > Does this seem like a reasonable approach, or should we refactor the
append
> > > entirely?
> > 
> > Hmm, that feels weird/unfortunate to me, but maybe it works.  I thought the
> goal was to act like the renderer never got to the blocked URL, but here we're
> making it look like it went through the blocked URL to end up at about:blank.
> > 
> > That said, I don't know if anything looks at the intermediate hops of the
> redirects array and infers that the renderer had access to them, and I don't
> know this area well enough to recommend a better plan.  It certainly seems
> better to do this than leave the blocked URL as the last item of the redirects
> array, which we do assume to be equal to the committed URL in some places. 
I'll
> defer unless Nate has a better idea.
> 
> So far as I can tell, SafeBrowsing is the only client of this data (it ends up
> in the `BrowseInfo` struct in `browser_feature_extractor.h`). It seems like
that
> will end up being more helpful than harmful, as it gives SB a signal that a
> particular page was blocked. I imagine that could be useful for their
analysis.
> 
> +nparker@, who might have opinions.
> 
> If neither nparker@ nor japhet@ object, this seems like the simplest approach,
> and I'd like to run with it. It's certainly an improvement over the status
quo.

Works for me, thanks.  LGTM pending their approval.

[Charlie Reis, 2016-06-07 22:23:18]

Description was changed from

==========
Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910
==========

to

==========
Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910
==========

[Charlie Reis, 2016-06-07 22:23:18]

creis@chromium.org changed reviewers:
+ nparker@chromium.org
- nathanr@chromium.org

[Charlie Reis, 2016-06-07 22:24:24]

[-nathanr, +nparker]

Looks like there was a typo in the reviewer list.  Nathan, can you take a look
at the discussion about the redirects array and whether it's helpful/harmful to
have a blocked URL in it?

[Nathan Parker, 2016-06-08 15:55:33]

+jialiul for Charlie's request

[Nathan Parker, 2016-06-08 15:56:06]

nparker@chromium.org changed reviewers:
+ jialiul@chromium.org

[Jialiu Lin, 2016-06-08 18:23:14]

On 2016/06/07 at 08:08:42, mkwst wrote:
> On 2016/06/06 at 20:43:07, creis wrote:
> > On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> > > Charlie: It seems reasonable to me to treat blocking as a redirect of its
own (I
> > > can imagine that clients of the redirects array might actually want the
URL that
> > > we would have loaded if it hadn't been blocked). That approach also lends
itself
> > > to the current implementation (as we append a redirect in FrameLoader
before we
> > > have any idea whether or not we'll end up blocking the request.
> > > 
> > > Does this seem like a reasonable approach, or should we refactor the
append
> > > entirely?
> > 
> > Hmm, that feels weird/unfortunate to me, but maybe it works.  I thought the
goal was to act like the renderer never got to the blocked URL, but here we're
making it look like it went through the blocked URL to end up at about:blank.
> > 
> > That said, I don't know if anything looks at the intermediate hops of the
redirects array and infers that the renderer had access to them, and I don't
know this area well enough to recommend a better plan.  It certainly seems
better to do this than leave the blocked URL as the last item of the redirects
array, which we do assume to be equal to the committed URL in some places.  I'll
defer unless Nate has a better idea.
> 
> So far as I can tell, SafeBrowsing is the only client of this data (it ends up
in the `BrowseInfo` struct in `browser_feature_extractor.h`). It seems like that
will end up being more helpful than harmful, as it gives SB a signal that a
particular page was blocked. I imagine that could be useful for their analysis.
> 
> +nparker@, who might have opinions.
> 
> If neither nparker@ nor japhet@ object, this seems like the simplest approach,
and I'd like to run with it. It's certainly an improvement over the status quo.

Thanks for the code pointer, mkwst! It seems redirect array is used as one of
the features in client side phishing detection. That part of code is pretty old,
I guess no body knows how this feature actually impact the detection results,
but based on my educated guess, it shouldn't. So please go head. Let me know
when your code is landed in stable. I will keep an eye on safe browsing
dashboard just in case.

[Jialiu Lin, 2016-06-08 18:23:48]

lgtm

[Mike West, 2016-06-08 18:38:43]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-06-08 18:38:43]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2022083002/#ps100001
(title: "redirects")

[commit-bot: I haz the power, 2016-06-08 18:39:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2022083002/100001

[Mike West, 2016-06-08 18:45:02]

On 2016/06/08 at 18:23:14, jialiul wrote:
> On 2016/06/07 at 08:08:42, mkwst wrote:
> > On 2016/06/06 at 20:43:07, creis wrote:
> > > On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> > > > Charlie: It seems reasonable to me to treat blocking as a redirect of
its own (I
> > > > can imagine that clients of the redirects array might actually want the
URL that
> > > > we would have loaded if it hadn't been blocked). That approach also
lends itself
> > > > to the current implementation (as we append a redirect in FrameLoader
before we
> > > > have any idea whether or not we'll end up blocking the request.
> > > > 
> > > > Does this seem like a reasonable approach, or should we refactor the
append
> > > > entirely?
> > > 
> > > Hmm, that feels weird/unfortunate to me, but maybe it works.  I thought
the goal was to act like the renderer never got to the blocked URL, but here
we're making it look like it went through the blocked URL to end up at
about:blank.
> > > 
> > > That said, I don't know if anything looks at the intermediate hops of the
redirects array and infers that the renderer had access to them, and I don't
know this area well enough to recommend a better plan.  It certainly seems
better to do this than leave the blocked URL as the last item of the redirects
array, which we do assume to be equal to the committed URL in some places.  I'll
defer unless Nate has a better idea.
> > 
> > So far as I can tell, SafeBrowsing is the only client of this data (it ends
up in the `BrowseInfo` struct in `browser_feature_extractor.h`). It seems like
that will end up being more helpful than harmful, as it gives SB a signal that a
particular page was blocked. I imagine that could be useful for their analysis.
> > 
> > +nparker@, who might have opinions.
> > 
> > If neither nparker@ nor japhet@ object, this seems like the simplest
approach, and I'd like to run with it. It's certainly an improvement over the
status quo.
> 
> Thanks for the code pointer, mkwst! It seems redirect array is used as one of
the features in client side phishing detection. That part of code is pretty old,
I guess no body knows how this feature actually impact the detection results,
but based on my educated guess, it shouldn't. So please go head. Let me know
when your code is landed in stable. I will keep an eye on safe browsing
dashboard just in case.

Great, thanks!

[Mike West, 2016-06-08 18:59:39]

On 2016/06/08 at 18:45:02, Mike West (OOO until 30th) wrote:
> On 2016/06/08 at 18:23:14, jialiul wrote:
> > On 2016/06/07 at 08:08:42, mkwst wrote:
> > > On 2016/06/06 at 20:43:07, creis wrote:
> > > > On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> > > > > Charlie: It seems reasonable to me to treat blocking as a redirect of
its own (I
> > > > > can imagine that clients of the redirects array might actually want
the URL that
> > > > > we would have loaded if it hadn't been blocked). That approach also
lends itself
> > > > > to the current implementation (as we append a redirect in FrameLoader
before we
> > > > > have any idea whether or not we'll end up blocking the request.
> > > > > 
> > > > > Does this seem like a reasonable approach, or should we refactor the
append
> > > > > entirely?
> > > > 
> > > > Hmm, that feels weird/unfortunate to me, but maybe it works.  I thought
the goal was to act like the renderer never got to the blocked URL, but here
we're making it look like it went through the blocked URL to end up at
about:blank.
> > > > 
> > > > That said, I don't know if anything looks at the intermediate hops of
the redirects array and infers that the renderer had access to them, and I don't
know this area well enough to recommend a better plan.  It certainly seems
better to do this than leave the blocked URL as the last item of the redirects
array, which we do assume to be equal to the committed URL in some places.  I'll
defer unless Nate has a better idea.
> > > 
> > > So far as I can tell, SafeBrowsing is the only client of this data (it
ends up in the `BrowseInfo` struct in `browser_feature_extractor.h`). It seems
like that will end up being more helpful than harmful, as it gives SB a signal
that a particular page was blocked. I imagine that could be useful for their
analysis.
> > > 
> > > +nparker@, who might have opinions.
> > > 
> > > If neither nparker@ nor japhet@ object, this seems like the simplest
approach, and I'd like to run with it. It's certainly an improvement over the
status quo.
> > 
> > Thanks for the code pointer, mkwst! It seems redirect array is used as one
of the features in client side phishing detection. That part of code is pretty
old, I guess no body knows how this feature actually impact the detection
results, but based on my educated guess, it shouldn't. So please go head. Let me
know when your code is landed in stable. I will keep an eye on safe browsing
dashboard just in case.
> 
> Great, thanks!

Relatedly: if y'all aren't using the data (or don't know how you're using the
data?) maybe we can just remove it? :)

[Jialiu Lin, 2016-06-08 19:14:05]

On 2016/06/08 at 18:59:39, mkwst wrote:
> On 2016/06/08 at 18:45:02, Mike West (OOO until 30th) wrote:
> > On 2016/06/08 at 18:23:14, jialiul wrote:
> > > On 2016/06/07 at 08:08:42, mkwst wrote:
> > > > On 2016/06/06 at 20:43:07, creis wrote:
> > > > > On 2016/06/06 08:40:10, Mike West (OOO until 30th) wrote:
> > > > > > Charlie: It seems reasonable to me to treat blocking as a redirect
of its own (I
> > > > > > can imagine that clients of the redirects array might actually want
the URL that
> > > > > > we would have loaded if it hadn't been blocked). That approach also
lends itself
> > > > > > to the current implementation (as we append a redirect in
FrameLoader before we
> > > > > > have any idea whether or not we'll end up blocking the request.
> > > > > > 
> > > > > > Does this seem like a reasonable approach, or should we refactor the
append
> > > > > > entirely?
> > > > > 
> > > > > Hmm, that feels weird/unfortunate to me, but maybe it works.  I
thought the goal was to act like the renderer never got to the blocked URL, but
here we're making it look like it went through the blocked URL to end up at
about:blank.
> > > > > 
> > > > > That said, I don't know if anything looks at the intermediate hops of
the redirects array and infers that the renderer had access to them, and I don't
know this area well enough to recommend a better plan.  It certainly seems
better to do this than leave the blocked URL as the last item of the redirects
array, which we do assume to be equal to the committed URL in some places.  I'll
defer unless Nate has a better idea.
> > > > 
> > > > So far as I can tell, SafeBrowsing is the only client of this data (it
ends up in the `BrowseInfo` struct in `browser_feature_extractor.h`). It seems
like that will end up being more helpful than harmful, as it gives SB a signal
that a particular page was blocked. I imagine that could be useful for their
analysis.
> > > > 
> > > > +nparker@, who might have opinions.
> > > > 
> > > > If neither nparker@ nor japhet@ object, this seems like the simplest
approach, and I'd like to run with it. It's certainly an improvement over the
status quo.
> > > 
> > > Thanks for the code pointer, mkwst! It seems redirect array is used as one
of the features in client side phishing detection. That part of code is pretty
old, I guess no body knows how this feature actually impact the detection
results, but based on my educated guess, it shouldn't. So please go head. Let me
know when your code is landed in stable. I will keep an eye on safe browsing
dashboard just in case.
> > 
> > Great, thanks!
> 
> Relatedly: if y'all aren't using the data (or don't know how you're using the
data?) maybe we can just remove it? :)
Let's keep it for now. Client side phishing detection uses some sorted of
simplified machine learning model. This model is passed to chrome client in
binary format (that's why we don't know how it interacts with different
features). Removing a feature might break the model.

[commit-bot: I haz the power, 2016-06-08 21:35:02]

Description was changed from

==========
Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910
==========

to

==========
Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910
==========

[commit-bot: I haz the power, 2016-06-08 21:35:04]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-08 21:37:38]

Description was changed from

==========
Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910
==========

to

==========
Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG=615910

Committed: https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119
Cr-Commit-Position: refs/heads/master@{#398685}
==========

[commit-bot: I haz the power, 2016-06-08 21:37:40]

Patchset 5 (id:??) landed as
https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119
Cr-Commit-Position: refs/heads/master@{#398685}

[tommycli, 2016-06-13 18:13:03]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2068443002/ by tommycli@chromium.org.

The reason for reverting is: Speculative revert for breaking Dr. Memory:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%2...

First breaking build:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%2....