Renderer initiated navigations from non instant process should not fall into instant.

Renderer initiated navigations from non instant process should not fall into instant.

Currently any renderer initiated navigations to the Instant URL that are bounced to the browser will fall into Instant process. We dont want this happening for security reasons. This CL fixes that by adding a new notion of a "privileged site" with restricted entry points.

BUG=231589
COLLABORATOR=shishir@chromium.org

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

[Shishir, 2013-04-30 22:54:11]

Hi Charlie,

I have split out the changes to TemplateURL into a separate CL since it was
getting a bit large. 

PTAL.

Thanks
Shishir

[Charlie Reis, 2013-05-01 01:17:54]

Wow, cool-- I didn't realize you could upload patches to my original CL.

I think this looks pretty good if we can justify the new concept to jam@.  I
updated the CL description a bit to mention it, and I'll send a note to him.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:767: // effective URL distinct
from other URLs on the search provider's domain.
s/effective/privileged/

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:769: return
GetEffectiveURLForInstant(url, profile);
We should rename this to GetPrivilegedURLForInstant, since we're no longer using
the "effective URL" system.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1882: // Dont
use https server for the non instant URL so that the browser uses
nit: Don't

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1883: //
differnt RenderViews.
nit: different

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1910: = true;
nit: Move = to previous line (should fit, and more consistent with elsewhere).

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1933: // Instant
page end up in an Instant process.
nit: // Instant process

https://codereview.chromium.org/13877028/diff/11001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/13877028/diff/11001/content/public/browser/co...
content/public/browser/content_browser_client.h:150: // sites embedd this info.
This comment is the most important thing to get right, because it's introducing
a new concept: a privileged site.  We don't have this notion defined anywhere
else yet.  How about this?

Determines whether a navigation from |current_instance| to |url| would be a
valid entry point to a "privileged site," based on whether it
|is_renderer_initiated|.  A privileged site requires careful process isolation
to ensure its privileges do not leak, and it can only be entered via known
paths.

If this is a valid entry to a privileged site, this function should rewrite the
origin of |url| with a non-http(s) origin that represents the privileged site. 
This will distinguish the resulting SiteInstance from other SiteInstances in the
process model.

https://codereview.chromium.org/13877028/diff/11001/content/public/browser/co...
content/public/browser/content_browser_client.h:155: SiteInstance*
curr_instance);
nit: current_instance

(Since this is a public API, we may as well spell it out everywhere.  Sorry for
my earlier abbreviation.)

[Shishir, 2013-05-01 07:56:14]

Adding jam for content API changes.

PAL.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:767: // effective URL distinct
from other URLs on the search provider's domain.
On 2013/05/01 01:17:54, creis wrote:
> s/effective/privileged/

Done.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:769: return
GetEffectiveURLForInstant(url, profile);
On 2013/05/01 01:17:54, creis wrote:
> We should rename this to GetPrivilegedURLForInstant, since we're no longer
using
> the "effective URL" system.

Done.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1882: // Dont
use https server for the non instant URL so that the browser uses
On 2013/05/01 01:17:54, creis wrote:
> nit: Don't

Done.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1883: //
differnt RenderViews.
On 2013/05/01 01:17:54, creis wrote:
> nit: different

Done.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1910: = true;
On 2013/05/01 01:17:54, creis wrote:
> nit: Move = to previous line (should fit, and more consistent with elsewhere).

Done.

https://codereview.chromium.org/13877028/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1933: // Instant
page end up in an Instant process.
On 2013/05/01 01:17:54, creis wrote:
> nit: // Instant process

Done.

https://codereview.chromium.org/13877028/diff/11001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/13877028/diff/11001/content/public/browser/co...
content/public/browser/content_browser_client.h:150: // sites embedd this info.
On 2013/05/01 01:17:54, creis wrote:
> This comment is the most important thing to get right, because it's
introducing
> a new concept: a privileged site.  We don't have this notion defined anywhere
> else yet.  How about this?
> 
> Determines whether a navigation from |current_instance| to |url| would be a
> valid entry point to a "privileged site," based on whether it
> |is_renderer_initiated|.  A privileged site requires careful process isolation
> to ensure its privileges do not leak, and it can only be entered via known
> paths.
> 
> If this is a valid entry to a privileged site, this function should rewrite
the
> origin of |url| with a non-http(s) origin that represents the privileged site.

> This will distinguish the resulting SiteInstance from other SiteInstances in
the
> process model.

Done.

https://codereview.chromium.org/13877028/diff/11001/content/public/browser/co...
content/public/browser/content_browser_client.h:155: SiteInstance*
curr_instance);
On 2013/05/01 01:17:54, creis wrote:
> nit: current_instance
> 
> (Since this is a public API, we may as well spell it out everywhere.  Sorry
for
> my earlier abbreviation.)

Done.

[Charlie Reis, 2013-05-01 16:50:06]

LGTM with nit.

https://codereview.chromium.org/13877028/diff/25003/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/13877028/diff/25003/content/public/browser/co...
content/public/browser/content_browser_client.h:145: 
nit: Remove extra blank line.

[sreeram, 2013-05-01 18:00:57]

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:764: is_instant_process =
instant_service->IsInstantProcess(process_id);
Do we need to check process ID? Couldn't we just check that |current_instance|
has the "chrome-search://" scheme?

[Charlie Reis, 2013-05-01 18:05:29]

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:764: is_instant_process =
instant_service->IsInstantProcess(process_id);
On 2013/05/01 18:00:57, sreeram wrote:
> Do we need to check process ID? Couldn't we just check that |current_instance|
> has the "chrome-search://" scheme?

In theory they should be equivalent.  I'd probably sleep better if we checked
for both.  :)

[Shishir, 2013-05-01 18:13:52]

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:764: is_instant_process =
instant_service->IsInstantProcess(process_id);
On 2013/05/01 18:00:57, sreeram wrote:
> Do we need to check process ID? Couldn't we just check that |current_instance|
> has the "chrome-search://" scheme?

Yes, they should be exactly the same. One subtle thing to note is that site gets
assigned to a SiteInstance earlier than the process (which is when the
InstantService is updated). I don't think it matters in our case, and we could
possibly dcheck that the origin is the privileged instant URL.

[sreeram, 2013-05-01 18:35:17]

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:764: is_instant_process =
instant_service->IsInstantProcess(process_id);
On 2013/05/01 18:13:52, Shishir wrote:
> On 2013/05/01 18:00:57, sreeram wrote:
> > Do we need to check process ID? Couldn't we just check that
|current_instance|
> > has the "chrome-search://" scheme?
> 
> Yes, they should be exactly the same. One subtle thing to note is that site
gets
> assigned to a SiteInstance earlier than the process (which is when the
> InstantService is updated). I don't think it matters in our case, and we could
> possibly dcheck that the origin is the privileged instant URL.

Right, but that's for the _new_ SiteInstance, right? At the time we use
|current_instance|, we shouldn't be dealing with that situation (where the
process hasn't been assigned yet). Right?

So let's do both checks, as Charlie suggested. And DCHECK() that both checks
give the same value.

[Shishir, 2013-05-01 19:11:11]

PTAL.

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/25003/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:764: is_instant_process =
instant_service->IsInstantProcess(process_id);
On 2013/05/01 18:35:18, sreeram wrote:
> On 2013/05/01 18:13:52, Shishir wrote:
> > On 2013/05/01 18:00:57, sreeram wrote:
> > > Do we need to check process ID? Couldn't we just check that
> |current_instance|
> > > has the "chrome-search://" scheme?
> > 
> > Yes, they should be exactly the same. One subtle thing to note is that site
> gets
> > assigned to a SiteInstance earlier than the process (which is when the
> > InstantService is updated). I don't think it matters in our case, and we
could
> > possibly dcheck that the origin is the privileged instant URL.
> 
> Right, but that's for the _new_ SiteInstance, right? At the time we use
> |current_instance|, we shouldn't be dealing with that situation (where the
> process hasn't been assigned yet). Right?
> 
> So let's do both checks, as Charlie suggested. And DCHECK() that both checks
> give the same value.

Done.

https://codereview.chromium.org/13877028/diff/25003/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/13877028/diff/25003/content/public/browser/co...
content/public/browser/content_browser_client.h:145: 
On 2013/05/01 16:50:06, creis wrote:
> nit: Remove extra blank line.

Done.

[sreeram, 2013-05-01 19:17:45]

lgtm

https://codereview.chromium.org/13877028/diff/42002/chrome/browser/search/sea...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/13877028/diff/42002/chrome/browser/search/sea...
chrome/browser/search/search.h:138: // If the input is already an privileged URL
then that same URL is returned.
an -> a

[jam, 2013-05-01 21:42:37]

lgtm

[Shishir, 2013-05-01 21:53:49]

Fixed Toolbarmodel test that was failing.

https://codereview.chromium.org/13877028/diff/42002/chrome/browser/search/sea...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/13877028/diff/42002/chrome/browser/search/sea...
chrome/browser/search/search.h:138: // If the input is already an privileged URL
then that same URL is returned.
On 2013/05/01 19:17:45, sreeram wrote:
> an -> a

Done.

[sreeram, 2013-05-01 21:59:50]

slgtm

https://codereview.chromium.org/13877028/diff/43004/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/13877028/diff/43004/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:157: void
ResetTemplateURLForInstant(const GURL instant_url) {
const GURL&

https://codereview.chromium.org/13877028/diff/43004/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:213: // The URL being
navigate to should be treated as the Instant URL. Else
navigate -> navigated

[Shishir, 2013-05-01 23:42:16]

https://codereview.chromium.org/13877028/diff/43004/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/13877028/diff/43004/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:157: void
ResetTemplateURLForInstant(const GURL instant_url) {
On 2013/05/01 21:59:50, sreeram wrote:
> const GURL&

Done.

https://codereview.chromium.org/13877028/diff/43004/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:213: // The URL being
navigate to should be treated as the Instant URL. Else
On 2013/05/01 21:59:50, sreeram wrote:
> navigate -> navigated

Done.

[Shishir, 2013-05-02 17:21:06]

Commented out DCHECK as PolicyCheck.ReplaceSearchTerms currently violates it.
Added TODO.

[Charlie Reis, 2013-05-02 17:25:20]

https://codereview.chromium.org/13877028/diff/56001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/56001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:735: // TODO(shishir): Uncomment
the followng DCHECK after tests are fixed.
Is the ReplaceSearchTerms CL difficult to fix?  It would be much better to just
fix it and land this with the DCHECK in place if possible.

If that's going to put M28 at risk, then we should at least call out the
specific failing test in the TODO (and fix the typo in "followng").

[Charlie Reis, 2013-05-02 17:25:55]

On 2013/05/02 17:25:20, creis wrote:
> Is the ReplaceSearchTerms CL difficult to fix?  It would be much better to
just

s/CL/test/, of course.

[Shishir, 2013-05-02 18:07:06]

Fixed the tests, and on the trybots.

https://codereview.chromium.org/13877028/diff/56001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/56001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:735: // TODO(shishir): Uncomment
the followng DCHECK after tests are fixed.
On 2013/05/02 17:25:20, creis wrote:
> Is the ReplaceSearchTerms CL difficult to fix?  It would be much better to
just
> fix it and land this with the DCHECK in place if possible.
> 
> If that's going to put M28 at risk, then we should at least call out the
> specific failing test in the TODO (and fix the typo in "followng").


Fixed the test. Turns out the code that caused this DCHECK to fail was not even
necessary.

[Charlie Reis, 2013-05-02 18:33:02]

https://codereview.chromium.org/13877028/diff/56001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/13877028/diff/56001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:735: // TODO(shishir): Uncomment
the followng DCHECK after tests are fixed.
On 2013/05/02 18:07:07, Shishir wrote:
> On 2013/05/02 17:25:20, creis wrote:
> > Is the ReplaceSearchTerms CL difficult to fix?  It would be much better to
> just
> > fix it and land this with the DCHECK in place if possible.
> > 
> > If that's going to put M28 at risk, then we should at least call out the
> > specific failing test in the TODO (and fix the typo in "followng").
> 
> 
> Fixed the test. Turns out the code that caused this DCHECK to fail was not
even
> necessary.

Fantastic!  LGTM.

[commit-bot: I haz the power, 2013-05-02 21:47:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/13877028/67001

[commit-bot: I haz the power, 2013-05-02 22:17:13]

Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-02 22:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/13877028/67001

[commit-bot: I haz the power, 2013-05-02 23:40:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/13877028/89001

[commit-bot: I haz the power, 2013-05-03 04:21:23]

Change committed as 198051