1429181 - Credit card network validation on entry and summary pages

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

When a new payment card is being used, determine it's type (e.g. Mastercard, Visa, etc.) and whether it's a debit/credit/prepaid card.

We can also provide hints if the card checksum doesn't pass the Luhn algorithm if it should for that type.

Comment 1

[Sam Foster [:sfoster] (he/him)]

We can determine the card network quite easily - at least for the limited subset of networks supported by basic card, which at this time is: 

- amex
- cartebancaire (France's national interbank network, out of scope for initial MVP)
- diners
- discover
- jcb
- mastercard
- mir (Russian national payment system, out of scope for initial MVP)
- unionpay
- visa

Source: https://www.w3.org/Payments/card-network-ids

Of those listed network ids, my current understanding is that only unionpay numbers may not pass the Luhn algorithm. 

In some cases we can infer if a card is debit or credit from the PAN, e.g. Maestro should match /^(5(018|0[23]|[68])|6(39|7))/ (source, see [1])

That provides reasonable coverage, but leaves some significant edge? cases like Interac. (Which however is not on the list of supported network ids, so should we even be including this in our discussion?)

ISTM to me that even if we do use a 3rd party BIN service, we can never guarantee a 100% match rate. So we need a fallback UX or UX that is tolerant of not having all the required information. That being the case, we can get started with one of the open source libraries or cobble together our own to get started here. 

I'm continuing to look around, but so far, the braintree library[1] and the payform library[1] look like places to start. 

1. https://github.com/jondavidjohn/payform/blob/master/src/payform.coffee
2. https://github.com/braintree/credit-card-type

So, I can make a start here. Validation I can figure out, but I'm still not clear what we want to do once we have figured out a card is a debit/credit card. Do we show an inline warning message if the merchant has indicated they can't accept certain networks or card types?

Comment 2

[Matthew N. [:MattN]]

Automatic credit card type/network detection isn't part of M3 so this bug was more about implementing UI changes related to use of certain networks or types.

I believe the UX specs were going to be updated to cover the warnings about using unacceptable networks/types.

Comment 3

[Matthew N. [:MattN]]

Making the summary clear that based on scope and UX changes that this will focus on supportedNetworks, not supportedTypes.

It should show warnings/errors on the add/edit and summary pages to help the user choose a card with a supported card network.

Comment 5

[Matthew N. [:MattN]]

In case you didn't notice, please fix https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/browser/components/payments/res/components/basic-card-option.js#21 to be `cc-type` and also remove the exclusion at https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/browser/components/payments/res/containers/payment-method-picker.js#34-35

Thanks

Comment 6

[Matthew N. [:MattN]]

Please also add a valid cc-type to some of the debugging records: https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/browser/components/payments/res/debugging.js#281

Comment 7

[Sam Foster [:sfoster] (he/him)]

It seems like just adding the data property to a payment method in a browser test is enough to produce this warning:

Console message: [JavaScript Warning: "Security wrapper denied access to property "supportedNetworks" on privileged Javascript object. ...."

The resulting data object - as received by the paymentDialogWrapper - is always empty.

I don't get the same result or warning when invoking the payment dialog manually - with the fix to the _serializeRequest method to properly return array contents, the payment method structure looks correct when you dump the state.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: supportedNetworks-test.diff

Comment 9

[Sam Foster [:sfoster] (he/him)]

Attachment: supportedNetworks-test.diff

run mach test browser/components/payments/test/browser/browser_request_serialization.js to reproduce

Comment 10

[Sam Foster [:sfoster] (he/him)]

Jared and I spent some time looking that this, stepping through in the debugger. 

1. Apply the patch in attachment 9006318 [details] [diff] [review] to add supportedNetworks to the instrument we use in the test, which is necessary to see this issue. That patch also fixed a bug in the serializing method, but I believe this has been masking an issue rather being the root source of the problem. 

2. Add a `debugger` statement to the top of initializeFrame() in paymentRequestDialog.js, before the call to this._serializeRequest(this.request);

3. Run the browser_request_serialization.js test:
mach mochitest --jsdebugger browser/components/payments/test/browser/browser_request_serialization.js

Any of the tests in browser_request_serialization.js should work. You can mark the first one only to run by adding .only() to the add_task() call. 

3. The test should start running and the debugger should pop up at the debugger break point. 

4. From here you can examine this.request. In the console, try: `this.request.paymentMethods.queryElementAt(0, Ci.nsIPaymentMethodData).data` and/or step through to get to the point in the recursive loop that serializes the paymentMethods nsiArray. It seems that the data property we trying to serialize is an empty object, rather than the expected { supportedNetworks: ["visa", "mastercard"] } structure. 

4.x At this point, you can already see the warning in the terminal "Console message: [JavaScript Warning: "Security wrapper denied access to property "supportedNetworks" on privileged Javascript object. ...."

5. You can conduct the same debugging steps manually (i.e. not using mochitest) with paymentrequest.show/demo/ to verify that the paymentMethods are populated properly in this context. 

The .idl for nsiPaymentMethodData marks the .data property as 'jsval'. I don't know if it is significant that in this case, the value is an object containing an array of strings? But something is peculiar here and although everything works as expected normally, inside the browser chrome test we are losing data and currently blocked from writing tests for this. 

Any ideas or suggestions Marcos?

Comment 11

[Marcos Caceres [:marcosc]]

I'm getting "error: corrupt patch at line 39" when trying to apply the patch. I'll try to construct manually and see if I can provide some useful feedback.

Comment 12

[Marcos Caceres [:marcosc]]

I wasn't able to get to this today :( My compiles are taking ~1 hour. 

However, it sounds like X-ray wrapper issue. 

@bholley, @eden, would these objects need to be treated in some special way? The biding layer should have handled this.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Sam Foster [:sfoster] from comment #10) 
> 4.x At this point, you can already see the warning in the terminal "Console
> message: [JavaScript Warning: "Security wrapper denied access to property
> "supportedNetworks" on privileged Javascript object. ...."

This means you're exposing a privileged JS object to untrusted script, and the security wrappers are saving your bacon. I'm not sure how your code is structured, but you should figure out why that happens, because it's bad and probably also the source of your problem.

Comment 14

[Sam Foster [:sfoster] (he/him)]

(In reply to Bobby Holley (:bholley) from comment #13)
> 
> This means you're exposing a privileged JS object to untrusted script, and
> the security wrappers are saving your bacon. I'm not sure how your code is
> structured, but you should figure out why that happens, because it's bad and
> probably also the source of your problem.

Yeah, the question is whether the bug is somewhere in web payment's test boilerplate (I don't think so?), in the mochitest browser-chrome test harness, or in the binding layer as Marcos suggests. It could also be that the bug is that the paymentMethods / supportedNetworks stuff *does* currently work without complaint outside the test harness when it shouldn't?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Sam Foster [:sfoster] from comment #14)
> (In reply to Bobby Holley (:bholley) from comment #13)
> > 
> > This means you're exposing a privileged JS object to untrusted script, and
> > the security wrappers are saving your bacon. I'm not sure how your code is
> > structured, but you should figure out why that happens, because it's bad and
> > probably also the source of your problem.
> 
> Yeah, the question is whether the bug is somewhere in web payment's test
> boilerplate (I don't think so?), in the mochitest browser-chrome test
> harness, or in the binding layer as Marcos suggests. It could also be that
> the bug is that the paymentMethods / supportedNetworks stuff *does*
> currently work without complaint outside the test harness when it shouldn't?

My hunch is that payment JS code somewhere is at least partially implicated.

Anyway, it's straightforward enough to find out. Run the test, set a breakpoint where that wrapper message is printed, and dump the C++ and JS stacks to get an idea of what's going on.

Comment 16

[Sam Foster [:sfoster] (he/him)]

Attachment: supportedNetworks-test.diff

Updating the patch which should yield failure in the test_serializeRequest_paymentMethods test, and the security wrapper warning.

Comment 17

[Marcos Caceres [:marcosc]]

Bobby, would it be possible for when the security wrapper warning happens to also show a JS line number? The JS debugger is extremely unstable (usually just crashes the browser if you try to interact with it), so having more hints as to where to look would be super helpful.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Marcos Caceres [:marcosc] from comment #17)
> Bobby, would it be possible for when the security wrapper warning happens to
> also show a JS line number? The JS debugger is extremely unstable (usually
> just crashes the browser if you try to interact with it), so having more
> hints as to where to look would be super helpful.

Should be possible. I'd take a patch to use xpc_PrintJSStack, though I'd probably want it preffed off by default (the log message can advertise the pref when the pref is false).

Comment 19

[Bobby Holley (:bholley)]

(In reply to Marcos Caceres [:marcosc] from comment #17)
> Bobby, would it be possible for when the security wrapper warning happens to
> also show a JS line number? The JS debugger is extremely unstable (usually
> just crashes the browser if you try to interact with it)

Oh, and to be clear - when I said "run the test in a debugger", I meant something like gdb.

Comment 20

[Matthew N. [:MattN]]

The stack is so this is coming from the merchant task to show the dialog in the test, not the shipping code:
 * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000118674cca XUL`xpc::ReportWrapperDenial(cx=0x0000000111328000, id=JS::HandleId @ 0x00007ffee2d9e0c8, type=WrapperDenialForCOW, reason="Access to privileged JS object not permitted") at XrayWrapper.cpp:189 [opt]
    frame #1: 0x000000011867d40d XUL`xpc::FilteringWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::OpaqueWithSilentFailing>::enter(this=<unavailable>, cx=0x0000000111328000, wrapper=<unavailable>, id=JS::HandleId @ r15, act=1, mayThrow=<unavailable>, bp=0x00007ffee2d9e339) const at FilteringWrapper.cpp:208 [opt]
    frame #2: 0x000000011c18f5f1 XUL`js::AutoEnterPolicy::AutoEnterPolicy(this=0x00007ffee2d9e330, cx=0x0000000111328000, handler=0x000000011edf8218, wrapper=JS::HandleObject @ r13, id=JS::HandleId @ r15, act=1, mayThrow=true) at Proxy.h:607 [opt]
    frame #3: 0x000000011c18509e XUL`js::Proxy::getInternal(cx=<unavailable>, proxy=JS::HandleObject @ 0x00007ffee2d9e318, receiver=<unavailable>, id=JS::HandleId @ r14, vp=<unavailable>) at Proxy.cpp:362 [opt]
    frame #4: 0x000000011c184f46 XUL`js::Proxy::get(cx=0x0000000111328000, proxy=JS::HandleObject @ 0x00007ffee2d9e3e8, receiver_=JS::HandleValue @ 0x00007ffee2d9e3d0, id=<unavailable>, vp=<unavailable>) at Proxy.cpp:394 [opt]
    frame #5: 0x000000011c2aa107 XUL`js::GetProperty(cx=0x0000000111328000, obj=JS::HandleObject @ 0x00007ffee2d9e438, receiver=JS::HandleValue @ r12, id=JS::HandleId @ r15, vp=JS::MutableHandleValue @ r14) at NativeObject.h:1722 [opt]
    frame #6: 0x000000011c129ed3 XUL`JS_ForwardGetPropertyTo(cx=0x0000000111328000, obj=JS::HandleObject @ 0x00007ffee2d9e478, id=JS::HandleId @ 0x00007ffee2d9e480, receiver=JS::HandleValue @ 0x00007ffee2d9e488, vp=JS::MutableHandleValue @ r14) at jsapi.cpp:2633 [opt]
    frame #7: 0x000000011c12a103 XUL`JS_GetPropertyById(cx=0x0000000111328000, obj=JS::HandleObject @ 0x00007ffee2d9e4d8, id=<unavailable>, vp=<unavailable>) at jsapi.cpp:2651 [opt]
    frame #8: 0x00000001190f1179 XUL`mozilla::dom::BasicCardRequest::Init(this=0x00007ffee2d9e6f8, cx=0x0000000111328000, val=<unavailable>, sourceDescription="Value", passedToJSImpl=<unavailable>) at BasicCardPaymentBinding.cpp:96 [opt]
    frame #9: 0x000000011a75e43c XUL`mozilla::dom::BasicCardService::IsValidBasicCardRequest(this=<unavailable>, aCx=0x0000000111328000, aData=<unavailable>, aErrorMsg=u"") at BasicCardPayment.cpp:159 [opt]
    frame #10: 0x000000011a763e56 XUL`mozilla::dom::PaymentRequest::IsValidMethodData(aCx=0x0000000111328000, aMethodData=<unavailable>, aErrorMsg=<unavailable>) at PaymentRequest.cpp:276 [opt]
    frame #11: 0x000000011a764cb0 XUL`mozilla::dom::PaymentRequest::Constructor(aGlobal=0x00007ffee2d9e940, aMethodData=0x00007ffee2d9eb20, aDetails=0x00007ffee2d9ea78, aOptions=0x00007ffee2d9ea18, aRv=0x00007ffee2d9e998) at PaymentRequest.cpp:573 [opt]

Comment 21

[Matthew N. [:MattN]]

Sam, the following is a workaround to unblock you:

diff --git a/browser/components/payments/test/PaymentTestUtils.jsm b/browser/components/payments/test/PaymentTestUtils.jsm
--- a/browser/components/payments/test/PaymentTestUtils.jsm
+++ b/browser/components/payments/test/PaymentTestUtils.jsm
@@ -84,5 +84,5 @@ var PaymentTestUtils = {
      */
     createAndShowRequest: ({methodData, details, options}) => {
-      const rq = new content.PaymentRequest(methodData, details, options);
+      const rq = new content.PaymentRequest(Cu.cloneInto(methodData, content), details, options);
       content.rq = rq; // assign it so we can retrieve it later
 
I'm going to file a follow-up to figure out why the BasicCardRequest dictionary is getting treated differently wrt. xray wrappers than the other arguments to PaymentRequest.

Comment 22

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1429181 - Consider supportedNetworks when determining if payment method is valid

* A new accepted-cards element to represent the labeled list of card icons
* Add the accepted cards section to the summary and card add/edit page
* mochitest for the accepted-cards element
* Make cc-type a required field and validate it against the list of supported networks
* Add verification of the pay button disabling when card network is not supported

* TODO/Questions: I need to confirm the behavior we want when adding/editing a card which doesnt match the supported networks for the current merchant. We want a warning there really? And not to prevent the user from completing the add/edit. 


MozReview-Commit-ID: LIgG1OkMVOP

Comment 23

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1429181 - Fix serialization of payment method data. r?MattN

* Includes a workaround to explicitly cloneInto the paymentMethods data when we create the PaymentRequest object in the content window in tests.

MozReview-Commit-ID: LFy0h3fIXXA

Comment 24

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1429181 - Consider supportedNetworks when determining if payment method is valid. r?MattN

* A new accepted-cards element to represent the labeled list of card icons
* Add the accepted cards section to the summary and card add/edit page
* mochitest for the accepted-cards element
* Make cc-type a required field and validate it against the list of supported networks
* Add verification of the pay button disabling when card network is not supported

* TODO/Questions: I need to confirm the behavior we want when adding/editing a card which doesnt match the supported networks for the current merchant. We want a warning there really? And not to prevent the user from completing the add/edit.

Depends on D5823

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 9008955 [details]
Bug 1429181 - Consider supportedNetworks when determining if payment method is valid. r?MattN

Matthew N. [:MattN] (PM me if requests are blocking you) has approved the revision.

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 9008953 [details]
Bug 1429181 - Fix serialization of payment method data. r?MattN

Matthew N. [:MattN] (PM me if requests are blocking you) has approved the revision.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1a4703b8464
Fix serialization of payment method data. r=MattN

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7351a4c8d1a1
Consider supportedNetworks when determining if payment method is valid. r=MattN

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 7351a4c8d1a1 (Bug 1429181) for multiple payment-related failures.

Push with failures:https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&revision=7351a4c8d1a12361238dc7df4fb8d5bfe03b2555

Backout link: https://hg.mozilla.org/integration/autoland/rev/e5670d433e4c5786ea4994263245834de7ba0f3a

Failures logs: https://treeherder.mozilla.org/logviewer.html#?job_id=199395257&repo=autoland&lineNumber=13466

https://treeherder.mozilla.org/logviewer.html#?job_id=199401863&repo=autoland&lineNumber=3589

Comment 30

[Dorel Luca [:dluca]]

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

Comment 31

[Sam Foster [:sfoster] (he/him)]

The first patch landed - that fixed the serialization and added a workaround for the xray/security wrapper issue in the test. The 2nd patch was backed out for multiple payment test failures, notably in browser_show_dialog.js, so this should remain open until that can re-land.

Comment 32

[Sam Foster [:sfoster] (he/him)]

try push with hopefully-fixed test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eea0d296c28a4434966a5a834650a99ed2b8458d

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 9008955 [details]
Bug 1429181 - Consider supportedNetworks when determining if payment method is valid. r?MattN

Matthew N. [:MattN] (PM me if requests are blocking you) has requested changes to the revision.

Comment 35

[Matthew N. [:MattN]]

Attachment: Bug 1429181 - Fix PaymentTestUtil.DialogContentTasks.select* to work with repeated calls. r=sfoster

Also fix test_basic_card_form.html and browser_show_dialog.js

Comment 36

[Matthew N. [:MattN]]

Attachment: Bug 1429181 - Fix test_payment_dialog_required_top_level_items.html with cc-type. r=sfoster

a) We have two names starting with the same prefix: "Mrs." which causes problem when typing on a <select> to filter, especially if the desired option is already selected.
b) The prefix of the payment method option would start with whitespace if cc-type is "".
c) There would be no state change for the shipping option picker when we're requesting to select the option that's already selected.

Depends on D6230

Comment 37

[Matthew N. [:MattN]]

Attachment: Bug 1429181 - Make test_payment_method_picker.html work with cc-type. r=sfoster

synthesizeKey requires the prefix of the label which would be whitespace without cc-type so add a cc-type

Depends on D6231

Comment 38

[Matthew N. [:MattN]]

Try push with my test fixes on top of Sam's try push from comment 32: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd9990d40e49ecf67538420f167327f117636aef

Comment 39

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9010142 [details]
Bug 1429181 - Fix PaymentTestUtil.DialogContentTasks.select* to work with repeated calls. r=sfoster

Sam Foster [:sfoster] has approved the revision.

Comment 40

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9010148 [details]
Bug 1429181 - Make test_payment_method_picker.html work with cc-type. r=sfoster

Sam Foster [:sfoster] has approved the revision.

Comment 41

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9010146 [details]
Bug 1429181 - Fix test_payment_dialog_required_top_level_items.html with cc-type. r=sfoster

Sam Foster [:sfoster] has approved the revision.

Comment 42

[Matthew N. [:MattN]]

Comment on attachment 9008955 [details]
Bug 1429181 - Consider supportedNetworks when determining if payment method is valid. r?MattN

Matthew N. [:MattN] (PM me if requests are blocking you) has approved the revision.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90201e199fea
Consider supportedNetworks when determining if payment method is valid. r=MattN
https://hg.mozilla.org/integration/autoland/rev/eec81649e2cf
Fix PaymentTestUtil.DialogContentTasks.select* to work with repeated calls. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/f410e48228a9
Fix test_payment_dialog_required_top_level_items.html with cc-type. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/1a8ec63c6456
Make test_payment_method_picker.html work with cc-type. r=sfoster

Comment 44

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/90201e199fea
https://hg.mozilla.org/mozilla-central/rev/eec81649e2cf
https://hg.mozilla.org/mozilla-central/rev/f410e48228a9
https://hg.mozilla.org/mozilla-central/rev/1a8ec63c6456

Comment 45

[Hani Yacoub, Desktop QA]

Going through the comments is making me not to be sure on what exactly should be verified here or maybe I've already verify it in other bugs.
I would really appreciate if you could help me with what should I verify here?
Thanks.

Comment 46

[Sam Foster [:sfoster] (he/him)]

(In reply to Hani Yacoub from comment #45)

Going through the comments is making me not to be sure on what exactly should be verified here or maybe I've already verify it in other bugs.
I would really appreciate if you could help me with what should I verify here?
Thanks.

I feel like this might have been verified already also, but if you want to check it off your list, I think the main thing to verify here is that a card from a network which is not supported by the merchant is flagged as invalid and payment is prevented. There's a test page at https://sfoster.github.io/payments-scratch/supportedNetworks.html which lets you generate a paymentrequest with supported networks.

(note an invalid network throws a DOM error and the paymentrequest.show() should fail)

Comment 47

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 66.0a1 (2019-01-27) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.14 and on Ubuntu 18.04 x64.