815148 - [WebPayment] DOMRequest mozPay events are being fired over a wrong target.

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[Fernando Jiménez Moreno [:ferjm]]

Steps to reproduce:

1. Launch app A.
2. Call navigator.mozPay, so the trusted UI will be shown containing a payment flow.
3. Click the HOME button, so the trusted UI will be closed.
4. Launch app B.
5. Repeat 2 and 3 steps on app B.
6. Launch app A again, so the trusted UI will be shown again.
7. Finish the payment flow.

Expected:
The trusted UI closes and *app A* receives the |DOMRequest.onsuccess| (or .onerror) event.

Actual:
The trusted UI closes and *app B* receives the |DOMRequest.onsuccess| (or .onerror) event.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Dispatching the result of a payment flow to a wrong target seems pretty bad, so I am nominating it for blocking basecamp.

Comment 2

[Jason Smith [:jsmith]]

This feels like a security issue - Raymond, do you agree? What sec severity would you suggest if you think this is a security issue?

Comment 3

[Jason Smith [:jsmith]]

Spoke with rforbes in IRC - sec-high it is

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 686583 [details] [diff] [review]
v1

Review of attachment 686583 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but see comments.

::: b2g/chrome/content/payment.js
@@ +78,5 @@
>  }
>  
> +// We save the identifier of the DOM request, so we can dispatch the results
> +// of the payment flow to the appropriate content process.
> +let requestId;

Nit: move that to the beginning of the file since requestId is used in paymentSuccess and paymentFailed

::: b2g/components/PaymentGlue.js
@@ +124,5 @@
>                          .frameLoader;
>        let mm = frameLoader.messageManager;
>        try {
>          mm.loadFrameScript(kPaymentShimFile, true);
> +        mm.sendAsyncMessage("Payment:LoadShim", { requestId: aRequestId });

Using an async message here, we can in theory have a race condition where payment.js does not have the requestId when paymentSuccess() or paymentError() are called. Let's pay the price of a sync message here.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> ::: b2g/components/PaymentGlue.js
> @@ +124,5 @@
> >                          .frameLoader;
> >        let mm = frameLoader.messageManager;
> >        try {
> >          mm.loadFrameScript(kPaymentShimFile, true);
> > +        mm.sendAsyncMessage("Payment:LoadShim", { requestId: aRequestId });
> 
> Using an async message here, we can in theory have a race condition where
> payment.js does not have the requestId when paymentSuccess() or
> paymentError() are called. Let's pay the price of a sync message here.

I am afraid that sending a sync message is not going to be possible. From [1]: "When a message is sent from chrome to content, it must be dispatched asynchronously, because chrome is not allowed to block on content. However, content scripts may synchronously send a message to chrome and wait for a response". So the message sent from chrome to the loaded content script needs to be async. In any case, as we discussed in person, I don't think there is a way that this race condition ever happens. Note that the 'paymentSuccess' and 'paymentError' callbacks are injected on 'DOMContentLoaded' event, which should happen way after the 'LoadShim' message is received.

I can add a check for the 'requestId' in the 'paymentSuccess' and 'paymentError' callbacks though.

[1] https://developer.mozilla.org/en-US/docs/The_message_manager

Comment 7

[[:fabrice] Fabrice Desré]

ok!

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c304d3db90e2

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c304d3db90e2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21c9aecc314f
https://hg.mozilla.org/releases/mozilla-beta/rev/1abf5abbc1d7

Comment 11

[Jason Smith [:jsmith]]

Verified on 1/25.