Credit card network validation on entry and summary pages

VERIFIED FIXED in Firefox 64

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: MattN, Assigned: sfoster)

Tracking

(Blocks 1 bug)

Trunk
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed, firefox65 fixed, firefox66 verified)

Details

(Whiteboard: [webpayments] [user-testing])

Attachments

(6 attachments, 3 obsolete attachments)

5.54 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-phabricator-request
MattN
: review+
Details | Review
46 bytes, text/x-phabricator-request
MattN
: review+
Details | Review
46 bytes, text/x-phabricator-request
sfoster
: review+
Details | Review
46 bytes, text/x-phabricator-request
sfoster
: review+
Details | Review
46 bytes, text/x-phabricator-request
sfoster
: review+
Details | Review
Reporter

Description

2 years ago
partly-obsolete
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.
Priority: P3 → P2
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Whiteboard: [webpayments] → [webpayments] [user-testing]
Flags: qe-verify+
QA Contact: hani.yacoub
Assignee

Comment 1

11 months ago
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?
Flags: needinfo?(MattN+bmo)

Updated

11 months ago
Priority: P2 → P3
Whiteboard: [webpayments] [user-testing] → [webpayments-reserve] [user-testing]
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.
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.
Flags: needinfo?(MattN+bmo)
See Also: → 1477105
Summary: Credit card type detection and validation upon input → Credit card network validation on entry and summary pages
Priority: P3 → P2

Updated

10 months ago
Whiteboard: [webpayments-reserve] [user-testing] → [webpayments] [user-testing]
Assignee

Updated

10 months ago
Assignee: nobody → sfoster

Updated

10 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1
Duplicate of this bug: 1429198
Assignee

Comment 7

10 months ago
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.
Assignee

Comment 8

10 months ago
Posted patch supportedNetworks-test.diff (obsolete) — Splinter Review
Assignee

Comment 9

10 months ago
Posted patch supportedNetworks-test.diff (obsolete) — Splinter Review
run mach test browser/components/payments/test/browser/browser_request_serialization.js to reproduce
Attachment #9006316 - Attachment is obsolete: true
Assignee

Comment 10

10 months ago
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?
Flags: needinfo?(mcaceres)
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.
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.
Flags: needinfo?(mcaceres)
Flags: needinfo?(echuang)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Assignee

Comment 14

10 months ago
(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?
(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.
Assignee

Comment 16

10 months ago
Updating the patch which should yield failure in the test_serializeRequest_paymentMethods test, and the security wrapper warning.
Attachment #9006318 - Attachment is obsolete: true
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.
Flags: needinfo?(bobbyholley)
(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).
Flags: needinfo?(bobbyholley)
(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.
Blocks: 1477102

Updated

10 months ago
Whiteboard: [webpayments] [user-testing] → [webpayments] [user-testing][ux]
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]
Whiteboard: [webpayments] [user-testing][ux] → [webpayments] [user-testing]
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.
Depends on: 1490868
Flags: needinfo?(echuang)
Assignee

Comment 22

9 months ago
* 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
Attachment #9008947 - Attachment is obsolete: true
Assignee

Comment 23

9 months ago
* 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
Assignee

Comment 24

9 months ago
* 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 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.
Attachment #9008955 - Flags: review+
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.
Attachment #9008953 - Flags: review+

Comment 27

9 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1a4703b8464
Fix serialization of payment method data. r=MattN

Comment 28

9 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7351a4c8d1a1
Consider supportedNetworks when determining if payment method is valid. r=MattN

Comment 30

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1a4703b8464
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee

Comment 31

9 months ago
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(sfoster)
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Assignee

Updated

9 months ago
Duplicate of this bug: 1490807
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.
Attachment #9008955 - Flags: review+
Also fix test_basic_card_form.html and browser_show_dialog.js
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
synthesizeKey requires the prefix of the label which would be whitespace without cc-type so add a cc-type

Depends on D6231
Assignee

Comment 39

9 months ago
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.
Attachment #9010142 - Flags: review+
Assignee

Comment 40

9 months ago
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.
Attachment #9010148 - Flags: review+
Assignee

Comment 41

9 months ago
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.
Attachment #9010146 - Flags: review+
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.
Attachment #9008955 - Flags: review+

Comment 43

9 months ago
Pushed by sfoster@mozilla.com:
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
Assignee

Updated

9 months ago
Blocks: 1473662

Comment 45

5 months ago

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.

Flags: needinfo?(sfoster)
Assignee

Comment 46

5 months ago

(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)

Flags: needinfo?(sfoster)

Comment 47

5 months ago

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.