Prevent usage of web payments API over insecure HTTPS.

Prevent usage of web payments API over insecure HTTPS.

Before this patch, the web payments UI would allow user to make payments
easily on pages with invalid HTTPS certificates. Even if the URL bar
showed a red, crossed-out "https", the web payments UI would show a
green "https" with a green lock icon.

This patch fixes the problem by checking the security level of the page.
An HTTPS page that's not EV_SECURE, SECURE, or
SECURE_WITH_POLICY_INSTALLED_CERT is prevented from using any payment
apps.

After this patch, invoking PaymentRequest.show() will always return
NotSupportedError on pages with invalid HTTPS certificates. This is
because Chrome is not providing any payment apps for such pages.
Invoking PaymentRequest.canMakePayment() will always return "false" for
the same reason.

Caveat: Pages with invalid HTTPS certificates are still considered
"SecureContext" in web platform, so throwing "SecurityError" in the
PaymentRequest constructor is not an option.

To test an invalid HTTPS certificate:
1) Visit https://edellroot.badssl.com/input/web-payment.
2) Bypass the interstitial.
3) Tap [Initiate payment] button.
   Observe: The web payments UI does not show.

To test a valid HTTPS certificate:
1) Visit https://badssl.com/input/web-payment.
2) Tap [Initiate payment] button.
   Observe: The web payments UI shows.

BUG=678764
Review-Url: https://codereview.chromium.org/2815763002
Cr-Commit-Position: refs/heads/master@{#465022}
Committed: https://chromium.googlesource.com/chromium/src/+/6e3cf7c6d281cff41cae5aa4d764f6fd7854c75e

[please use gerrit instead, 2017-04-11 22:43:37]

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

[commit-bot: I haz the power, 2017-04-11 22:44:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2017-04-11 22:45:45]

rouslan@chromium.org changed reviewers:
+ gogerald@chromium.org, palmer@chromium.org

[please use gerrit instead, 2017-04-11 22:45:46]

Chris, security PTAL.

Ganggui, Java PTAL.

Anthony, test cases FYI. Eventually these test cases should behave this way on
desktop as well.

[commit-bot: I haz the power, 2017-04-11 23:27:09]

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

[commit-bot: I haz the power, 2017-04-11 23:27:10]

Dry run: This issue passed the CQ dry run.

[palmer, 2017-04-11 23:30:19]

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:423:
boolean isLocalhost = "localhost".equalsIgnoreCase(uri.getHost())
Unfortunately, sigh, the name localhost is not guaranteed to actually resolve to
an IP address assigned to the loopback interface. Apparently, in practice, it
sometimes actually does not.

(I suppose the same could be true of 127.0.0.1 and of ::1, but in practice we
don't see this happening, AFAIK.)

That said, I think it might be best to call into 

./third_party/WebKit/public/web/WebDocument.h:75:  BLINK_EXPORT bool
IsSecureContext() const;

instead.

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:429:
disconnectFromClientWithDebugMessage("Not in SecureContext");
Tiny nit: I'd say "Not in a secure context".

[gogerald1, 2017-04-12 17:04:50]

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:443:
if (!isLocalhost && !isLocalFile && !isSecureHttps) {
would it be better to put all these logic in C++ side so as to make it cross
platform? or put these logic in a separate function at least?

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:444:
onAllPaymentAppsCreated();
corrupt the meaning of our metrics here?
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[please use gerrit instead, 2017-04-13 21:35:56]

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

[commit-bot: I haz the power, 2017-04-13 21:36:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 21:48:08]

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

[commit-bot: I haz the power, 2017-04-13 21:48:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[please use gerrit instead, 2017-04-13 21:51:17]

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

[please use gerrit instead, 2017-04-13 21:51:24]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-13 21:51:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 22:52:32]

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

[commit-bot: I haz the power, 2017-04-13 22:52:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[please use gerrit instead, 2017-04-14 15:03:07]

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

[commit-bot: I haz the power, 2017-04-14 15:03:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2017-04-14 15:09:55]

rouslan@chromium.org changed reviewers:
+ mathp@chromium.org

[please use gerrit instead, 2017-04-14 15:09:57]

Chris and Ganggui: PTAL patch 3. Sorry it became huge after making the behavior
crossplatform. On the bright side, desktop now behaves exactly the same as
Android!

Mathieu: PTAL C++ bits in patch 3. I've also implemented NotSupportedError for
desktop, because that's how invalid SSL certificates are handled. I've added a
test for NotSupportedError on desktop, but the SSL tests are manual, so I will
add them to the test doc.

Mathieu: Please note that I've removed validation from payment_request_spec.cc,
because the validation already happens in payment_request.cc, immediately before
passing the data to payment_request_spec.cc.

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:423:
boolean isLocalhost = "localhost".equalsIgnoreCase(uri.getHost())
On 2017/04/11 23:30:19, palmer wrote:
> Unfortunately, sigh, the name localhost is not guaranteed to actually resolve
to
> an IP address assigned to the loopback interface. Apparently, in practice, it
> sometimes actually does not.
> 
> (I suppose the same could be true of 127.0.0.1 and of ::1, but in practice we
> don't see this happening, AFAIK.)
> 
> That said, I think it might be best to call into 
> 
> ./third_party/WebKit/public/web/WebDocument.h:75:  BLINK_EXPORT bool
> IsSecureContext() const;
> 
> instead.

Done. See origin_security_checker.cc.

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:429:
disconnectFromClientWithDebugMessage("Not in SecureContext");
On 2017/04/11 23:30:19, palmer wrote:
> Tiny nit: I'd say "Not in a secure context".

Done.

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:443:
if (!isLocalhost && !isLocalFile && !isSecureHttps) {
On 2017/04/12 17:04:50, gogerald1 wrote:
> would it be better to put all these logic in C++ side so as to make it cross
> platform? or put these logic in a separate function at least?

Done. See origin_security_checker.h and ssl_validity_checker.h.

https://codereview.chromium.org/2815763002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:444:
onAllPaymentAppsCreated();
On 2017/04/12 17:04:50, gogerald1 wrote:
> corrupt the meaning of our metrics here?
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Bad SSLs should not happen for too many users, so it's OK to log the "no payment
methods found" metric for them, IMHO.

[please use gerrit instead, 2017-04-14 15:10:51]

rouslan@chromium.org changed reviewers:
+ mahmadi@chromium.org

[please use gerrit instead, 2017-04-14 15:10:51]

Moe: PTAL iOS in ptach 3.

[commit-bot: I haz the power, 2017-04-14 16:03:59]

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

[commit-bot: I haz the power, 2017-04-14 16:03:59]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-04-14 17:31:52]

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

[commit-bot: I haz the power, 2017-04-14 17:32:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 18:33:27]

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

[commit-bot: I haz the power, 2017-04-14 18:33:28]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-04-17 03:20:45]

Colossal work! lgtm with tests added

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:60: return
SslValidityChecker::IsValidSslCertificate(web_contents_);
This is apparently true in tests? How?

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
File chrome/browser/payments/ssl_validity_checker.h (right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
chrome/browser/payments/ssl_validity_checker.h:18: // Returns true for
|web_contents| with a valid SSL certificate.
would be good to document a little bit what you indicated in the cl description

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_browsertest.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest.cc:41: class
PaymentRequestNoShippingTest : public PaymentRequestBrowserTestBase {
Do you think we could have a test that tries to show the UI on an page with
invalid ssl? See my other comment in can_make_payment_browsertest about how to
achieve this with the test delegate

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_browsertest_base.cc
(right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:206: for
(const auto& expected_string : expected_strings) {
Thanks :) let's use const std::string&

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc
(right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc:16:
class PaymentRequestCanMakePaymentQueryTest
could we add a test for an invalid ssl page, expecting false?
PaymentRequestBrowserTestBase has a handle on the
TestChromePaymentRequestDelegate, in case you want it to do specific things

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_payment_app_browsertest.cc
(right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_payment_app_browsertest.cc:26:
ExpectBodyContains({"NotSupportedError"});
hopefully sometime soon this will return something else :)

https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
File components/payments/content/payment_request_spec_unittest.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
components/payments/content/payment_request_spec_unittest.cc:43: // Test that
empty method data notifies observers of an invalid spec.
fix

https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
components/payments/content/payment_request_state.cc:54:
spec_->supported_card_networks_set().count(
can you file a bug that this line is unnecessary since available_instruments_
only contains supported instruments

[Moe, 2017-04-17 13:26:18]

On 2017/04/17 03:20:45, Mathieu wrote:
> Colossal work! lgtm with tests added
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
> File chrome/browser/payments/chrome_payment_request_delegate.cc (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
> chrome/browser/payments/chrome_payment_request_delegate.cc:60: return
> SslValidityChecker::IsValidSslCertificate(web_contents_);
> This is apparently true in tests? How?
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
> File chrome/browser/payments/ssl_validity_checker.h (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
> chrome/browser/payments/ssl_validity_checker.h:18: // Returns true for
> |web_contents| with a valid SSL certificate.
> would be good to document a little bit what you indicated in the cl
description
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/payments/payment_request_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/payments/payment_request_browsertest.cc:41: class
> PaymentRequestNoShippingTest : public PaymentRequestBrowserTestBase {
> Do you think we could have a test that tries to show the UI on an page with
> invalid ssl? See my other comment in can_make_payment_browsertest about how to
> achieve this with the test delegate
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/payments/payment_request_browsertest_base.cc
> (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:206: for
> (const auto& expected_string : expected_strings) {
> Thanks :) let's use const std::string&
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
> File
>
chrome/browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
>
chrome/browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc:16:
> class PaymentRequestCanMakePaymentQueryTest
> could we add a test for an invalid ssl page, expecting false?
> PaymentRequestBrowserTestBase has a handle on the
> TestChromePaymentRequestDelegate, in case you want it to do specific things
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
> File
chrome/browser/ui/views/payments/payment_request_payment_app_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
>
chrome/browser/ui/views/payments/payment_request_payment_app_browsertest.cc:26:
> ExpectBodyContains({"NotSupportedError"});
> hopefully sometime soon this will return something else :)
> 
>
https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
> File components/payments/content/payment_request_spec_unittest.cc (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
> components/payments/content/payment_request_spec_unittest.cc:43: // Test that
> empty method data notifies observers of an invalid spec.
> fix
> 
>
https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
> File components/payments/content/payment_request_state.cc (right):
> 
>
https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
> components/payments/content/payment_request_state.cc:54:
> spec_->supported_card_networks_set().count(
> can you file a bug that this line is unnecessary since available_instruments_
> only contains supported instruments

i/c/b/payments lgtm

[please use gerrit instead, 2017-04-17 16:59:30]

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

[commit-bot: I haz the power, 2017-04-17 16:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gogerald1, 2017-04-17 17:26:41]

lgtm with a nit suggestion,

https://codereview.chromium.org/2815763002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/SslValidityChecker.java
(right):

https://codereview.chromium.org/2815763002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/SslValidityChecker.java:19:
public static boolean isValidSslCertificate(WebContents webContents) {
nit: isSslCertificateValid?

[please use gerrit instead, 2017-04-17 17:54:40]

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

[commit-bot: I haz the power, 2017-04-17 17:55:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2017-04-17 18:19:55]

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:60: return
SslValidityChecker::IsValidSslCertificate(web_contents_);
On 2017/04/17 03:20:45, Mathieu wrote:
> This is apparently true in tests? How?

Tests use such URLs as
https://127.0.0.1:45436/payment_request_no_shipping_test.html and
https://a.com:42667/payment_request_main.html. These have valid HTTPS certs.

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
File chrome/browser/payments/ssl_validity_checker.h (right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/payments...
chrome/browser/payments/ssl_validity_checker.h:18: // Returns true for
|web_contents| with a valid SSL certificate.
On 2017/04/17 03:20:45, Mathieu wrote:
> would be good to document a little bit what you indicated in the cl
description

Done.

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_browsertest.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest.cc:41: class
PaymentRequestNoShippingTest : public PaymentRequestBrowserTestBase {
On 2017/04/17 03:20:45, Mathieu wrote:
> Do you think we could have a test that tries to show the UI on an page with
> invalid ssl? See my other comment in can_make_payment_browsertest about how to
> achieve this with the test delegate

Done.

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_browsertest_base.cc
(right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_browsertest_base.cc:206: for
(const auto& expected_string : expected_strings) {
On 2017/04/17 03:20:45, Mathieu wrote:
> Thanks :) let's use const std::string&

Done.

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc
(right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc:16:
class PaymentRequestCanMakePaymentQueryTest
On 2017/04/17 03:20:45, Mathieu wrote:
> could we add a test for an invalid ssl page, expecting false?
> PaymentRequestBrowserTestBase has a handle on the
> TestChromePaymentRequestDelegate, in case you want it to do specific things

Done.

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_payment_app_browsertest.cc
(right):

https://codereview.chromium.org/2815763002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_payment_app_browsertest.cc:26:
ExpectBodyContains({"NotSupportedError"});
On 2017/04/17 03:20:45, Mathieu wrote:
> hopefully sometime soon this will return something else :)

Acknowledged.

https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2815763002/diff/80001/components/payments/con...
components/payments/content/payment_request_state.cc:54:
spec_->supported_card_networks_set().count(
On 2017/04/17 03:20:45, Mathieu wrote:
> can you file a bug that this line is unnecessary since available_instruments_
> only contains supported instruments

I've changed this to DCHECK instead.

https://codereview.chromium.org/2815763002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/SslValidityChecker.java
(right):

https://codereview.chromium.org/2815763002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/SslValidityChecker.java:19:
public static boolean isValidSslCertificate(WebContents webContents) {
On 2017/04/17 17:26:41, gogerald1 wrote:
> nit: isSslCertificateValid?

Done.

[commit-bot: I haz the power, 2017-04-17 19:06:20]

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

[commit-bot: I haz the power, 2017-04-17 19:06:21]

Dry run: This issue passed the CQ dry run.

[palmer, 2017-04-17 19:30:06]

LGTM % nit and question.

https://codereview.chromium.org/2815763002/diff/120001/components/payments/co...
File
components/payments/content/android/java/src/org/chromium/components/payments/OriginSecurityChecker.java
(right):

https://codereview.chromium.org/2815763002/diff/120001/components/payments/co...
components/payments/content/android/java/src/org/chromium/components/payments/OriginSecurityChecker.java:24:
* Returns true for a valid URL with a cryptoraphic scheme, e.g., HTTPS,
HTTPS-SO, WSS.
Nit: typo: "cryptographic".

https://codereview.chromium.org/2815763002/diff/120001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request.mm (right):

https://codereview.chromium.org/2815763002/diff/120001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request.mm:111: // TODO(crbug.com/709036):
Validate method data.
How will the caller distinguish empty data from invalid input? Rejected Promise
vs. empty result? Is that important? (I.e. why change from bool to void)

[please use gerrit instead, 2017-04-17 20:06:10]

https://codereview.chromium.org/2815763002/diff/120001/components/payments/co...
File
components/payments/content/android/java/src/org/chromium/components/payments/OriginSecurityChecker.java
(right):

https://codereview.chromium.org/2815763002/diff/120001/components/payments/co...
components/payments/content/android/java/src/org/chromium/components/payments/OriginSecurityChecker.java:24:
* Returns true for a valid URL with a cryptoraphic scheme, e.g., HTTPS,
HTTPS-SO, WSS.
On 2017/04/17 19:30:06, palmer wrote:
> Nit: typo: "cryptographic".

Done.

https://codereview.chromium.org/2815763002/diff/120001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request.mm (right):

https://codereview.chromium.org/2815763002/diff/120001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request.mm:111: // TODO(crbug.com/709036):
Validate method data.
On 2017/04/17 19:30:06, palmer wrote:
> How will the caller distinguish empty data from invalid input? Rejected
Promise
> vs. empty result? Is that important? (I.e. why change from bool to void)

The renderer should have validated all data beforehand and prevented invalid
data going into the browser. If the browser receives invalid data nevertheless,
then the renderer has been compromised. The web payments API should hard-abort
and disconnect from the renderer. (Terminating the renderer would be best.)

The old function used to treat absence of "basic-card" in the
|web_payment_request_.method_data| as invalid data from the renderer
(hard-abort). This does not match the spec. Instead, the code should perform
these two operations gracefully:
1) Resolve any caneMakePayment() promise with "false".
2) Reject any show() promise with "NotSupportedError".

I've added this information to the bug in the TODO comment.

[please use gerrit instead, 2017-04-17 20:06:30]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-04-17 20:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
mahmadi@chromium.org, gogerald@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/2815763002/#ps140001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-04-17 20:06:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 21:23:33]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492459590293680,
"parent_rev": "780ef7b430dc6976831ae4f952cf34579b101f70", "commit_rev":
"6e3cf7c6d281cff41cae5aa4d764f6fd7854c75e"}

[commit-bot: I haz the power, 2017-04-17 21:23:47]

Description was changed from

==========
Prevent usage of web payments API over insecure HTTPS.

Before this patch, the web payments UI would allow user to make payments
easily on pages with invalid HTTPS certificates. Even if the URL bar
showed a red, crossed-out "https", the web payments UI would show a
green "https" with a green lock icon.

This patch fixes the problem by checking the security level of the page.
An HTTPS page that's not EV_SECURE, SECURE, or
SECURE_WITH_POLICY_INSTALLED_CERT is prevented from using any payment
apps.

After this patch, invoking PaymentRequest.show() will always return
NotSupportedError on pages with invalid HTTPS certificates. This is
because Chrome is not providing any payment apps for such pages.
Invoking PaymentRequest.canMakePayment() will always return "false" for
the same reason.

Caveat: Pages with invalid HTTPS certificates are still considered
"SecureContext" in web platform, so throwing "SecurityError" in the
PaymentRequest constructor is not an option.

To test an invalid HTTPS certificate:
1) Visit https://edellroot.badssl.com/input/web-payment.
2) Bypass the interstitial.
3) Tap [Initiate payment] button.
   Observe: The web payments UI does not show.

To test a valid HTTPS certificate:
1) Visit https://badssl.com/input/web-payment.
2) Tap [Initiate payment] button.
   Observe: The web payments UI shows.

BUG=678764
==========

to

==========
Prevent usage of web payments API over insecure HTTPS.

Before this patch, the web payments UI would allow user to make payments
easily on pages with invalid HTTPS certificates. Even if the URL bar
showed a red, crossed-out "https", the web payments UI would show a
green "https" with a green lock icon.

This patch fixes the problem by checking the security level of the page.
An HTTPS page that's not EV_SECURE, SECURE, or
SECURE_WITH_POLICY_INSTALLED_CERT is prevented from using any payment
apps.

After this patch, invoking PaymentRequest.show() will always return
NotSupportedError on pages with invalid HTTPS certificates. This is
because Chrome is not providing any payment apps for such pages.
Invoking PaymentRequest.canMakePayment() will always return "false" for
the same reason.

Caveat: Pages with invalid HTTPS certificates are still considered
"SecureContext" in web platform, so throwing "SecurityError" in the
PaymentRequest constructor is not an option.

To test an invalid HTTPS certificate:
1) Visit https://edellroot.badssl.com/input/web-payment.
2) Bypass the interstitial.
3) Tap [Initiate payment] button.
   Observe: The web payments UI does not show.

To test a valid HTTPS certificate:
1) Visit https://badssl.com/input/web-payment.
2) Tap [Initiate payment] button.
   Observe: The web payments UI shows.

BUG=678764

Review-Url: https://codereview.chromium.org/2815763002
Cr-Commit-Position: refs/heads/master@{#465022}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e3cf7c6d281cff41cae5aa4d764...
==========

[commit-bot: I haz the power, 2017-04-17 21:23:48]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6e3cf7c6d281cff41cae5aa4d764...