Don't autofill credit cards on non-secure pages

Don't autofill credit cards on non-secure pages

Instead of checking for scheme only when suggesting credit card autofills,
ensure that the page is secure.

Also add missing tests for storing credit cards (even when submitted on
http).

Committed: https://crrev.com/337de86ef89076ef6a442acfc6e2904a0827d389
Cr-Commit-Position: refs/heads/master@{#337796}

[sigbjorn, 2015-05-15 08:38:27]

sigbjorn@opera.com changed reviewers:
+ estade@chromium.org, isherman@chromium.org

[sigbjorn, 2015-05-15 08:38:28]

Want to review a change to make autofill adhere to the same security standards
as other code?

https://codereview.chromium.org/1136473006/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/1136473006/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager_unittest.cc:715:
autofill_client_.SetContextSecurity(false);
This line is the reason for moving this function inside the class. The rest of
this function is unchanged (apart from breaking long lines).

[Evan Stade, 2015-05-15 16:54:45]

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

[Evan Stade, 2015-05-15 16:54:46]

+palmer (security team member)

https://codereview.chromium.org/1136473006/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (left):

https://codereview.chromium.org/1136473006/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:90: return
form.source_url().SchemeIs(url::kHttpsScheme);
we can't just make this content::IsOriginSecure() or something?

[Evan Stade, 2015-05-15 16:56:24]

https://codereview.chromium.org/1136473006/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (left):

https://codereview.chromium.org/1136473006/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:90: return
form.source_url().SchemeIs(url::kHttpsScheme);
On 2015/05/15 16:54:46, Evan Stade wrote:
> we can't just make this content::IsOriginSecure() or something?

I mean, I guess we can't reach into content:: because iOS uses this code.
(Speaking of which, this patch breaks iOS.) But palmer@ will know the correct
way to check URL security.

[palmer, 2015-05-15 17:56:41]

> > we can't just make this content::IsOriginSecure() or something?
> 
> I mean, I guess we can't reach into content:: because iOS uses this code.
> (Speaking of which, this patch breaks iOS.) But palmer@ will know the correct
> way to check URL security.

Yes, IsOriginSecure is the true right way to do it.

But, GURL::SchemeIsCryptographic (recently landed) is the new, better way to
check for secure URL schemes. In fact I am about to land a patch that fixes all
call sites that look like "url().SchemeIs(url::kHttpsScheme)".

So, how about this solution, which looks horrible but will work maximally well:

#ifdef _WHATEVER_THE_IOS_DEFINE_IS
  if (!url.SchemeIsCryptographic())
    fail();
#else
  if (!content::IsOriginSecure(url))
    fail();
#endif

[Evan Stade, 2015-05-15 18:51:45]

On 2015/05/15 17:56:41, palmer wrote:
> > > we can't just make this content::IsOriginSecure() or something?
> > 
> > I mean, I guess we can't reach into content:: because iOS uses this code.
> > (Speaking of which, this patch breaks iOS.) But palmer@ will know the
correct
> > way to check URL security.
> 
> Yes, IsOriginSecure is the true right way to do it.
> 
> But, GURL::SchemeIsCryptographic (recently landed) is the new, better way to
> check for secure URL schemes. In fact I am about to land a patch that fixes
all
> call sites that look like "url().SchemeIs(url::kHttpsScheme)".
> 
> So, how about this solution, which looks horrible but will work maximally
well:
> 
> #ifdef _WHATEVER_THE_IOS_DEFINE_IS
>   if (!url.SchemeIsCryptographic())
>     fail();
> #else
>   if (!content::IsOriginSecure(url))
>     fail();
> #endif

https://www.youtube.com/watch?v=WWaLxFIVX1s

I don't even think that would work because DEPS still bans dependencies on
content/ from core/.

We probably need to add a function to some generic interface which is
implemented separately on iOS and !iOS.

[palmer, 2015-05-15 19:59:50]

> I don't even think that would work because DEPS still bans dependencies on
> content/ from core/.

I've just about had it with our 'layering' 'model'. (It is neither layering, nor
a model. Discuss.)

I guess we'll have to use GURL::SchemeIsCryptographic, and field complaints from
developers who can't test on http://localhost.

[Ilya Sherman, 2015-05-15 20:18:25]

On 2015/05/15 19:59:50, palmer wrote:
> > I don't even think that would work because DEPS still bans dependencies on
> > content/ from core/.
> 
> I've just about had it with our 'layering' 'model'. (It is neither layering,
nor
> a model. Discuss.)
> 
> I guess we'll have to use GURL::SchemeIsCryptographic, and field complaints
from
> developers who can't test on http://localhost.

Evan, I don't understand your concern.  The AutofillClient interface is indeed
the right interface for implementing functionality that can't be shared with
iOS.  It sounds like the iOS implementation of that interface should test
url.SchemeIsCryptographic().  What am I missing?

[Evan Stade, 2015-05-15 20:50:48]

On 2015/05/15 20:18:25, Ilya Sherman wrote:
> On 2015/05/15 19:59:50, palmer wrote:
> > > I don't even think that would work because DEPS still bans dependencies on
> > > content/ from core/.
> > 
> > I've just about had it with our 'layering' 'model'. (It is neither layering,
> nor
> > a model. Discuss.)
> > 
> > I guess we'll have to use GURL::SchemeIsCryptographic, and field complaints
> from
> > developers who can't test on http://localhost.
> 
> Evan, I don't understand your concern.  The AutofillClient interface is indeed
> the right interface for implementing functionality that can't be shared with
> iOS.  It sounds like the iOS implementation of that interface should test
> url.SchemeIsCryptographic().  What am I missing?

a) the actual implementation
b) the fact that this has nothing to do with Autofill and is likely to come up
in other places in Chrome

[Evan Stade, 2015-05-18 18:15:50]

https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (left):

https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:90: return
form.source_url().SchemeIs(url::kHttpsScheme);
so palmer@, should we just change this to SchemeIsCryptographic for now and
worry about refactoring later?

[palmer, 2015-05-18 18:26:19]

https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (left):

https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:90: return
form.source_url().SchemeIs(url::kHttpsScheme);
> so palmer@, should we just change this to SchemeIsCryptographic for now and
> worry about refactoring later?

LGTM either way. (And, at the moment and into at least the medium term, even the
status quo is not wrong.)

[Ilya Sherman, 2015-05-18 18:41:31]

https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (left):

https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:90: return
form.source_url().SchemeIs(url::kHttpsScheme);
On 2015/05/18 18:15:50, Evan Stade wrote:
> so palmer@, should we just change this to SchemeIsCryptographic for now and
> worry about refactoring later?

Sorry, I'm still not following why we shouldn't just use the AutofillDriver or
AutofillClient interface to do the fully correct thing.  (I'm realizing now that
sigbjorn@ chose the client interface, whereas the driver interface would
probably be a better choice.)

[Evan Stade, 2015-05-18 18:54:46]

On 2015/05/18 18:41:31, Ilya Sherman wrote:
>
https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
> File components/autofill/core/browser/autofill_manager.cc (left):
> 
>
https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
> components/autofill/core/browser/autofill_manager.cc:90: return
> form.source_url().SchemeIs(url::kHttpsScheme);
> On 2015/05/18 18:15:50, Evan Stade wrote:
> > so palmer@, should we just change this to SchemeIsCryptographic for now and
> > worry about refactoring later?
> 
> Sorry, I'm still not following why we shouldn't just use the AutofillDriver or
> AutofillClient interface to do the fully correct thing.  (I'm realizing now
that
> sigbjorn@ chose the client interface, whereas the driver interface would
> probably be a better choice.)

again, because this check has nothing to do with Autofill per se, and should not
be part of that interface. Having a way to do the (most) correct thing from
anywhere in components/ would be useful, but adding cruft to some Autofill
interface does not seem worth it. What is the practical drawback of just using
SchemeIsCryptographic?

[Ilya Sherman, 2015-05-18 21:19:22]

On 2015/05/18 18:54:46, Evan Stade wrote:
> On 2015/05/18 18:41:31, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
> > File components/autofill/core/browser/autofill_manager.cc (left):
> > 
> >
>
https://codereview.chromium.org/1136473006/diff/20001/components/autofill/cor...
> > components/autofill/core/browser/autofill_manager.cc:90: return
> > form.source_url().SchemeIs(url::kHttpsScheme);
> > On 2015/05/18 18:15:50, Evan Stade wrote:
> > > so palmer@, should we just change this to SchemeIsCryptographic for now
and
> > > worry about refactoring later?
> > 
> > Sorry, I'm still not following why we shouldn't just use the AutofillDriver
or
> > AutofillClient interface to do the fully correct thing.  (I'm realizing now
> that
> > sigbjorn@ chose the client interface, whereas the driver interface would
> > probably be a better choice.)
> 
> again, because this check has nothing to do with Autofill per se, and should
not
> be part of that interface. Having a way to do the (most) correct thing from
> anywhere in components/ would be useful, but adding cruft to some Autofill
> interface does not seem worth it. What is the practical drawback of just using
> SchemeIsCryptographic?

The practical drawback is that local website development is slightly harder.  I
guess I don't see it as all that much cruft, and am not holding my breath for a
shim layer that provides a content-like interface that iOS components can use,
even for a fraction of the content API.

[sigbjorn, 2015-05-19 08:00:43]

Note that this fix has nothing to do with localhost. This fix ensures that
credit card details are not offered for autosubmission on insecure pages. E.g.
pages with insecure inlines, invalid certificates, forms with http targets, etc.
Any localhost changes are entirely accidental. SchemeIsCryptographic is
necessary, but not sufficient to measure the security of a page.

I can move the interface to the driver instead of the client if that is
preferred. The client seemed the right choice, as it was directly using the
webcontents already.

[Evan Stade, 2015-05-19 16:44:43]

estade@chromium.org changed required reviewers:
+ palmer@chromium.org

[Evan Stade, 2015-05-19 16:47:18]

On 2015/05/19 08:00:43, sigbjorn wrote:
> Note that this fix has nothing to do with localhost. This fix ensures that
> credit card details are not offered for autosubmission on insecure pages. E.g.
> pages with insecure inlines, invalid certificates, forms with http targets,
etc.
> Any localhost changes are entirely accidental. SchemeIsCryptographic is
> necessary, but not sufficient to measure the security of a page.
> 
> I can move the interface to the driver instead of the client if that is
> preferred. The client seemed the right choice, as it was directly using the
> webcontents already.

palmer@, can you address this? What you said earlier ("even the status quo is
not wrong") seems to contradict sigbjorn@'s assertions. When do we need to check
cert status and when is a scheme check sufficient?

[palmer, 2015-05-19 18:36:58]

> > Any localhost changes are entirely accidental. SchemeIsCryptographic is
> > necessary, but not sufficient to measure the security of a page.
> > 
> > I can move the interface to the driver instead of the client if that is
> > preferred. The client seemed the right choice, as it was directly using the
> > webcontents already.
> 
> palmer@, can you address this? What you said earlier ("even the status quo is
> not wrong") seems to contradict sigbjorn@'s assertions. When do we need to
check
> cert status and when is a scheme check sufficient?

When I said the status quo is not wrong, I meant specifically the utility
function

 89 bool FormIsHTTPS(const FormStructure& form) {	
 90   return form.source_url().SchemeIs(url::kHttpsScheme);	
 91 }

as opposed to less ad hoc SchemeIsCryptographic. I should have been more
precise; sorry about that.

I see checking certificate status as a very nice enhancement (and we do the same
thing for password auto-fill). The main thing is to not break testing; I think
we can use net::IsLocalhost and IsLocalhostTLD to determine if we don't need to
bother with certificate checking. That won't cover all testing cases (e.g.
self-signed HTTPS on testing-server.example.com — testing-server.localhost would
work better for a variety of reasons), but it will cover a range (I hope large)
of testing deployments while protecting public deployments.

We should probably harmonize the password autofill behavior with the credit card
autofill behavior. vabr and gcasto know about password autofill.

[Evan Stade, 2015-05-19 21:08:45]

On 2015/05/19 18:36:58, palmer wrote:
> > > Any localhost changes are entirely accidental. SchemeIsCryptographic is
> > > necessary, but not sufficient to measure the security of a page.
> > > 
> > > I can move the interface to the driver instead of the client if that is
> > > preferred. The client seemed the right choice, as it was directly using
the
> > > webcontents already.
> > 
> > palmer@, can you address this? What you said earlier ("even the status quo
is
> > not wrong") seems to contradict sigbjorn@'s assertions. When do we need to
> check
> > cert status and when is a scheme check sufficient?
> 
> When I said the status quo is not wrong, I meant specifically the utility
> function
> 
>  89 bool FormIsHTTPS(const FormStructure& form) {	
>  90   return form.source_url().SchemeIs(url::kHttpsScheme);	
>  91 }
> 
> as opposed to less ad hoc SchemeIsCryptographic. I should have been more
> precise; sorry about that.
> 
> I see checking certificate status as a very nice enhancement (and we do the
same
> thing for password auto-fill). The main thing is to not break testing; I think
> we can use net::IsLocalhost and IsLocalhostTLD to determine if we don't need
to
> bother with certificate checking. That won't cover all testing cases (e.g.
> self-signed HTTPS on http://testing-server.example.com
— testing-server.localhost would
> work better for a variety of reasons), but it will cover a range (I hope
large)
> of testing deployments while protecting public deployments.
> 
> We should probably harmonize the password autofill behavior with the credit
card
> autofill behavior. vabr and gcasto know about password autofill.

that appears to be here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

A new challenger has appeared!

[palmer, 2015-05-19 21:29:11]

> > We should probably harmonize the password autofill behavior with the credit
> card
> > autofill behavior. vabr and gcasto know about password autofill.
> 
> that appears to be here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

That seems to boil down to (from
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h):

149     bool canAccessPasswordManager() const { return !isUnique(); }

167     // The origin is a globally unique identifier assigned when the Document
is
168     // created.
http://www.whatwg.org/specs/web-apps/current-work/#sandboxOrigin
169     //
170     // There's a subtle difference between a unique origin and an origin
that
171     // has the SandboxOrigin flag set. The latter implies the former, and,
in
172     // addition, the SandboxOrigin flag is inherited by iframes.
173     bool isUnique() const { return m_isUnique; }

I'm not sure I'm seeing the whole story, here, though.

[sigbjorn, 2015-05-20 06:59:17]

The password manager ensures that the stored origin and the used origin are
identical.
The credit card manager ensures that the used origin and web page are secure. 
I don't think these two can be coordinated, they do two different things.

For testing on localhost: I'd be happy to replace the check for
SECURITY_STYLE_AUTHENTICATED with (IsLocalhost || SECURITY_STYLE_AUTHENTICATED).
However, I don't know how to check that an http localhost page contains no
insecure inlines from other domains. The definition of NORMAL_CONTENT says:
"HTTP page, or HTTPS page with no insecure content."
If the testing use case is for web site developers to test before production,
allowing insecure inlines on localhost could trick them, as it would no longer
work once in production. For users, using their credit card on a localhost
service with insecure inlines wouldn't really be secure - although this is
likely not a big issue in the wild. Either way, both indicate that insecure
inlines should not be allowed on localhost, even over http. It is perfectly
possible to set up a localhost test server with https. Note that testing on http
localhost was not previously possible, so I don't see a need to enable it now. I
vote for keeping the checks for secure origin and page.
If the testing use case is for developing Chromium, there are testing tools
which allow setups with certificates, so I don't think that should be an issue.

[sigbjorn, 2015-05-22 07:32:04]

The CQ bit was checked by sigbjorn@opera.com

[sigbjorn, 2015-05-22 07:32:05]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps40001
(title: "Remove debug code")

[commit-bot: I haz the power, 2015-05-22 07:32:35]

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

[commit-bot: I haz the power, 2015-05-22 07:39:16]

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

[commit-bot: I haz the power, 2015-05-22 07:39:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sigbjorn, 2015-05-22 07:45:46]

Nobody has spoken up with anything else, so attempted a commit, given the OK
above. Got the following error message, so someone else will have to OK this as
well.

Missing LGTM from an OWNER for these files:
    chrome/browser/ui/autofill/chrome_autofill_client.cc
    chrome/browser/ui/autofill/chrome_autofill_client.h
    components/autofill/core/browser/autofill_client.h
    components/autofill/core/browser/autofill_manager.cc
    components/autofill/core/browser/autofill_manager_unittest.cc
    components/autofill/core/browser/test_autofill_client.cc
    components/autofill/core/browser/test_autofill_client.h

[Evan Stade, 2015-05-22 16:22:47]

On 2015/05/22 07:45:46, sigbjorn wrote:
> Nobody has spoken up with anything else, so attempted a commit, given the OK
> above. Got the following error message, so someone else will have to OK this
as
> well.
> 
> Missing LGTM from an OWNER for these files:
>     chrome/browser/ui/autofill/chrome_autofill_client.cc
>     chrome/browser/ui/autofill/chrome_autofill_client.h
>     components/autofill/core/browser/autofill_client.h
>     components/autofill/core/browser/autofill_manager.cc
>     components/autofill/core/browser/autofill_manager_unittest.cc
>     components/autofill/core/browser/test_autofill_client.cc
>     components/autofill/core/browser/test_autofill_client.h

yea, not lgtm until the security team has decided on the best way to resolve
this. palmer@ started an email thread but it doesn't appear to have gotten much
attention unfortunately.

[sigbjorn, 2015-05-28 08:38:20]

Ping

[Evan Stade, 2015-05-28 18:21:56]

On 2015/05/28 08:38:20, sigbjorn wrote:
> Ping

sorry this is taking so long. You should probably ping the email thread though,
not this CL.

[sigbjorn, 2015-06-02 08:06:04]

On 2015/05/28 18:21:56, Evan Stade wrote:
> On 2015/05/28 08:38:20, sigbjorn wrote:
> > Ping
> 
> sorry this is taking so long. You should probably ping the email thread
though,
> not this CL.

Pinging here again, after having pinged the email thread.

[sigbjorn, 2015-06-05 07:15:42]

Ping - is silence consent?

Nobody has spoken up against this, and I understand the concern as being that
this might potentially not be the long term direction. This is a gain, at the
very least in the short term, and nobody seems to have any long term plans
anyhow.

[Evan Stade, 2015-06-05 18:58:10]

On 2015/06/05 07:15:42, sigbjorn wrote:
> Ping - is silence consent?
> 
> Nobody has spoken up against this, and I understand the concern as being that
> this might potentially not be the long term direction. This is a gain, at the
> very least in the short term, and nobody seems to have any long term plans
> anyhow.

no, silence is not consent. I apologize that this is taking so long, but this is
the wrong place to ping. Further pings should be directed at the email thread
because the people who need to give an opinion are not cc'd on this CL.

[jww, 2015-06-08 18:37:34]

jww@chromium.org changed reviewers:
+ jww@chromium.org

[jww, 2015-06-08 18:37:35]

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
It seems bad to introduce this. We have isPrivilegedContext() already
implemented, which is a spec compliant way of determining if special powers are
allowed on a given context:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

At the very least, this code should be using isPrivilegedContext() in addition
to the mixed content checks, as it does a full set of checks up the ancestor
chain.

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:312:
web_contents()->GetController().GetVisibleEntry();
This is probably not what we want. We probably want GetLastCommitted, since
that's the content actually on the page.

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:319:
content::SECURITY_STYLE_AUTHENTICATED &&
We probably shouldn't be using the security style to make security decisions.
This is really a UX indicator. For example, this doesn't take into account URLs
that have been manually whitelisted as "secure" for testing purposes.

If the primitives we need are not already in place to determine security status
that we need, we should write new ones.

https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_client.h:182: return
form_origin.SchemeIsCryptographic();
As per the comment in gurl.h, we should almost never use
SchemeIsCryptographic(). Instead, check out IsOriginSecure() in origin_util.h.
This does a more complete check (although, as per my earlier comment, this
should probably be an IsPrivilegedContext() check instead).

[jww, 2015-06-08 18:37:59]

jww@chromium.org changed reviewers:
- palmer@chromium.org

[jww, 2015-06-08 18:38:00]

jww@chromium.org changed required reviewers:
- palmer@chromium.org

[jww, 2015-06-08 22:09:56]

On 2015/06/08 18:37:35, jww wrote:
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
> ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
> It seems bad to introduce this. We have isPrivilegedContext() already
> implemented, which is a spec compliant way of determining if special powers
are
> allowed on a given context:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> At the very least, this code should be using isPrivilegedContext() in addition
> to the mixed content checks, as it does a full set of checks up the ancestor
> chain.
> 
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:312:
> web_contents()->GetController().GetVisibleEntry();
> This is probably not what we want. We probably want GetLastCommitted, since
> that's the content actually on the page.
> 
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:319:
> content::SECURITY_STYLE_AUTHENTICATED &&
> We probably shouldn't be using the security style to make security decisions.
> This is really a UX indicator. For example, this doesn't take into account
URLs
> that have been manually whitelisted as "secure" for testing purposes.
> 
> If the primitives we need are not already in place to determine security
status
> that we need, we should write new ones.
> 
>
https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
> File components/autofill/core/browser/autofill_client.h (right):
> 
>
https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
> components/autofill/core/browser/autofill_client.h:182: return
> form_origin.SchemeIsCryptographic();
> As per the comment in gurl.h, we should almost never use
> SchemeIsCryptographic(). Instead, check out IsOriginSecure() in origin_util.h.
> This does a more complete check (although, as per my earlier comment, this
> should probably be an IsPrivilegedContext() check instead).

I missed a lot of the earlier discussion (esp with palmer), which seems to
supersede most of my comments, so ignore where applicable :-) I'm rereading the
thread here again, and I'll try to respond to the outstanding questions.

[jww, 2015-06-08 22:33:03]

On 2015/06/08 22:09:56, jww wrote:
> On 2015/06/08 18:37:35, jww wrote:
> >
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
> > ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
> > It seems bad to introduce this. We have isPrivilegedContext() already
> > implemented, which is a spec compliant way of determining if special powers
> are
> > allowed on a given context:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > At the very least, this code should be using isPrivilegedContext() in
addition
> > to the mixed content checks, as it does a full set of checks up the ancestor
> > chain.
> > 
> >
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:312:
> > web_contents()->GetController().GetVisibleEntry();
> > This is probably not what we want. We probably want GetLastCommitted, since
> > that's the content actually on the page.
> > 
> >
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:319:
> > content::SECURITY_STYLE_AUTHENTICATED &&
> > We probably shouldn't be using the security style to make security
decisions.
> > This is really a UX indicator. For example, this doesn't take into account
> URLs
> > that have been manually whitelisted as "secure" for testing purposes.
> > 
> > If the primitives we need are not already in place to determine security
> status
> > that we need, we should write new ones.
> > 
> >
>
https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
> > File components/autofill/core/browser/autofill_client.h (right):
> > 
> >
>
https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
> > components/autofill/core/browser/autofill_client.h:182: return
> > form_origin.SchemeIsCryptographic();
> > As per the comment in gurl.h, we should almost never use
> > SchemeIsCryptographic(). Instead, check out IsOriginSecure() in
origin_util.h.
> > This does a more complete check (although, as per my earlier comment, this
> > should probably be an IsPrivilegedContext() check instead).
> 
> I missed a lot of the earlier discussion (esp with palmer), which seems to
> supersede most of my comments, so ignore where applicable :-) I'm rereading
the
> thread here again, and I'll try to respond to the outstanding questions.

From a security team perspective, this lgtm, although as palmer@ mentioned
earlier, I'd rather see this move towards using IsOriginSecure in the future,
and it would be great to have a better and consistent way of checking for mixed
content and the like. In particular, I really don't like the security_style
check.

Finally, the good think about IsOriginSecure is, as I mention in my comments,
that it checks the whitelist to see if any origins were explicitly added to the
command line as a testing flag to be treated as secure. I see no reason that
shouldn't be respected in the CC autofill.

[Evan Stade, 2015-06-08 22:41:42]

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
this param is not used

[Evan Stade, 2015-06-08 22:51:32]

Is there anything blocking us from using IsOriginSecure right now (in
ChromeAutofillClient)? ChromeAutofillClient is not used on iOS as far as I can
tell.

Also you'll have to update ios/chrome/browser/ui/autofill/autofill_client_ios.h

[sigbjorn, 2015-06-12 11:47:15]

On 2015/06/08 22:51:32, Evan Stade wrote:
> Is there anything blocking us from using IsOriginSecure right now (in
> ChromeAutofillClient)? ChromeAutofillClient is not used on iOS as far as I can
> tell.

ChromeAutofillClient has no use for IsOriginSecure. (Though it could be used to
exit early. If the form origin isn't even potentially secure, there is no way
the page will be secure.)

> Also you'll have to update
ios/chrome/browser/ui/autofill/autofill_client_ios.h

The fallback in autofill_client.h will work as a default for iOS and Android, no
changes are needed there. The code works the same way as before, but using
IsSchemeCryptographic instead of SchemeIs(url::kHttpsScheme).

[sigbjorn, 2015-06-12 11:47:54]

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
On 2015/06/08 22:41:42, Evan Stade wrote:
> this param is not used

It is used in the fallback mechanism. It is not needed for a proper
implementation. Once all OSes have overridden the default implementation, it may
be removed.

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
On 2015/06/08 18:37:35, jww wrote:
> It seems bad to introduce this. We have isPrivilegedContext() already
> implemented, which is a spec compliant way of determining if special powers
are
> allowed on a given context:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> At the very least, this code should be using isPrivilegedContext() in addition
> to the mixed content checks, as it does a full set of checks up the ancestor
> chain.

isPrivilegedContext seems to only check isPotentiallyTrustworthy on the URLs.
The purpose of this CL is to additionally check the connection security. The
check for ssl_status.content_status being normal could be replaced by a check
for isPrivilegedContext. isPrivilegedContext would have to be an asynchronous
call though, complicating the code unnecessarily for no additional gain. The
ssl_status has to be checked in any case to check the connection security.

https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:319:
content::SECURITY_STYLE_AUTHENTICATED &&
On 2015/06/08 18:37:34, jww wrote:
> We probably shouldn't be using the security style to make security decisions.
> This is really a UX indicator. For example, this doesn't take into account
URLs
> that have been manually whitelisted as "secure" for testing purposes.

Using the security style is on purpose. The user has one indicator for the
security on the page, and that is the padlock. If he sees the padlock, he should
be able to type his credit card, if he doesn't, he shouldn't, and we should warn
him if he tries to autocomplete one. The autocompletion warning is UI, and
should be tied to other UI indicators, anything else would be confusing.

The definition of SECURITY_STYLE_AUTHENTICATED is "indicates that we
successfully retrieved this object over an authenticated protocol, such as
HTTPS", which is exactly what we need.

I can add an override for manually whitelisted domains, how would I be able to
check against such a list?

> If the primitives we need are not already in place to determine security
status
> that we need, we should write new ones.

SSLStatus could really do with having two different states for "HTTP page" and
"HTTPS page with no insecure content", and not reuse the same for both. That
would mean there would be no need for the security style check. I expect
changing that enum would require extensive changes though.

https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_client.h:182: return
form_origin.SchemeIsCryptographic();
On 2015/06/08 18:37:35, jww wrote:
> As per the comment in gurl.h, we should almost never use
> SchemeIsCryptographic(). Instead, check out IsOriginSecure() in origin_util.h.
> This does a more complete check (although, as per my earlier comment, this
> should probably be an IsPrivilegedContext() check instead).

This is the fallback mechanism. Autofill_manager will call this, without knowing
which inherited class is actually used. The intention would be that this be
overridden in platform classes similar to what I have done in
chrome_autofill_client, but as I am not an Android nor iOS developer, this is
hard to do for me for other platforms. Meanwhile, it contains the same logic as
before (replacing SchemeIs(url::kHttpsScheme) with SchemeIsCryptographic).

IsOriginSecure is not available to iOS, and cannot be used at this level. See
https://sites.google.com/a/chromium.org/dev/developers/design-documents/layer...,
section "I need to use the content layer in foo.cc, but I'm getting a DEPS
violation when I try to upload!".

[jww, 2015-06-12 15:45:20]

On 2015/06/12 11:47:54, sigbjorn wrote:
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
> ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
> On 2015/06/08 22:41:42, Evan Stade wrote:
> > this param is not used
> 
> It is used in the fallback mechanism. It is not needed for a proper
> implementation. Once all OSes have overridden the default implementation, it
may
> be removed.
> 
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:309: bool
> ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {
> On 2015/06/08 18:37:35, jww wrote:
> > It seems bad to introduce this. We have isPrivilegedContext() already
> > implemented, which is a spec compliant way of determining if special powers
> are
> > allowed on a given context:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > At the very least, this code should be using isPrivilegedContext() in
addition
> > to the mixed content checks, as it does a full set of checks up the ancestor
> > chain.
> 
> isPrivilegedContext seems to only check isPotentiallyTrustworthy on the URLs.
> The purpose of this CL is to additionally check the connection security. The
> check for ssl_status.content_status being normal could be replaced by a check
> for isPrivilegedContext. isPrivilegedContext would have to be an asynchronous
> call though, complicating the code unnecessarily for no additional gain. The
> ssl_status has to be checked in any case to check the connection security.
> 
>
https://codereview.chromium.org/1136473006/diff/40001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:319:
> content::SECURITY_STYLE_AUTHENTICATED &&
> On 2015/06/08 18:37:34, jww wrote:
> > We probably shouldn't be using the security style to make security
decisions.
> > This is really a UX indicator. For example, this doesn't take into account
> URLs
> > that have been manually whitelisted as "secure" for testing purposes.
> 
> Using the security style is on purpose. The user has one indicator for the
> security on the page, and that is the padlock. If he sees the padlock, he
should
> be able to type his credit card, if he doesn't, he shouldn't, and we should
warn
> him if he tries to autocomplete one. The autocompletion warning is UI, and
> should be tied to other UI indicators, anything else would be confusing.
> 
> The definition of SECURITY_STYLE_AUTHENTICATED is "indicates that we
> successfully retrieved this object over an authenticated protocol, such as
> HTTPS", which is exactly what we need.
> 
> I can add an override for manually whitelisted domains, how would I be able to
> check against such a list?
Well, that's one of the things that IsSecureOrigin() gives you :-) Silly iOS not
letting us use //content...
> 
> > If the primitives we need are not already in place to determine security
> status
> > that we need, we should write new ones.
> 
> SSLStatus could really do with having two different states for "HTTP page" and
> "HTTPS page with no insecure content", and not reuse the same for both. That
> would mean there would be no need for the security style check. I expect
> changing that enum would require extensive changes though.
> 
>
https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
> File components/autofill/core/browser/autofill_client.h (right):
> 
>
https://codereview.chromium.org/1136473006/diff/40001/components/autofill/cor...
> components/autofill/core/browser/autofill_client.h:182: return
> form_origin.SchemeIsCryptographic();
> On 2015/06/08 18:37:35, jww wrote:
> > As per the comment in gurl.h, we should almost never use
> > SchemeIsCryptographic(). Instead, check out IsOriginSecure() in
origin_util.h.
> > This does a more complete check (although, as per my earlier comment, this
> > should probably be an IsPrivilegedContext() check instead).
> 
> This is the fallback mechanism. Autofill_manager will call this, without
knowing
> which inherited class is actually used. The intention would be that this be
> overridden in platform classes similar to what I have done in
> chrome_autofill_client, but as I am not an Android nor iOS developer, this is
> hard to do for me for other platforms. Meanwhile, it contains the same logic
as
> before (replacing SchemeIs(url::kHttpsScheme) with SchemeIsCryptographic).
> 
> IsOriginSecure is not available to iOS, and cannot be used at this level. See
>
https://sites.google.com/a/chromium.org/dev/developers/design-documents/layer...,
> section "I need to use the content layer in foo.cc, but I'm getting a DEPS
> violation when I try to upload!".

[sigbjorn, 2015-06-17 09:39:05]

The CQ bit was checked by sigbjorn@opera.com

[sigbjorn, 2015-06-17 09:39:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps60001
(title: "Use GetLastCommittedEntry")

[commit-bot: I haz the power, 2015-06-17 09:39:31]

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

[commit-bot: I haz the power, 2015-06-17 09:39:39]

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

[commit-bot: I haz the power, 2015-06-17 09:39:40]

A disapproval has been posted by following reviewers: estade@chromium.org.
Please make sure to get positive review before another CQ attempt.

[sigbjorn, 2015-06-19 13:56:26]

On 2015/06/17 09:39:40, commit-bot: I haz the power wrote:
> A disapproval has been posted by following reviewers:
mailto:estade@chromium.org.

estade is ooo till 25th, anyone else want to/able to do anything in the
meanwhile?

[Ilya Sherman, 2015-06-19 19:51:11]

On 2015/06/19 13:56:26, sigbjorn wrote:
> On 2015/06/17 09:39:40, commit-bot: I haz the power wrote:
> > A disapproval has been posted by following reviewers:
> mailto:estade@chromium.org.
> 
> estade is ooo till 25th, anyone else want to/able to do anything in the
> meanwhile?

Since Evan seems to still have concerns about this CL, you'll need to wait for
him to return.  I do completely understand your frustration about this CL review
progressing quite slowly.

[Evan Stade, 2015-06-25 23:54:58]

thanks for sticking with this for so long

https://codereview.chromium.org/1136473006/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_client.h:182: return
form_origin.SchemeIsCryptographic();
ah, I didn't notice this code because normally we don't put code in header
files. This would go in a .cc file, except this is an interface and there should
be no default implementations.

[sigbjorn, 2015-06-26 07:30:44]

https://codereview.chromium.org/1136473006/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_client.h:182: return
form_origin.SchemeIsCryptographic();
On 2015/06/25 23:54:57, Evan Stade wrote:
> ah, I didn't notice this code because normally we don't put code in header
> files. This would go in a .cc file, except this is an interface and there
should
> be no default implementations.

What do you prefer as a solution then? To avoid a default implementation, all
inherited classes must be fixed. I believe that leaves Mac and Android, I don't
know if there are any more. I do not have the setup to fix code for those OSes.
(I can try fixing them blindly, and use reviews and attempted commits to notice
any obvious issues.)

Another approach is to file separate bugs for those platforms, and remove the
default implementation once those have been fixed.

[Evan Stade, 2015-06-26 14:17:30]

On 2015/06/26 07:30:44, sigbjorn wrote:
>
https://codereview.chromium.org/1136473006/diff/60001/components/autofill/cor...
> File components/autofill/core/browser/autofill_client.h (right):
> 
>
https://codereview.chromium.org/1136473006/diff/60001/components/autofill/cor...
> components/autofill/core/browser/autofill_client.h:182: return
> form_origin.SchemeIsCryptographic();
> On 2015/06/25 23:54:57, Evan Stade wrote:
> > ah, I didn't notice this code because normally we don't put code in header
> > files. This would go in a .cc file, except this is an interface and there
> should
> > be no default implementations.
> 
> What do you prefer as a solution then? To avoid a default implementation, all
> inherited classes must be fixed. I believe that leaves Mac and Android, I
don't
> know if there are any more. I do not have the setup to fix code for those
OSes.
> (I can try fixing them blindly, and use reviews and attempted commits to
notice
> any obvious issues.)
> 
> Another approach is to file separate bugs for those platforms, and remove the
> default implementation once those have been fixed.

Typically I make a best effort and then use the trybots to verify. It should be
fairly easy to get right without too many trybot round trips since it's a one
liner.

[sigbjorn, 2015-06-29 10:14:38]

sigbjorn@opera.com changed reviewers:
+ boliu@chromium.org, dconnelly@chromium.org

[sigbjorn, 2015-06-29 10:15:25]

Sorry about rebase getting mixed up with my latest changes. My changes are in
the function IsContextSecure exclusively (across 5 files).

[sigbjorn, 2015-06-29 10:15:36]

The CQ bit was checked by sigbjorn@opera.com

[sigbjorn, 2015-06-29 10:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps80001
(title: "Remove default fallback for IsContextSecure, implement in inherited
classes")

[commit-bot: I haz the power, 2015-06-29 10:15:48]

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

[commit-bot: I haz the power, 2015-06-29 10:15:55]

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

[commit-bot: I haz the power, 2015-06-29 10:15:57]

A disapproval has been posted by following reviewers: estade@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Evan Stade, 2015-06-29 17:36:00]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-29 17:36:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136473006/80001

[Evan Stade, 2015-06-29 17:38:54]

LG after nits are fixed. I've sent the CL to the try bots.

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
components/autofill/core/browser/test_autofill_client.h:59: bool
IsContextSecure(const GURL& form_origin) override;
nit: is_context_secure() const (and inline the impl)

(simple setters and getters are exceptions to the no-inline-in-header-file rule)

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
components/autofill/core/browser/test_autofill_client.h:61: void
SetContextSecurity(bool is_context_secure);
nit: set_is_context_secure() (and inline the impl)

[commit-bot: I haz the power, 2015-06-29 17:54:28]

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

[commit-bot: I haz the power, 2015-06-29 17:54:30]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[sigbjorn, 2015-06-30 07:12:13]

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
components/autofill/core/browser/test_autofill_client.h:59: bool
IsContextSecure(const GURL& form_origin) override;
On 2015/06/29 17:38:54, Evan Stade wrote:
> nit: is_context_secure() const (and inline the impl)
> 
> (simple setters and getters are exceptions to the no-inline-in-header-file
rule)

This is an override, so that means I'd have to change the name of this function
elsewhere too. Elsewhere it is not a getter. Did you mean to change the name
throughout?

[Evan Stade, 2015-06-30 15:47:13]

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/1136473006/diff/80001/components/autofill/cor...
components/autofill/core/browser/test_autofill_client.h:59: bool
IsContextSecure(const GURL& form_origin) override;
On 2015/06/30 07:12:13, sigbjorn wrote:
> On 2015/06/29 17:38:54, Evan Stade wrote:
> > nit: is_context_secure() const (and inline the impl)
> > 
> > (simple setters and getters are exceptions to the no-inline-in-header-file
> rule)
> 
> This is an override, so that means I'd have to change the name of this
function
> elsewhere too. Elsewhere it is not a getter. Did you mean to change the name
> throughout?

ah, yea, nevermind about that one

[Evan Stade, 2015-06-30 15:48:12]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-06-30 15:48:13]

lgtm

[Evan Stade, 2015-06-30 15:48:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps100001
(title: "Nitpick and test run fixes")

[commit-bot: I haz the power, 2015-06-30 15:49:14]

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

[commit-bot: I haz the power, 2015-06-30 15:57:34]

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

[commit-bot: I haz the power, 2015-06-30 15:57:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[boliu, 2015-06-30 16:20:06]

https://codereview.chromium.org/1136473006/diff/100001/android_webview/native...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/100001/android_webview/native...
android_webview/native/aw_autofill_client.cc:184: // TODO (sigbjorn): Return if
the context is secure, not just
Android webview can use the same implementation as chrome I think?

If ios can do the same thing, then a better api would be to have the client
return SSLStatus. Then most of the implementation can be shared in the autofill
component.

[sigbjorn, 2015-07-01 07:28:12]

sigbjorn@opera.com changed reviewers:
+ bondd@chromium.org

[sigbjorn, 2015-07-01 07:28:14]

https://codereview.chromium.org/1136473006/diff/100001/android_webview/native...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/100001/android_webview/native...
android_webview/native/aw_autofill_client.cc:184: // TODO (sigbjorn): Return if
the context is secure, not just
On 2015/06/30 16:20:06, boliuOOOuntilJuly6 wrote:
> Android webview can use the same implementation as chrome I think?
> 
> If ios can do the same thing, then a better api would be to have the client
> return SSLStatus. Then most of the implementation can be shared in the
autofill
> component.

Good point. Perhaps bondd can answer this question? Can iOS in
AutofillClientIOS::IsContextSecure access the SSLStatus of the form's page? (If
so, could you assist with code to do so?)

[bondd, 2015-07-01 22:07:42]

On 2015/07/01 07:28:14, sigbjorn wrote:
>
https://codereview.chromium.org/1136473006/diff/100001/android_webview/native...
> File android_webview/native/aw_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/1136473006/diff/100001/android_webview/native...
> android_webview/native/aw_autofill_client.cc:184: // TODO (sigbjorn): Return
if
> the context is secure, not just
> On 2015/06/30 16:20:06, boliuOOOuntilJuly6 wrote:
> > Android webview can use the same implementation as chrome I think?
> > 
> > If ios can do the same thing, then a better api would be to have the client
> > return SSLStatus. Then most of the implementation can be shared in the
> autofill
> > component.
> 
> Good point. Perhaps bondd can answer this question? Can iOS in
> AutofillClientIOS::IsContextSecure access the SSLStatus of the form's page?
(If
> so, could you assist with code to do so?)

Hi sigbjorn, unfortunately I am not very familiar with this code.
I took a look anyway and found this line
  web::SSLStatus& sslStatus = [self currentEntry].navigationItem->GetSSL();
 
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/navigation...
CRWSessionController also has a -lastCommittedEntry method you could use.
AutofillClientIOS has access to ios::ChromeBrowserState through browser_state_.
My guess is that you could find CRWSessionController somehow from
ChromeBrowserState, and then do
  [sessionController lastCommittedEntry].navigationItem->GetSSL();
I'm not sure how to get from ChromeBrowserState to CRWSessionController.
Hopefully you can figure that part out. At this point I'm just spelunking
through codesearch, same as you would be. Also, this is just a guess. If anyone
has more concrete info please let us know.

[bondd, 2015-07-01 22:16:51]

https://codereview.chromium.org/1136473006/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:316: if (!navigation_entry)
AFAIK checking for DCHECK() failure like this is against the style guide.
See near the bottom of this page:
https://www.chromium.org/developers/coding-style
Search for "you should not handle DCHECK() failures"

[jww, 2015-07-01 22:25:46]

https://codereview.chromium.org/1136473006/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:316: if (!navigation_entry)
On 2015/07/01 22:16:50, bondd wrote:
> AFAIK checking for DCHECK() failure like this is against the style guide.
> See near the bottom of this page:
> https://www.chromium.org/developers/coding-style
> Search for "you should not handle DCHECK() failures"

More specifically, this should not be a DCHECK() as the GetLastCommittedEntry()
contract states that it may return NULL, and DCHECK should not be used to handle
exceptional cases.

[sigbjorn, 2015-07-02 14:46:39]

On 2015/07/01 22:07:42, bondd wrote:

> Hi sigbjorn, unfortunately I am not very familiar with this code.
> I took a look anyway and found this line
>   web::SSLStatus& sslStatus = [self currentEntry].navigationItem->GetSSL();
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/navigation...
> CRWSessionController also has a -lastCommittedEntry method you could use.
> AutofillClientIOS has access to ios::ChromeBrowserState through
browser_state_.
> My guess is that you could find CRWSessionController somehow from
> ChromeBrowserState, and then do
>   [sessionController lastCommittedEntry].navigationItem->GetSSL();
> I'm not sure how to get from ChromeBrowserState to CRWSessionController.
> Hopefully you can figure that part out. At this point I'm just spelunking
> through codesearch, same as you would be. Also, this is just a guess. If
anyone
> has more concrete info please let us know.

Unfortunately I do not have an iOS development setup, so it won't be done in
this round :( Once both Android and iOS have existing implementations, it should
be possible to move the API return the SSLStatus and move the last few lines of
the code to autofill_manager.

boliu: I'll try a blind fix of copying and pasting the code to
aw_autofill_client. The only indications of it working or not will be CQ runs.

[sigbjorn, 2015-07-02 14:58:37]

The CQ bit was checked by sigbjorn@opera.com to run a CQ dry run

[sigbjorn, 2015-07-02 14:58:38]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
jww@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps140001
(title: "Blind fix for Android")

[commit-bot: I haz the power, 2015-07-02 14:58:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136473006/140001

[commit-bot: I haz the power, 2015-07-02 15:09:42]

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

[commit-bot: I haz the power, 2015-07-02 15:09:44]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[sigbjorn, 2015-07-02 15:15:49]

The CQ bit was checked by sigbjorn@opera.com to run a CQ dry run

[sigbjorn, 2015-07-02 15:15:49]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
jww@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps160001
(title: "[chromium-style] Virtual methods shouldn't be declared inline")

[commit-bot: I haz the power, 2015-07-02 15:16:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136473006/160001

[commit-bot: I haz the power, 2015-07-02 15:35:41]

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

[commit-bot: I haz the power, 2015-07-02 15:35:43]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

(exceeded global retry quota)

[sigbjorn, 2015-07-03 08:05:09]

The CQ bit was checked by sigbjorn@opera.com to run a CQ dry run

[sigbjorn, 2015-07-03 08:05:10]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
jww@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps180001
(title: "More blind Android fixes")

[commit-bot: I haz the power, 2015-07-03 08:05:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136473006/180001

[commit-bot: I haz the power, 2015-07-03 08:21:27]

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

[commit-bot: I haz the power, 2015-07-03 08:21:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

(exceeded global retry quota)

[sigbjorn, 2015-07-03 08:25:29]

The CQ bit was checked by sigbjorn@opera.com to run a CQ dry run

[sigbjorn, 2015-07-03 08:25:30]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
jww@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps200001
(title: "Blind Android fix")

[commit-bot: I haz the power, 2015-07-03 08:25:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136473006/200001

[commit-bot: I haz the power, 2015-07-03 10:43:32]

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

[commit-bot: I haz the power, 2015-07-03 10:43:35]

Dry run: This issue passed the CQ dry run.

[sigbjorn, 2015-07-06 07:45:32]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2015-07-06 07:46:01]

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

[commit-bot: I haz the power, 2015-07-06 07:54:11]

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

[commit-bot: I haz the power, 2015-07-06 07:54:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sigbjorn, 2015-07-06 08:37:37]

Missing LGTM from an OWNER for these files:
    android_webview/native/aw_autofill_client.cc
    android_webview/native/aw_autofill_client.h
    ios/chrome/browser/ui/autofill/autofill_client_ios.h
    ios/chrome/browser/ui/autofill/autofill_client_ios.mm

[boliu, 2015-07-06 17:56:07]

lgtm % comment

https://codereview.chromium.org/1136473006/diff/200001/android_webview/native...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/1136473006/diff/200001/android_webview/native...
android_webview/native/aw_autofill_client.cc:186: content::SSLStatus ssl_status;
can you add a comment here and in ChromeAutofillClient::IsContextSecure, that
these two methods are supposed to match, and be kept in sync

[sigbjorn, 2015-07-07 07:51:07]

The CQ bit was checked by sigbjorn@opera.com

[sigbjorn, 2015-07-07 07:51:08]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
estade@chromium.org, jww@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps220001
(title: "Add comments")

[commit-bot: I haz the power, 2015-07-07 07:51:28]

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

[commit-bot: I haz the power, 2015-07-07 07:58:14]

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

[commit-bot: I haz the power, 2015-07-07 07:58:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sigbjorn, 2015-07-07 08:02:21]

sigbjorn@opera.com changed reviewers:
+ jdonnelly@chromium.org

[sigbjorn, 2015-07-07 08:02:22]

dconnelly / jdonnelly?

Missing LGTM from an OWNER for these files:
    ios/chrome/browser/ui/autofill/autofill_client_ios.h
    ios/chrome/browser/ui/autofill/autofill_client_ios.mm

[Justin Donnelly, 2015-07-07 14:18:16]

lgtm with nit

https://codereview.chromium.org/1136473006/diff/220001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/autofill/autofill_client_ios.mm (right):

https://codereview.chromium.org/1136473006/diff/220001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:157: // the form_origin.
See crbug.com/505388
Add period to the end.

[sigbjorn, 2015-07-08 09:17:29]

The CQ bit was checked by sigbjorn@opera.com

[sigbjorn, 2015-07-08 09:17:30]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
estade@chromium.org, jww@chromium.org, palmer@chromium.org,
jdonnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/1136473006/#ps240001
(title: "Add period")

[commit-bot: I haz the power, 2015-07-08 09:18:13]

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

[commit-bot: I haz the power, 2015-07-08 10:11:23]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-08 10:12:11]

Patchset 13 (id:??) landed as
https://crrev.com/337de86ef89076ef6a442acfc6e2904a0827d389
Cr-Commit-Position: refs/heads/master@{#337796}

[Evan Stade, 2015-07-08 21:05:53]

On 2015/07/08 10:12:11, commit-bot: I haz the power wrote:
> Patchset 13 (id:??) landed as
> https://crrev.com/337de86ef89076ef6a442acfc6e2904a0827d389
> Cr-Commit-Position: refs/heads/master@{#337796}

woohoo! Thanks for sticking with this CL and seeing it through to everyone's
satisfaction.