-
-
Notifications
You must be signed in to change notification settings - Fork 929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verify all credit card issuer patterns are valid with validator.js #2350
Comments
Do you mean to check them in tests here, or in runtime as new dependency in faker? |
In tests here. |
This issue only requires the implementation of the tests or fixing possible invalid patterns as well? If only the first one, this might be a |
It might also be fixing patterns. Or making sure the validatorjs' validation patterns for the credit cards are valid. |
Merged/Fixed |
I ran a large number of generated numbers through
Additionally when running |
There are six patterns for These two succeed:
These four fail:
For reference the validatorjs regex is Referring to https://web.archive.org/web/20170822221741/https://www.discovernetwork.com/downloads/IPP_VAR_Compliance.pdf it would seem to be that patterns A, B, C are incorrect on Faker's end. They generate 20 digit CC numbers which are invalid as the max length is 19. Pattern D is incorrect on validator's end, as Discover cards may start with 64 and be 16 digits. |
In order to properly support According to Wikipedia Maestro cards are 12-19 digits long and start with one of these prefixes: |
The validator regex is This seems to be a weird edge case for card numbers starting 677189 which is actually detected as a unionpay card by validator! Would suggest to remove this pattern (which appears in a full 50% of generated mastercard numbers and replace with a more common prefix, like the new "2" prefixed cards https://www.cardfellow.com/blog/new-mastercard-bins/ |
Probably this needs
|
I think you are right. Thanks for the very detailed analysis so far ❤️ . |
There's an open PR for adding maestro support here |
@matthewmayer just as a headsup; merging PRs in validator happens infrequently and at random moments. So it would be best not to rely on outdated PRs but try to make all upstream changes to validator in a single PR |
No big hurry 😀 when a project already has 150 open PRs I probably won't help by adding yet another one. For the time being I pinged the original author of the maestro PR to see if they want to take it on. Frankly I'm not convinced the credit card method is very frequently used given
So while this is a nice to fix I don't think it's impacting many users of Faker. |
Created a new PR at validator for improved Maestro support. validatorjs/validator.js#2286 |
Changed my mind... as Maestro is being discontinued
The existing patterns in Faker don't match what Mastercard publish as the BIN ranges at https://developer.mastercard.com/bin-lookup/documentation/support/ And validator doesn't currently support it. I think it would be best to just remove Maestro completely similar to #2356 |
I was thinking of ´taking on this issue, but immediatly got stuck as I was not sure where these tests would be located at.
I'm kinda lost and would appreciated some feedback. |
IMO here:
|
We're currently not sure if all of our credit card issuer patterns are valid patterns. We should check them with validator.js.
The text was updated successfully, but these errors were encountered: