[WebPayments] Desktop: Adding validation and binding checks

[WebPayments] Desktop: Adding validation and binding checks when initializing or showing PaymentRequest.

BUG=673430
Review-Url: https://codereview.chromium.org/2568933002
Cr-Commit-Position: refs/heads/master@{#442371}
Committed: https://chromium.googlesource.com/chromium/src/+/8ce922854237356ea07d4a5a67aa6d8289470f36

[tmartino, 2016-12-12 19:34:09]

Description was changed from

==========
[WebPayments] Desktop: Moving display code from Init to Show.

BUG=
==========

to

==========
[WebPayments] Desktop: Moving display code from Init to Show.

BUG=673430
==========

[tmartino, 2016-12-12 19:34:32]

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

[tmartino, 2016-12-12 19:35:30]

https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
File chrome/browser/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
chrome/browser/payments/payment_request_impl.cc:67: void
PaymentRequestImpl::Show() {
It seems like this needs to have a check to ensure it was Init'd first, yeah? Do
we have a policy on what should happen in this case? DCHECK? CHECK? Return with
no-op? Call OnError?

[please use gerrit instead, 2016-12-12 20:46:49]

https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
File chrome/browser/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
chrome/browser/payments/payment_request_impl.cc:67: void
PaymentRequestImpl::Show() {
On 2016/12/12 19:35:30, tmartino wrote:
> It seems like this needs to have a check to ensure it was Init'd first, yeah?
Do
> we have a policy on what should happen in this case? DCHECK? CHECK? Return
with
> no-op? Call OnError?
------------------------------
Init():

Validate |details| using:

https://cs.chromium.org/chromium/src/components/payments/payment_details_vali...

If the |details| are invalid, make sure that |client->OnError(USER_CANCEL)| is
called and both pipes to the renderer are disconnected. The two pipes are
|binding_| and |client_|.

The renderer should not be sending invalid data, so this means the renderer has
been compromised.

------------------------------
Show():

If you don't have an open pipe to the renderer, return. There's nothing you can
do. (Don't show UI.)
------------------------------

[Mathieu, 2017-01-05 15:53:54]

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

[Mathieu, 2017-01-05 15:53:55]

https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
File chrome/browser/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
chrome/browser/payments/payment_request_impl.cc:67: void
PaymentRequestImpl::Show() {
On 2016/12/12 20:46:48, rouslan wrote:
> On 2016/12/12 19:35:30, tmartino wrote:
> > It seems like this needs to have a check to ensure it was Init'd first,
yeah?
> Do
> > we have a policy on what should happen in this case? DCHECK? CHECK? Return
> with
> > no-op? Call OnError?
> ------------------------------
> Init():
> 
> Validate |details| using:
> 
>
https://cs.chromium.org/chromium/src/components/payments/payment_details_vali...
> 
> If the |details| are invalid, make sure that |client->OnError(USER_CANCEL)| is
> called and both pipes to the renderer are disconnected. The two pipes are
> |binding_| and |client_|.
> 
> The renderer should not be sending invalid data, so this means the renderer
has
> been compromised.
> 
> ------------------------------
> Show():
> 
> If you don't have an open pipe to the renderer, return. There's nothing you
can
> do. (Don't show UI.)
> ------------------------------

Note that while some of this code has landed in another CL, Rouslan's comments
are still worth implementing.

[tmartino, 2017-01-05 15:54:59]

On 2017/01/05 at 15:53:55, mathp wrote:
>
https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
> File chrome/browser/payments/payment_request_impl.cc (right):
> 
>
https://codereview.chromium.org/2568933002/diff/1/chrome/browser/payments/pay...
> chrome/browser/payments/payment_request_impl.cc:67: void
PaymentRequestImpl::Show() {
> On 2016/12/12 20:46:48, rouslan wrote:
> > On 2016/12/12 19:35:30, tmartino wrote:
> > > It seems like this needs to have a check to ensure it was Init'd first,
yeah?
> > Do
> > > we have a policy on what should happen in this case? DCHECK? CHECK? Return
> > with
> > > no-op? Call OnError?
> > ------------------------------
> > Init():
> > 
> > Validate |details| using:
> > 
> >
https://cs.chromium.org/chromium/src/components/payments/payment_details_vali...
> > 
> > If the |details| are invalid, make sure that |client->OnError(USER_CANCEL)|
is
> > called and both pipes to the renderer are disconnected. The two pipes are
> > |binding_| and |client_|.
> > 
> > The renderer should not be sending invalid data, so this means the renderer
has
> > been compromised.
> > 
> > ------------------------------
> > Show():
> > 
> > If you don't have an open pipe to the renderer, return. There's nothing you
can
> > do. (Don't show UI.)
> > ------------------------------
> 
> Note that while some of this code has landed in another CL, Rouslan's comments
are still worth implementing.

Agreed. I'm in progress on them and I'll send out a revised CL this afternoon.

[please use gerrit instead, 2017-01-05 16:09:16]

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

[commit-bot: I haz the power, 2017-01-05 16:09:33]

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-01-05 16:15:11]

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

[commit-bot: I haz the power, 2017-01-05 16:15:12]

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

[tmartino, 2017-01-05 19:15:27]

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

[commit-bot: I haz the power, 2017-01-05 19:16:00]

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-01-05 20:03:12]

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

[commit-bot: I haz the power, 2017-01-05 20:03:13]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-01-05 20:05:40]

PTAL

[please use gerrit instead, 2017-01-05 20:11:58]

https://codereview.chromium.org/2568933002/diff/20001/chrome/browser/payments...
File chrome/browser/payments/payment_request_impl.cc (right):

https://codereview.chromium.org/2568933002/diff/20001/chrome/browser/payments...
chrome/browser/payments/payment_request_impl.cc:67: if
(!validatePaymentDetails(details, &err_msg)) {
Also:

LOG(ERROR) << err_msg;

By the way, you may also want to close binding_ and call that
UnassignPaymentRequest thingy.

You don't have to call client_->OnError(), because that tells renderer that an
error occurred. In fact, if this validation error happens in the browser, then
the renderer is already compromised. So there's no need to talk back to it.
Therefore, client_.reset() is sufficient.

https://codereview.chromium.org/2568933002/diff/20001/chrome/browser/payments...
chrome/browser/payments/payment_request_impl.cc:76: }
Can you do this? it should ensure that PaymentRequestImpl is deleted, I think.
krb@ originally designed the factory and knows best how it works, but I hear
mathp@ wants to improve on it.

if (!client_.is_bound() || !binding_.is_bound()) {
  payment_request_factory.Get().UnassignPaymentRequest(web_contents_);
  return;
}


// The rest of code.

[please use gerrit instead, 2017-01-05 20:12:34]

Please update the CL description, btw.

[tmartino, 2017-01-05 20:44:31]

Description was changed from

==========
[WebPayments] Desktop: Moving display code from Init to Show.

BUG=673430
==========

to

==========
[WebPayments] Desktop: Adding validation and binding checks when initializing or
showing PaymentRequest.

BUG=673430
==========

[tmartino, 2017-01-05 20:44:45]

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

[commit-bot: I haz the power, 2017-01-05 20:45:27]

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

[tmartino, 2017-01-05 20:45:44]

Done, thanks. PTAL again :)

[please use gerrit instead, 2017-01-05 21:03:31]

Description was changed from

==========
[WebPayments] Desktop: Adding validation and binding checks when initializing or
showing PaymentRequest.

BUG=673430
==========

to

==========
[WebPayments] Desktop: Adding validation and binding checks when initializing or
showing PaymentRequest.

BUG=673430
==========

[please use gerrit instead, 2017-01-05 21:04:01]

lgtm

[commit-bot: I haz the power, 2017-01-05 21:47:49]

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

[commit-bot: I haz the power, 2017-01-05 21:47:50]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-01-06 16:31:22]

lgtm

[tmartino, 2017-01-09 19:48:55]

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

[commit-bot: I haz the power, 2017-01-09 19:49:23]

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

[tmartino, 2017-01-09 20:03:58]

tmartino@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[tmartino, 2017-01-09 20:03:59]

+anthonyvd to take a quick look. Anthony, you touched some of this code, so I
wanted to make sure you're happy with how I rebased on top of your changes.

[anthonyvd, 2017-01-09 20:05:35]

lgtm

[Mathieu, 2017-01-09 20:07:01]

On 2017/01/09 20:05:35, anthonyvd wrote:
> lgtm

BTW, this is being submitted so there will be a slight rebase:
https://codereview.chromium.org/2611253004/

[commit-bot: I haz the power, 2017-01-09 20:47:20]

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

[commit-bot: I haz the power, 2017-01-09 20:47:21]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-01-09 20:54:25]

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

[commit-bot: I haz the power, 2017-01-09 20:55:15]

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

[tmartino, 2017-01-09 21:01:02]

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

[commit-bot: I haz the power, 2017-01-09 21:02:13]

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-01-09 22:07:01]

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

[commit-bot: I haz the power, 2017-01-09 22:07:02]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-01-09 22:10:52]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-01-09 22:10:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2568933002/#ps140001
(title: "Missed line in rebase")

[commit-bot: I haz the power, 2017-01-09 22:11:48]

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-01-09 22:23:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483999852027340,
"parent_rev": "82712717bb8f87ad7cc66e831d61d5fcb5352053", "commit_rev":
"8ce922854237356ea07d4a5a67aa6d8289470f36"}

[commit-bot: I haz the power, 2017-01-09 22:23:41]

Description was changed from

==========
[WebPayments] Desktop: Adding validation and binding checks when initializing or
showing PaymentRequest.

BUG=673430
==========

to

==========
[WebPayments] Desktop: Adding validation and binding checks when initializing or
showing PaymentRequest.

BUG=673430

Review-Url: https://codereview.chromium.org/2568933002
Cr-Commit-Position: refs/heads/master@{#442371}
Committed:
https://chromium.googlesource.com/chromium/src/+/8ce922854237356ea07d4a5a67aa...
==========

[commit-bot: I haz the power, 2017-01-09 22:23:44]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/8ce922854237356ea07d4a5a67aa...