Android: Disallow iframes from requesting to auto-log-in.

Android: Disallow iframes from requesting to auto-log-in.

Other embedded content, such as plug-ins, is also prevented from incurring
auto-log-in requests.

BUG=334230
TEST=You still get the auto-log-in infobar when the requesting site is the
top-level frame, but never otherwise.

Committed: https://crrev.com/865bd0940ada91272c4f92b19f996640b210745d
Cr-Commit-Position: refs/heads/master@{#299928}

[palmer, 2014-01-31 00:39:49]

Am I on the right track here? PTAL. Thank you!

[msw, 2014-02-03 23:37:00]

This seems reasonable to me, but I'm not sure it's entirely correct (might some
websites embed frames to login via 3rd party services? does this check belongs
in AutoLoginPrompter?). I would like to defer review to sky@ or another
appropriate reviewer/owner, since I'm not suffciently knowledgeable here; sorry
for that and the delay in my response.

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:144: // Only offer to
auto-log-in if the user can see the origin that is
nit: s/log-in/login/ here and below.

[sky, 2014-02-04 01:00:51]

+creis

[Charlie Reis, 2014-02-04 01:26:09]

I haven't thought much about the policy (don't let iframes request sign-in),
though it seems reasonable on the surface.  I don't understand the comment
below, though.

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
When is a WebContents not top-level?  iframes don't have their own WebContents.

[palmer, 2014-02-20 21:30:32]

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
On 2014/02/04 01:26:09, Charlie Reis wrote:
> When is a WebContents not top-level?  iframes don't have their own
WebContents.

I admit I'm no expert, but if a WebContents has an embedder, that would mean it
is not a top-level WebContents. Perhaps its embedder is top-level (i.e.
GetEmbedderWebContents return NULL).

But if iframes don't have their own WebContents, then I don't know what I'm
doing and would appreciate a clue. :)

[Fady Samuel, 2014-02-20 21:35:53]

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
On 2014/02/20 21:30:33, Chromium Palmer wrote:
> On 2014/02/04 01:26:09, Charlie Reis wrote:
> > When is a WebContents not top-level?  iframes don't have their own
> WebContents.
> 
> I admit I'm no expert, but if a WebContents has an embedder, that would mean
it
> is not a top-level WebContents. Perhaps its embedder is top-level (i.e.
> GetEmbedderWebContents return NULL).
> 
> But if iframes don't have their own WebContents, then I don't know what I'm
> doing and would appreciate a clue. :)


<webview>: The content of a webview thinks it's a top-level page, but it's
actually embedded by a Chrome App (and perhaps other contexts in the future).

[Charlie Reis, 2014-02-20 22:26:03]

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
On 2014/02/20 21:35:53, Fady Samuel wrote:
> On 2014/02/20 21:30:33, Chromium Palmer wrote:
> > On 2014/02/04 01:26:09, Charlie Reis wrote:
> > > When is a WebContents not top-level?  iframes don't have their own
> > WebContents.
> > 
> > I admit I'm no expert, but if a WebContents has an embedder, that would mean
> it
> > is not a top-level WebContents. Perhaps its embedder is top-level (i.e.
> > GetEmbedderWebContents return NULL).
> > 
> > But if iframes don't have their own WebContents, then I don't know what I'm
> > doing and would appreciate a clue. :)
> 
> 
> <webview>: The content of a webview thinks it's a top-level page, but it's
> actually embedded by a Chrome App (and perhaps other contexts in the future).

To be clear: iframes never have a WebContents of their own.  WebContents
generally corresponds to a tab and all of its iframes, except in Fady's case of
the <webview> tag.  The whole embedder WebContents notion will go away when
<webview> migrates from BrowserPlugin to using out-of-process iframes
(RenderFrameHosts).

I think you'll need more context than just the WebContents, such as some notion
of which frame the request came from.

[palmer, 2014-10-14 01:09:08]

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
> I think you'll need more context than just the WebContents, such as some
notion
> of which frame the request came from.

Any clues on where I can get that context? Sorry, I totally don't know this area
of the code.

[palmer, 2014-10-14 01:11:00]

> > I think you'll need more context than just the WebContents, such as some
> notion
> > of which frame the request came from.
> 
> Any clues on where I can get that context? Sorry, I totally don't know this
area
> of the code.

Hmm, what about testing that web_contents->GetMainFrame() ==
web_contents->GetFocusedFrame() ?

[Charlie Reis, 2014-10-14 19:56:41]

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
On 2014/10/14 01:09:08, Chromium Palmer wrote:
> > I think you'll need more context than just the WebContents, such as some
> notion
> > of which frame the request came from.
> 
> Any clues on where I can get that context? Sorry, I totally don't know this
area
> of the code.

On 2014/10/14 01:11:00, Chromium Palmer wrote:
> Hmm, what about testing that web_contents->GetMainFrame() ==
> web_contents->GetFocusedFrame() ?

I'm not familiar with auto login or the possible call stacks for this function,
but I doubt there's a guarantee that the login attempt is coming from the
currently focused frame.  It might be possible for the event to come from a
different frame than the focused frame in the first place, or the focus may have
changed by the time we get here.

It looks to me like there's not enough context here to tell which frame is
making the request.  Code search isn't cross-linking for me at the moment so
it's tough for me to investigate.  How does this code get called?

[palmer, 2014-10-15 20:54:20]

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_infobar_delegate.cc (right):

https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the attempt:
that is, if |web_contents| is top-level. iframes
> It looks to me like there's not enough context here to tell which frame is
> making the request.  Code search isn't cross-linking for me at the moment so
> it's tough for me to investigate.  How does this code get called?

Weirdly:

~/chromium/src $ search -n '\.cc$' -C AutoLoginInfoBarDelegate::Create -v
./chrome/browser/ui/auto_login_infobar_delegate.cc:132:bool
AutoLoginInfoBarDelegate::Create(content::WebContents* web_contents,
./chrome/browser/ui/auto_login_infobar_delegate.cc
./chrome/browser/ui/android/infobars/auto_login_prompter.cc:110:   
infobar_shown_ = AutoLoginInfoBarDelegate::Create(web_contents(), params_);
./chrome/browser/ui/android/infobars/auto_login_prompter.cc

That's it. I'll look around in the call site after this next meeting.

[Charlie Reis, 2014-10-15 22:00:56]

On 2014/10/15 20:54:20, Chromium Palmer wrote:
>
https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
> File chrome/browser/ui/auto_login_infobar_delegate.cc (right):
> 
>
https://codereview.chromium.org/131483010/diff/1/chrome/browser/ui/auto_login...
> chrome/browser/ui/auto_login_infobar_delegate.cc:145: // incurring the
attempt:
> that is, if |web_contents| is top-level. iframes
> > It looks to me like there's not enough context here to tell which frame is
> > making the request.  Code search isn't cross-linking for me at the moment so
> > it's tough for me to investigate.  How does this code get called?
> 
> Weirdly:
> 
> ~/chromium/src $ search -n '\.cc$' -C AutoLoginInfoBarDelegate::Create -v
> ./chrome/browser/ui/auto_login_infobar_delegate.cc:132:bool
> AutoLoginInfoBarDelegate::Create(content::WebContents* web_contents,
> ./chrome/browser/ui/auto_login_infobar_delegate.cc
> ./chrome/browser/ui/android/infobars/auto_login_prompter.cc:110:   
> infobar_shown_ = AutoLoginInfoBarDelegate::Create(web_contents(), params_);
> ./chrome/browser/ui/android/infobars/auto_login_prompter.cc
> 
> That's it. I'll look around in the call site after this next meeting.

I looked into this a bit, and it seems like you might want to gate this much
earlier.  We shouldn't even try to start the AutoLogin code if the request
triggering it wasn't for a main frame.

That looks like it's triggered from
ChromeResourceDispatcherHostDelegate::OnResponseStarted, where we can check
whether info->IsMainFrame() before calling
AutoLoginPrompter::ShowInfoBarIfPossible.

Hope that helps!

[palmer, 2014-10-15 23:41:32]

> I looked into this a bit, and it seems like you might want to gate this much
> earlier.  We shouldn't even try to start the AutoLogin code if the request
> triggering it wasn't for a main frame.
> 
> That looks like it's triggered from
> ChromeResourceDispatcherHostDelegate::OnResponseStarted, where we can check
> whether info->IsMainFrame() before calling
> AutoLoginPrompter::ShowInfoBarIfPossible.
> 
> Hope that helps!

Thank you Charlie! Here's a new patch set.

[Charlie Reis, 2014-10-16 00:05:27]

The change LGTM, though you'll need a chrome/ owner's review as well.

For this line in the CL description:
"Other embedded content, such as plug-ins, should also be prevented from
incurring auto-log-in requests."

should also be -> are also

(I wasn't originally clear if this was a TODO or not.  I think this change
handles that case, unless the plug-in is top-level.  Correct?)

[palmer, 2014-10-16 00:19:40]

> should also be -> are also
> 
> (I wasn't originally clear if this was a TODO or not.  I think this change
> handles that case, unless the plug-in is top-level.  Correct?)

Yes, I think so; thanks. ("is also" because content is singular.)

[palmer, 2014-10-16 00:20:03]

palmer@chromium.org changed reviewers:
+ thestig@chromium.org

[palmer, 2014-10-16 00:20:04]

thestig: LGTY for OWNERS?

[Lei Zhang, 2014-10-16 00:24:57]

Can you prefix the commit msg with "Android: " ?

It looks like by "plug-ins" you mean BrowserPlugins and not NPAPI/PPAPI plugins,
which AFAIK don't exist on Android except maybe in experimental branches, right?

[palmer, 2014-10-16 00:28:32]

> Can you prefix the commit msg with "Android: " ?

Done.

> It looks like by "plug-ins" you mean BrowserPlugins and not NPAPI/PPAPI
plugins,
> which AFAIK don't exist on Android except maybe in experimental branches,
right?

Correct.

[Lei Zhang, 2014-10-16 00:31:23]

All makes sense, lgtm.

[palmer, 2014-10-16 00:31:54]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-10-16 00:33:28]

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

[commit-bot: I haz the power, 2014-10-16 03:19:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 03:19:52]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[palmer, 2014-10-16 17:29:10]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:31:23]

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

[commit-bot: I haz the power, 2014-10-16 18:03:43]

Committed patchset #4 (id:190001)

[commit-bot: I haz the power, 2014-10-16 18:04:34]

Patchset 4 (id:??) landed as
https://crrev.com/865bd0940ada91272c4f92b19f996640b210745d
Cr-Commit-Position: refs/heads/master@{#299928}