signin: force web signin flow initiated visits to accounts.google.com to their own process.

signin: force web signin flow initiated visits to accounts.google.com to their own process.

BUG=81265, 180062, 180064, 180067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186550

[Roger Tawa OOO till Jul 10th, 2013-02-28 04:07:42]

Looks good Tim.  A couple of questions below.  I'll patch this in myself and do
some testing.

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.h:203: bool IsSigninProcess(const
content::RenderProcessHost* process) const;
Any reason to pass RenderProcessHost* instead of just an int?

https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:268: false);
service_login_url() returns a URL that looks like https://a.g.c/ServiceLogin. 
If this is really what you want, then I think what you can compare the URL
ignoring any query parameters.  See
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/sync/on...
for an example.

However, the chain of redirects during a sign in process could include something
else under https://a.g.c.  See
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/sync/on...,
minus the checks for youtube and blogger.  I'll do some testing myself and see.

[Roger Tawa OOO till Jul 10th, 2013-02-28 15:04:52]

https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:262: bool
IsWebBasedSigninFlowURL(const GURL& url) {
Missing SyncPromoUI::, get unresolved external.

[Roger Tawa OOO till Jul 10th, 2013-02-28 15:17:34]

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.cc:913: }
missing break; statement

[Roger Tawa OOO till Jul 10th, 2013-02-28 16:57:33]

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.cc:211: CHECK_EQ(signin_process_id_,
kInvalidProcessId);
Hit this DCHECK while trying to repro crbug.com/178701

[Charlie Reis, 2013-02-28 17:33:02]

Just a quick comment.  I'll take a closer look later today.

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.cc:215:
content::Source<content::RenderProcessHost>(process));
You can look up the RPH from the ID, using RenderProcessHost::FromID.  That way
you don't need it in the API.

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.h:203: bool IsSigninProcess(const
content::RenderProcessHost* process) const;
On 2013/02/28 04:07:42, Roger Tawa wrote:
> Any reason to pass RenderProcessHost* instead of just an int?

Yes, I think the ID would be sufficient here, and consistent with similar checks
elsewhere.

[tim (not reviewing), 2013-02-28 17:35:52]

Thanks for the comments guys -- I hadn't totally polished this up for
review yet fyi but thought I'd get the gist of it in front of you as early
as possible :)  I also seem to have uploaded a bad patch (with the link
issues roger points out).  I'll update something better shortly!


On Thu, Feb 28, 2013 at 9:33 AM, <creis@chromium.org> wrote:

> Just a quick comment.  I'll take a closer look later today.
>
>
>
> https://codereview.chromium.**org/12374007/diff/1/chrome/**
>
browser/signin/signin_manager.**cc<https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_manager.cc>
> File chrome/browser/signin/signin_**manager.cc (right):
>
> https://codereview.chromium.**org/12374007/diff/1/chrome/**
>
browser/signin/signin_manager.**cc#newcode215<https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_manager.cc#newcode215>
> chrome/browser/signin/signin_**manager.cc:215:
> content::Source<content::**RenderProcessHost>(process));
> You can look up the RPH from the ID, using RenderProcessHost::FromID.
> That way you don't need it in the API.
>
>
> https://codereview.chromium.**org/12374007/diff/1/chrome/**
>
browser/signin/signin_manager.**h<https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_manager.h>
> File chrome/browser/signin/signin_**manager.h (right):
>
> https://codereview.chromium.**org/12374007/diff/1/chrome/**
>
browser/signin/signin_manager.**h#newcode203<https://codereview.chromium.org/12374007/diff/1/chrome/browser/signin/signin_manager.h#newcode203>
> chrome/browser/signin/signin_**manager.h:203: bool IsSigninProcess(const
> content::RenderProcessHost* process) const;
> On 2013/02/28 04:07:42, Roger Tawa wrote:
>
>> Any reason to pass RenderProcessHost* instead of just an int?
>>
>
> Yes, I think the ID would be sufficient here, and consistent with
> similar checks elsewhere.
>
>
https://codereview.chromium.**org/12374007/<https://codereview.chromium.org/1...
>

[Roger Tawa OOO till Jul 10th, 2013-02-28 18:23:17]

https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:556: return false;
This check should only be done if there is a valid email.  CanOffer() is called
from BrowserTabContents::AttachTabHelpers() to know if a OneClickSigninHelper
should even be instantiated, and does not pass in an email address.

Move this if statement to line 559 below.

[tim (not reviewing), 2013-02-28 18:45:29]

On 2013/02/28 18:23:17, Roger Tawa wrote:
>
https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/sync/one_cl...
> File chrome/browser/ui/sync/one_click_signin_helper.cc (right):
> 
>
https://codereview.chromium.org/12374007/diff/1/chrome/browser/ui/sync/one_cl...
> chrome/browser/ui/sync/one_click_signin_helper.cc:556: return false;
> This check should only be done if there is a valid email.  CanOffer() is
called
> from BrowserTabContents::AttachTabHelpers() to know if a OneClickSigninHelper
> should even be instantiated, and does not pass in an email address.
> 
> Move this if statement to line 559 below.

Responded to the existing comments, please keep 'em coming.  I'm currently
looking into how I might test this.

[Charlie Reis, 2013-02-28 19:19:46]

Cool.  This seems like it's on the right track.

The main remaining question I have is whether the renderer process will know
that it needs to force a swap for link clicks (etc) outside the sign-in flow. 
I'm concerned that ShouldFork will need to know that it has to force a swap, so
we might have to pass a command line argument (like the Instant CL) after all.

If you try clicking a non-sign-in link in the renderer, does it stay in the same
process?

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:716: if
(SyncPromoUI::IsWebBasedSigninFlowURL(url))
Please add a comment similar to the one for Instant above, along with a reason
(i.e., that process will be allowed to sign the user into Chrome).

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:956: if
(SyncPromoUI::IsWebBasedSigninFlowURL(site_instance->GetSiteURL())) {
Can you add a comment here saying that we only expect there to be one of these
processes (since we use process-per-site for it), and that it will be cleared
when the process is deleted?

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:212: CHECK_EQ(signin_process_id_,
kInvalidProcessId);
I don't think we can enforce this CHECK.  There are races with process-per-site
that make it possible (though unlikely) that we could have two processes.  In
that case, only the most recent will be allowed to sign the user in.

I don't expect this to happen in practice (given how this URL is used), but we
shouldn't crash if it does.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:910: DCHECK_EQ(signin_process_id_,
Similar to above, this should be an if rather than a DCHECK.

We should remove the process from the registrar either way, of course.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:557: if
(!manager->IsSigninProcess(
Can you add a comment here, to the effect of "We only allow the dedicated
sign-in process to sign the user into Chrome, because it doesn't load any
untrusted pages."

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.h (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.h:82: static bool
IsWebBasedSigninFlowURL(const GURL& url);
Can you document this?

https://codereview.chromium.org/12374007/diff/2005/chrome/common/chrome_conte...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/common/chrome_conte...
chrome/common/chrome_content_client.cc:385:
standard_schemes->push_back(chrome::kChromeSigninScheme);
Hmm, what's the difference between standard_schemes and savable_schemes?  I
notice that the chrome-search guys are adding it to savable_schemes in a future
CL.

https://codereview.chromium.org/12374007/diff/2005/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/common/url_constant...
chrome/common/url_constants.h:416: extern const char kChromeSigninScheme[];
Please document why we have this, and that we do not want to grant it WebUI
bindings (since that's not always clear).

[Roger Tawa OOO till Jul 10th, 2013-02-28 21:13:05]

lgtm

I tried all the sign in cases (NTP, wrench, settings, one-click with gmail) and
all worked fine.  I tried with accounts that have 2 factor enabled and disabled.
 I also tried "sign in with google+" as more of an adge case, and it worked OK
too.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:268: false);
Nit: I think it might be better to compare using GURL::ReplaceComponents() to
strip off the query part, but up to you if you want to change.

[tim (not reviewing), 2013-03-01 01:53:57]

Updated to address comments and fork renderers.  Note crbug.com/101395 affects
us here as the signin page performs a POST, but it is to accounts.google.com, so
I think we're okay. If you click on any of the links to
non-"accounts.google.com" on the signin page, it will now fork to a new process.

Still working on test.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:716: if
(SyncPromoUI::IsWebBasedSigninFlowURL(url))
On 2013/02/28 19:19:46, creis wrote:
> Please add a comment similar to the one for Instant above, along with a reason
> (i.e., that process will be allowed to sign the user into Chrome).

Done.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:956: if
(SyncPromoUI::IsWebBasedSigninFlowURL(site_instance->GetSiteURL())) {
On 2013/02/28 19:19:46, creis wrote:
> Can you add a comment here saying that we only expect there to be one of these
> processes (since we use process-per-site for it), and that it will be cleared
> when the process is deleted?

Done.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:212: CHECK_EQ(signin_process_id_,
kInvalidProcessId);
On 2013/02/28 19:19:46, creis wrote:
> I don't think we can enforce this CHECK.  There are races with
process-per-site
> that make it possible (though unlikely) that we could have two processes.  In
> that case, only the most recent will be allowed to sign the user in.
> 
> I don't expect this to happen in practice (given how this URL is used), but we
> shouldn't crash if it does.

Done.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:910: DCHECK_EQ(signin_process_id_,
On 2013/02/28 19:19:46, creis wrote:
> Similar to above, this should be an if rather than a DCHECK.
> 
> We should remove the process from the registrar either way, of course.

Done.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:557: if
(!manager->IsSigninProcess(
On 2013/02/28 19:19:46, creis wrote:
> Can you add a comment here, to the effect of "We only allow the dedicated
> sign-in process to sign the user into Chrome, because it doesn't load any
> untrusted pages."

Done.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:268: false);
On 2013/02/28 21:13:06, Roger Tawa wrote:
> Nit: I think it might be better to compare using GURL::ReplaceComponents() to
> strip off the query part, but up to you if you want to change.

Yeah I didn't intend on committing as is, but I changed to use GetOrigin and
path() equality.

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.h (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.h:82: static bool
IsWebBasedSigninFlowURL(const GURL& url);
On 2013/02/28 19:19:46, creis wrote:
> Can you document this?

Done.

https://codereview.chromium.org/12374007/diff/2005/chrome/common/chrome_conte...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/common/chrome_conte...
chrome/common/chrome_content_client.cc:385:
standard_schemes->push_back(chrome::kChromeSigninScheme);
On 2013/02/28 19:19:46, creis wrote:
> Hmm, what's the difference between standard_schemes and savable_schemes?  I
> notice that the chrome-search guys are adding it to savable_schemes in a
future
> CL.

I'm not sure, but I still only see search in the 'standard' category, link?

https://codereview.chromium.org/12374007/diff/2005/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/common/url_constant...
chrome/common/url_constants.h:416: extern const char kChromeSigninScheme[];
On 2013/02/28 19:19:46, creis wrote:
> Please document why we have this, and that we do not want to grant it WebUI
> bindings (since that's not always clear).

Done.

[Charlie Reis, 2013-03-04 19:22:24]

Getting close.  I have a few other questions I'll run past you offline as well.

https://codereview.chromium.org/12374007/diff/2005/chrome/common/chrome_conte...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/12374007/diff/2005/chrome/common/chrome_conte...
chrome/common/chrome_content_client.cc:385:
standard_schemes->push_back(chrome::kChromeSigninScheme);
On 2013/03/01 01:53:57, timsteele wrote:
> On 2013/02/28 19:19:46, creis wrote:
> > Hmm, what's the difference between standard_schemes and savable_schemes?  I
> > notice that the chrome-search guys are adding it to savable_schemes in a
> future
> > CL.
> 
> I'm not sure, but I still only see search in the 'standard' category, link?

Their CL hasn't landed yet.  It looks like this gates whether the user can save
the page to disk.  I don't see any reason to disallow that, so we should follow
convention and add it to both.

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:72: GURL(),
SyncPromoUI::SOURCE_NTP_LINK, true));
nit: wrong indent

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:77: // unsuitable host, such as a
WebUI tab hosting the new tab page.
I don't understand what this means.  Won't we always create a privileged
renderer, including on navigations from normal web pages?

Can you test for that?

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:89:
EXPECT_TRUE(signin->IsSigninProcess(active_tab_process_id));
Can you add this line to verify that it doesn't have WebUI bindings?

EXPECT_EQ(0, active_tab->GetRenderViewHost()->GetEnabledBindings());

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.h:201: // OneClickSigninHelper).
nit: Mention that this resets if the process terminates.

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:558:
web_contents->GetRenderProcessHost()->GetID())) {
nit: 4 more spaces.

https://codereview.chromium.org/12374007/diff/11021/chrome/common/url_constan...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/common/url_constan...
chrome/common/url_constants.cc:526: // OneClickSigninHelper.
Great comment.  Can you move it to the .h file?

https://codereview.chromium.org/12374007/diff/11021/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:849: // process if it is a
Signin url, or to another process if not.
Yeah, I suppose there's no easy way to check whether you're doing a POST to a
valid sign-in URL or not from the renderer process.

Ok, we'll rely on the sign-in page not doing POSTs to untrusted pages until
101395 is fixed.  Can you add something to the comment saying why this check has
to be after the POST check?

[tim (not reviewing), 2013-03-04 23:40:18]

Comments addressed, PTAL.

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:72: GURL(),
SyncPromoUI::SOURCE_NTP_LINK, true));
On 2013/03/04 19:22:24, creis wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:89:
EXPECT_TRUE(signin->IsSigninProcess(active_tab_process_id));
On 2013/03/04 19:22:24, creis wrote:
> Can you add this line to verify that it doesn't have WebUI bindings?
> 
> EXPECT_EQ(0, active_tab->GetRenderViewHost()->GetEnabledBindings());

Done.

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.h:201: // OneClickSigninHelper).
On 2013/03/04 19:22:24, creis wrote:
> nit: Mention that this resets if the process terminates.

Done.

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:558:
web_contents->GetRenderProcessHost()->GetID())) {
On 2013/03/04 19:22:24, creis wrote:
> nit: 4 more spaces.

Done.

https://codereview.chromium.org/12374007/diff/11021/chrome/common/url_constan...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/common/url_constan...
chrome/common/url_constants.cc:526: // OneClickSigninHelper.
On 2013/03/04 19:22:24, creis wrote:
> Great comment.  Can you move it to the .h file?

Done.

https://codereview.chromium.org/12374007/diff/11021/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12374007/diff/11021/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:849: // process if it is a
Signin url, or to another process if not.
On 2013/03/04 19:22:24, creis wrote:
> Yeah, I suppose there's no easy way to check whether you're doing a POST to a
> valid sign-in URL or not from the renderer process.
> 
> Ok, we'll rely on the sign-in page not doing POSTs to untrusted pages until
> 101395 is fixed.  Can you add something to the comment saying why this check
has
> to be after the POST check?

I added a CHECK to my if-block to ensure we never fork on POST without doing
something more careful here.

[Charlie Reis, 2013-03-05 00:28:47]

LGTM with nit.

You may need to resolve a merge conflict in chrome_content_client.cc with
https://chromiumcodereview.appspot.com/11896113/, depending on who lands first.

Can you file bugs for the other restrictions we discussed and CC me on them? 
(Lock down to HTTPS only, intercept redirects to force them out of the process,
prevent postMessage using ShouldSwapProcess.)

https://codereview.chromium.org/12374007/diff/18001/chrome/common/url_constan...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/12374007/diff/18001/chrome/common/url_constan...
chrome/common/url_constants.cc:526: // OneClickSigninHelper.
nit: You don't need this comment, now that it's in the .h file.

[Charlie Reis, 2013-03-05 00:29:58]

[+cevans in CC as FYI]

Chris, just wanted to give you a heads up that we're moving ahead with process
isolation for this, if you want to see the approach we're taking.

[commit-bot: I haz the power, 2013-03-05 00:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/12374007/14029

[commit-bot: I haz the power, 2013-03-05 00:45:09]

Presubmit check for 12374007-14029 failed and returned exit status 1.

INFO:root:Found 17 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/renderer/chrome_content_renderer_client.cc
    chrome/test/base/chrome_render_view_host_test_harness.cc
    chrome/common/chrome_content_client.cc

Presubmit checks took 1.8s to calculate.

[tim (not reviewing), 2013-03-05 00:49:42]

Guess I misinterpreted the OWNERS file layout :(  +jam for OWNERS approval of
chrome/content, chrome/renderer, and chrome/test.

[darin (slow to review), 2013-03-05 07:29:56]

Do you mean for chrome-signin: URLs to be linkable from the web?

https://codereview.chromium.org/12374007/diff/14029/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12374007/diff/14029/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:851: // We explicitly do this
*after* checking that http_method is not POST,
nit: I'm confused by this comment.  It seems like this code will only matter
once we fix http://crbug.com/101395.

[tim (not reviewing), 2013-03-05 09:45:25]

Re: the comment about chrome-signin URLs being linkable from the web: that is
not my intent.  I'm trying to mimic the chrome-search scheme, and make
chrome-signin be a differentiator for un-privileged URLs on accounts.google.com.
 Let me know if I mixed things up in the CL or that doesn't jive with your
understanding.  

Thanks!
Tim

https://codereview.chromium.org/12374007/diff/14029/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12374007/diff/14029/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:851: // We explicitly do this
*after* checking that http_method is not POST,
On 2013/03/05 07:29:56, darin wrote:
> nit: I'm confused by this comment.  It seems like this code will only matter
> once we fix http://crbug.com/101395.

You're right, because the if-statement on line 844 above will prevent us from
ever getting here with http_method == POST.  However, it seemed prudent to add
an extra check in the event that bug 101395 is fixed, so that we're forced to
think of this case before opening the floodgates and have to add the ability to
check whether we're posting to a valid signin URL.  

Does that sound right to you?

[darin (slow to review), 2013-03-05 17:16:10]

On Tue, Mar 5, 2013 at 1:45 AM, <tim@chromium.org> wrote:

> Re: the comment about chrome-signin URLs being linkable from the web: that
> is
> not my intent.  I'm trying to mimic the chrome-search scheme, and make
> chrome-signin be a differentiator for un-privileged URLs on
> accounts.google.com.
>  Let me know if I mixed things up in the CL or that doesn't jive with your
> understanding.
>
>
Please test that you cannot link to these URLs from the web.  I think you
probably
need to call some methods on WebKit::WebSecurityPolicy to disable linking.
 I
recommend having Adam Barth review your settings there.  It is easy to make
mistakes when introducing new URL schemes that correspond to privileged
content.



> Thanks!
> Tim
>
>
>
> https://codereview.chromium.**org/12374007/diff/14029/**
>
chrome/renderer/chrome_**content_renderer_client.cc<https://codereview.chromium.org/12374007/diff/14029/chrome/renderer/chrome_content_renderer_client.cc>
> File chrome/renderer/chrome_**content_renderer_client.cc (right):
>
> https://codereview.chromium.**org/12374007/diff/14029/**
>
chrome/renderer/chrome_**content_renderer_client.cc#**newcode851<https://codereview.chromium.org/12374007/diff/14029/chrome/renderer/chrome_content_renderer_client.cc#newcode851>
> chrome/renderer/chrome_**content_renderer_client.cc:**851: // We
> explicitly
> do this *after* checking that http_method is not POST,
> On 2013/03/05 07:29:56, darin wrote:
>
>> nit: I'm confused by this comment.  It seems like this code will only
>>
> matter
>
>> once we fix http://crbug.com/101395.
>>
>
> You're right, because the if-statement on line 844 above will prevent us
> from ever getting here with http_method == POST.  However, it seemed
> prudent to add an extra check in the event that bug 101395 is fixed, so
> that we're forced to think of this case before opening the floodgates
> and have to add the ability to check whether we're posting to a valid
> signin URL.
>


Yes, that seems reasonable.  The comment doesn't say that though.
The comment seems to mostly just re-state what the code does.

-Darin


>
> Does that sound right to you?
>
>
https://codereview.chromium.**org/12374007/<https://codereview.chromium.org/1...
>

[tim (not reviewing), 2013-03-05 17:39:20]

Thanks Darin.  I took another whack at the comment in
ChromeContentRendererClient.  

I'll look into WebSecurityPolicy now.  +dhollowa FYI, as I based this patch off
of the analogous isolation CL for Instant / kInstantScheme. This might be
interesting there as well.

[darin (slow to review), 2013-03-05 18:41:05]

Also, have you explored solving this problem without introducing a new URL
scheme?  That seems like a very heavyweight solution.  Historically, we've
not needed to add new URL schemes.  (I don't fully grok why it was done for
instant.)

-Darin


On Tue, Mar 5, 2013 at 9:39 AM, <tim@chromium.org> wrote:

> Thanks Darin.  I took another whack at the comment in
> ChromeContentRendererClient.
>
> I'll look into WebSecurityPolicy now.  +dhollowa FYI, as I based this
> patch off
> of the analogous isolation CL for Instant / kInstantScheme. This might be
> interesting there as well.
>
>
https://codereview.chromium.**org/12374007/<https://codereview.chromium.org/1...
>

[Charlie Reis, 2013-03-05 19:22:01]

I agree that it would be good to do this without a new scheme if we can.  The
instant folks wanted one because they actually wanted to request data from the
scheme, but that's not the case here.

The basic requirement is that it ends up as a different site than the rest of
"https://google.com", so that the process swapping logic puts it in its own
process.

The closest analog is hosted apps.  For the Chrome Web Store, GetEffectiveURL
returns a chrome-extension://abcdef URL, which is how we give it a different
site.

We could try to mimic the CWS here?  One option is to just return a
chrome-extension://abcdef URL from GetEffectiveURL, using a hard-coded extension
ID to represent the sign-in pages.  Another option is to actually bundle it as a
hosted app with many of the same restrictions as the CWS, but that seems harder
to do.

Darin, do you have a suggestion for which route to go down?  I'm leaning towards
hard-coding an extension ID and keeping most of the rest of this CL, apart from
the new scheme.

Charlie


On 2013/03/05 18:41:05, darin wrote:
> Also, have you explored solving this problem without introducing a new URL
> scheme?  That seems like a very heavyweight solution.  Historically, we've
> not needed to add new URL schemes.  (I don't fully grok why it was done for
> instant.)
> 
> -Darin
> 
> 
> On Tue, Mar 5, 2013 at 9:39 AM, <mailto:tim@chromium.org> wrote:
> 
> > Thanks Darin.  I took another whack at the comment in
> > ChromeContentRendererClient.
> >
> > I'll look into WebSecurityPolicy now.  +dhollowa FYI, as I based this
> > patch off
> > of the analogous isolation CL for Instant / kInstantScheme. This might be
> > interesting there as well.
> >
> >
>
https://codereview.chromium.**org/12374007/%3Chttps://codereview.chromium.org...>
> >

[darin (slow to review), 2013-03-05 20:28:30]

That's an interesting idea.  If I'm not mistaken, aren't
chrome-extension:// URLs
linkable?  It seems like we might want to disable that for this use case.
 That
might be a good reason to introduce the new scheme.

-Darin


On Tue, Mar 5, 2013 at 11:22 AM, <creis@chromium.org> wrote:

> I agree that it would be good to do this without a new scheme if we can.
>  The
> instant folks wanted one because they actually wanted to request data from
> the
> scheme, but that's not the case here.
>
> The basic requirement is that it ends up as a different site than the rest
> of
> "https://google.com", so that the process swapping logic puts it in its
> own
> process.
>
> The closest analog is hosted apps.  For the Chrome Web Store,
> GetEffectiveURL
> returns a chrome-extension://abcdef URL, which is how we give it a
> different
> site.
>
> We could try to mimic the CWS here?  One option is to just return a
> chrome-extension://abcdef URL from GetEffectiveURL, using a hard-coded
> extension
> ID to represent the sign-in pages.  Another option is to actually bundle
> it as a
> hosted app with many of the same restrictions as the CWS, but that seems
> harder
> to do.
>
> Darin, do you have a suggestion for which route to go down?  I'm leaning
> towards
> hard-coding an extension ID and keeping most of the rest of this CL, apart
> from
> the new scheme.
>
> Charlie
>
>
>
> On 2013/03/05 18:41:05, darin wrote:
>
>> Also, have you explored solving this problem without introducing a new URL
>> scheme?  That seems like a very heavyweight solution.  Historically, we've
>> not needed to add new URL schemes.  (I don't fully grok why it was done
>> for
>> instant.)
>>
>
>  -Darin
>>
>
>
>  On Tue, Mar 5, 2013 at 9:39 AM, <mailto:tim@chromium.org> wrote:
>>
>
>  > Thanks Darin.  I took another whack at the comment in
>> > ChromeContentRendererClient.
>> >
>> > I'll look into WebSecurityPolicy now.  +dhollowa FYI, as I based this
>> > patch off
>> > of the analogous isolation CL for Instant / kInstantScheme. This might
>> be
>> > interesting there as well.
>> >
>> >
>>
>
> https://codereview.chromium.****org/12374007/%3Chttps://codere**
> view.chromium.org/12374007/ <http://codereview.chromium.org/12374007/>>
>
>> >
>>
>
>
>
>
https://codereview.chromium.**org/12374007/<https://codereview.chromium.org/1...
>

[tim (not reviewing), 2013-03-05 21:00:54]

I'm not sure about the link-ability of chrome-extension urls... any ideas on who
I could ping to find out?  

As for the scheme approach, am I correct in thinking there is benefit in
actually *not* calling in to WebSecurityPolicy and registering chrome-signin in
any official way, so that the majority of the code treats it as if it is an
unrecognized / bogus scheme (everything except for the GetEffectiveURL path I'm
writing / testing for)?

I added tests to ChildProcessSecurityPolicyTest to verify that it is not a web
safe scheme, however.

[tim (not reviewing), 2013-03-05 21:03:12]

[+cc abarth again]

[Charlie Reis, 2013-03-05 21:07:38]

On 2013/03/05 21:00:54, timsteele wrote:
> I'm not sure about the link-ability of chrome-extension urls... any ideas on
who
> I could ping to find out?  
> 
> As for the scheme approach, am I correct in thinking there is benefit in
> actually *not* calling in to WebSecurityPolicy and registering chrome-signin
in
> any official way, so that the majority of the code treats it as if it is an
> unrecognized / bogus scheme (everything except for the GetEffectiveURL path
I'm
> writing / testing for)?
> 
> I added tests to ChildProcessSecurityPolicyTest to verify that it is not a web
> safe scheme, however.

chrome-extension URLs are definitely linkable.  You can link to the Chrome Web
Store, for example.

I'm not convinced that's a bad thing, as long as we require you to enter on the
interstitial page and not the form POST URL.  Since we don't have support for
cross-process POST, this seems plausible.  The worst a web page can do is send
you to the page (in the privileged process) asking you to sign in, not do the
sign-in yourself.  This is the same model we have for the Chrome Web Store.

I'm personally leaning towards that model over the new scheme.

[darin (slow to review), 2013-03-05 21:12:38]

I'm OK with that if you think it is safe.  I was just wondering if we
shouldn't take the opportunity to tighten the screws as far as possible.
 If we don't need to support link-ability then don't support it.  Usually
security exploits involve a combination of unintended capabilities.

-Darin


On Tue, Mar 5, 2013 at 1:07 PM, <creis@chromium.org> wrote:

> On 2013/03/05 21:00:54, timsteele wrote:
>
>> I'm not sure about the link-ability of chrome-extension urls... any ideas
>> on
>>
> who
>
>> I could ping to find out?
>>
>
>  As for the scheme approach, am I correct in thinking there is benefit in
>> actually *not* calling in to WebSecurityPolicy and registering
>> chrome-signin
>>
> in
>
>> any official way, so that the majority of the code treats it as if it is
>> an
>> unrecognized / bogus scheme (everything except for the GetEffectiveURL
>> path
>>
> I'm
>
>> writing / testing for)?
>>
>
>  I added tests to ChildProcessSecurityPolicyTest to verify that it is not
>> a web
>> safe scheme, however.
>>
>
> chrome-extension URLs are definitely linkable.  You can link to the Chrome
> Web
> Store, for example.
>
> I'm not convinced that's a bad thing, as long as we require you to enter
> on the
> interstitial page and not the form POST URL.  Since we don't have support
> for
> cross-process POST, this seems plausible.  The worst a web page can do is
> send
> you to the page (in the privileged process) asking you to sign in, not do
> the
> sign-in yourself.  This is the same model we have for the Chrome Web Store.
>
> I'm personally leaning towards that model over the new scheme.
>
>
https://codereview.chromium.**org/12374007/<https://codereview.chromium.org/1...
>

[Charlie Reis, 2013-03-05 21:40:08]

On 2013/03/05 21:12:38, darin wrote:
> I'm OK with that if you think it is safe.  I was just wondering if we
> shouldn't take the opportunity to tighten the screws as far as possible.
>  If we don't need to support link-ability then don't support it.  Usually
> security exploits involve a combination of unintended capabilities.
> 
> -Darin
> 

I agree that locking it down and limiting who can navigate to these URLs is
desirable.  Maybe we can find a way to do it without resorting to a new scheme.

[tim (not reviewing), 2013-03-05 23:04:04]

On 2013/03/05 21:40:08, creis wrote:
> On 2013/03/05 21:12:38, darin wrote:
> > I'm OK with that if you think it is safe.  I was just wondering if we
> > shouldn't take the opportunity to tighten the screws as far as possible.
> >  If we don't need to support link-ability then don't support it.  Usually
> > security exploits involve a combination of unintended capabilities.
> > 
> > -Darin
> > 
> 
> I agree that locking it down and limiting who can navigate to these URLs is
> desirable.  Maybe we can find a way to do it without resorting to a new
scheme.

I chatted with Charlie and updated to use a chrome-extension:// URL instead
(using a dummy extension I manually packed and inspected). We'll look at
limiting redirects next / in a follow up change, we'll have to anyway if we want
to support the GMail interstitial signin flow.

Darin + Charlie, PTAL. Thanks.

[Charlie Reis, 2013-03-06 00:14:53]

https://codereview.chromium.org/12374007/diff/32001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/12374007/diff/32001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:76: // and normal renderers are
suitable hosts.
How did this change?

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

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:706: return
GURL(SyncPromoUI::kChromeSigninEffectiveURL);
This seems like it would cause problems, since you're forgetting the rest of the
path.  How would we distinguish between the interstitial and action URLs?

I was picturing something like chrome-extension://abcdef/path/from/url, which is
how we treat hosted apps.

Does it work as is?

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:99: // Under the covers, we
use a dummy chrome-extension to serve the purposes
nit: chrome-extension URL
(We don't have an actual extension object for it, which is what it sounds like
in the comment.)

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:268: if (url ==
GURL(kChromeSigninEffectiveURL))
You should check the scheme and host.

[tim (not reviewing), 2013-03-06 01:04:07]

https://codereview.chromium.org/12374007/diff/32001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_browsertest.cc (right):

https://codereview.chromium.org/12374007/diff/32001/chrome/browser/signin/sig...
chrome/browser/signin/signin_browsertest.cc:76: // and normal renderers are
suitable hosts.
On 2013/03/06 00:14:53, creis wrote:
> How did this change?

This changed because chrome-extension requires a different
RenderProcessHostPrivilege, and trying to load chrome-extension URLs in a normal
renderer is not permitted by IsSuitableHost.  chrome-signin was permitted,
because it wasn't a valid URL, and we use normal renderers for invalid URLs (see
GetPrivilegeRequiredByUrl).

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

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:706: return
GURL(SyncPromoUI::kChromeSigninEffectiveURL);
On 2013/03/06 00:14:54, creis wrote:
> This seems like it would cause problems, since you're forgetting the rest of
the
> path.  How would we distinguish between the interstitial and action URLs?
> 
> I was picturing something like chrome-extension://abcdef/path/from/url, which
is
> how we treat hosted apps.
> 
> Does it work as is?

The happy case worked as is, although it does seem better to keep the path
around.  Changed (and renamed the SyncPromoUI string to
'kChromeSigninEffectiveOrigin' as that seems more appropriate).

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:99: // Under the covers, we
use a dummy chrome-extension to serve the purposes
On 2013/03/06 00:14:54, creis wrote:
> nit: chrome-extension URL
> (We don't have an actual extension object for it, which is what it sounds like
> in the comment.)

Done.

https://codereview.chromium.org/12374007/diff/35001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:268: if (url ==
GURL(kChromeSigninEffectiveURL))
On 2013/03/06 00:14:54, creis wrote:
> You should check the scheme and host.

Done.

[Charlie Reis, 2013-03-06 01:28:46]

LGTM with nit.  Darin?

https://codereview.chromium.org/12374007/diff/50001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

https://codereview.chromium.org/12374007/diff/50001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:102:
"chrome-extension://acfccoigjajmmgbhpfbjnpckhjjegnih";
Separate from this CL, please ask erikkay@ how we can prevent this ID from ever
being uploaded to the Chrome Web Store.

https://codereview.chromium.org/12374007/diff/50001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.h (right):

https://codereview.chromium.org/12374007/diff/50001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.h:36: static const char*
kChromeSigninEffectiveOrigin;
nit: kChromeSigninEffectiveSite

(Site is more accurate than origin here, since we're using it for the
SiteInstance's site URL.)

[tim (not reviewing), 2013-03-06 01:47:26]

Renamed variable to kChromeSigninEffectiveSite.

I chatted with mpcomplete@ earlier who told me they rely on the vastness of the
ID space rather than "reserving" special extension IDs, but I will follow up to
make extra sure there isn't anything else we should be doing.

Thanks.

[tim (not reviewing), 2013-03-06 18:41:51]

On 2013/03/06 01:47:26, timsteele wrote:
> Renamed variable to kChromeSigninEffectiveSite.
> 
> I chatted with mpcomplete@ earlier who told me they rely on the vastness of
the
> ID space rather than "reserving" special extension IDs, but I will follow up
to
> make extra sure there isn't anything else we should be doing.
> 
> Thanks.

ping Darin.

[aside, Charlie, I'm putting together an alternative CL to to address the
security issue by dusting off the old ClientLogin asp flow and will send it your
way for review. This would mean enabling code *on beta* that's on m25 stable but
has never run by default on m26 (dev or beta). If it does work, I may hold off
on this CL until further notice. Fingers tightly crossed.]

[darin (slow to review), 2013-03-06 22:08:53]

LGTM

[commit-bot: I haz the power, 2013-03-06 22:25:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/12374007/55008

[tim (not reviewing), 2013-03-07 00:25:41]

Committed patchset #15 manually as r186550 (presubmit successful).