Don't autofill credit cards on non-secure pages

chrome/browser/ui/autofill/chrome_autofill_client.cc

Index: chrome/browser/ui/autofill/chrome_autofill_client.cc
diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.cc b/chrome/browser/ui/autofill/chrome_autofill_client.cc
index b57836999a7a56ab5310afa7d5dbc792799ae741..584718232de370afd33ecedddb3291f8796bd2ae 100644
--- a/chrome/browser/ui/autofill/chrome_autofill_client.cc
+++ b/chrome/browser/ui/autofill/chrome_autofill_client.cc
@@ -33,6 +33,7 @@
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/user_prefs/user_prefs.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_frame_host.h"
 #include "ui/gfx/geometry/rect.h"
 
@@ -305,4 +306,18 @@ void ChromeAutofillClient::LinkClicked(const GURL& url,
       url, content::Referrer(), disposition, ui::PAGE_TRANSITION_LINK, false));
 }
 
+bool ChromeAutofillClient::IsContextSecure(const GURL& form_origin) {

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

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.

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

this param is not used

[sigbjorn, 2015/06/12 11:47:53]

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.

[sigbjorn, 2015/06/12 11:47:53]

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.

+  content::SSLStatus ssl_status;
+  content::NavigationEntry* navigation_entry =
+      web_contents()->GetController().GetVisibleEntry();

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

This is probably not what we want. We probably want GetLastCommitted, since
that's the content actually on the page.

+  DCHECK(navigation_entry);
+  if (!navigation_entry)
+     return false;
+
+  ssl_status = navigation_entry->GetSSL();
+  return ssl_status.security_style ==
+      content::SECURITY_STYLE_AUTHENTICATED &&

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

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.

[sigbjorn, 2015/06/12 11:47:53]

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?
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.

+      ssl_status.content_status == content::SSLStatus::NORMAL_CONTENT;
+}
+
 }  // namespace autofill