Skip to content
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

Open
import-brain opened this issue Aug 27, 2023 · 18 comments
Open
Labels
c: locale Permutes locale definitions c: test m: finance Something is referring to the finance module p: 1-normal Nothing urgent
Milestone

Comments

@import-brain
Copy link
Member

import-brain commented Aug 27, 2023

We're currently not sure if all of our credit card issuer patterns are valid patterns. We should check them with validator.js.

@import-brain import-brain added s: on hold Blocked by something or frozen to avoid conflicts p: 1-normal Nothing urgent c: locale Permutes locale definitions m: finance Something is referring to the finance module labels Aug 27, 2023
@import-brain import-brain added this to the v9 - Next major milestone Aug 27, 2023
@HonzaMac
Copy link
Contributor

Do you mean to check them in tests here, or in runtime as new dependency in faker?

@ST-DDT
Copy link
Member

ST-DDT commented Aug 28, 2023

In tests here.

@ST-DDT ST-DDT added the c: test label Aug 28, 2023
@xDivisionByZerox
Copy link
Member

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 good first issue.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 6, 2023

It might also be fixing patterns. Or making sure the validatorjs' validation patterns for the credit cards are valid.

@ST-DDT ST-DDT removed the s: on hold Blocked by something or frozen to avoid conflicts label Sep 6, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Sep 6, 2023

Blocked by #2344

Merged/Fixed

@matthewmayer
Copy link
Contributor

matthewmayer commented Sep 7, 2023

I ran a large number of generated numbers through validator.isCreditCard. First the general check (no provider)

{
  mastercard: { valid: 91151, invalid: 0 },
  jcb: { valid: 90173, invalid: 0 },
  american_express: { valid: 90996, invalid: 0 },
  diners_club: { valid: 91178, invalid: 0 },
  discover: { valid: 30566, invalid: 61030 },
  visa: { valid: 90643, invalid: 0 },
  maestro: { valid: 16220, invalid: 74712 },
}

discover and maestro cards have problems, rest are OK.

Additionally when running validator.isCreditCard(ccnumber, {provider: ...}) some diners_club and mastercard cards fail. maestro is not a supported provider by validator.

@matthewmayer
Copy link
Contributor

matthewmayer commented Sep 7, 2023

There are six patterns for discover in Faker.

These two succeed:

65##-####-####-###L
6011-####-####-###L

These four fail:

A) 6011-62##-####-####-###L
B) 64[4-9]#-62##-####-####-###L
C) 65##-62##-####-####-###L
D) 64[4-9]#-####-####-###L

For reference the validatorjs regex is /^6(?:011|5[0-9][0-9])[0-9]{12,15}$/

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.

@matthewmayer
Copy link
Contributor

In order to properly support maestro, this would need to be added to validator.

According to Wikipedia Maestro cards are 12-19 digits long and start with one of these prefixes:
6759, 676770, 676774, 5018, 5020, 5038, 5893, 6304, 6759, 6761, 6762, 6763

@matthewmayer
Copy link
Contributor

mastercard has two patterns in Faker. Both pass the general isCreditCard check but
`5[1-5]##-####-####-###L' succeeds but '6771-89##-####-###L' fails.

The validator regex is /^5[1-5][0-9]{2}|(222[1-9]|22[3-9][0-9]|2[3-6][0-9]{2}|27[01][0-9]|2720)[0-9]{12}$/

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/

@matthewmayer
Copy link
Contributor

Probably this needs

  • a few small PRs to fix the patterns
  • then some upstream changes to validator
  • then update the tests to verify the all pass the validator checks

@ST-DDT
Copy link
Member

ST-DDT commented Sep 9, 2023

I think you are right. Thanks for the very detailed analysis so far ❤️ .

@matthewmayer
Copy link
Contributor

There's an open PR for adding maestro support here

validatorjs/validator.js#2223

@WikiRik
Copy link

WikiRik commented Sep 9, 2023

@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

@matthewmayer
Copy link
Contributor

matthewmayer commented Sep 9, 2023

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

  • it has output defunct patterns for years and no one has complained
  • very few applications should ever be directly handling credit card numbers or storing them in a database. Typically it's always handled by a third party payment processor.

So while this is a nice to fix I don't think it's impacting many users of Faker.

@matthewmayer
Copy link
Contributor

Created a new PR at validator for improved Maestro support. validatorjs/validator.js#2286

@matthewmayer
Copy link
Contributor

Changed my mind... as Maestro is being discontinued
https://www.mastercard.com/news/europe/en/perspectives/en/2021/blog-from-valerie-nowak-why-this-maestro-is-retiring-after-30-years/

Built primarily for a physical world, Maestro cards cannot consistently be used for e-commerce payments, in part because the numbering convention on Maestro cards (up to 19 digits) is not compatible with widely used e-commerce portals.

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

@xDivisionByZerox
Copy link
Member

I was thinking of ´taking on this issue, but immediatly got stuck as I was not sure where these tests would be located at.

  1. As a new describe block in test/modules/finance.spec.ts
  2. As a new case in test/all-functional.spec.ts
  3. As a new block in test/locale-data.spec.ts
  4. Somewhere else?

I'm kinda lost and would appreciated some feedback.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 29, 2024

IMO here:

  1. As a new describe block in test/modules/finance.spec.ts

@ST-DDT ST-DDT modified the milestones: v9.0, vAnytime Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions c: test m: finance Something is referring to the finance module p: 1-normal Nothing urgent
Projects
None yet
Development

No branches or pull requests

6 participants