faker-ruby / faker

A library for generating fake data such as names, addresses, and phone numbers.
MIT License
11.23k stars 3.18k forks source link

French iban has invalid checksum #2588

Closed elkesrio closed 1 year ago

elkesrio commented 1 year ago

Describe the bug

Some french ibans have invalid checksums

To Reproduce

Using Faker version 2.15.1:

Faker::Bank.iban(country_code: "fr") # => "FR006769721996MP541QY956313"

This iban has an invalid checksum (https://www.ibancalculator.com/iban_validieren.html)

fbuys commented 1 year ago

Hey @elkesrio, I am looking at this link, to better understand the structure of a french iban. Do you know how the check digits for the iban is calculated?

elkesrio commented 1 year ago

Hey @fbuys, I have no idea. You can check ibandit gem for better understanding

fbuys commented 1 year ago

I'll do some more digging but is seems like this part defines how the check digit is calculated. Might be all we need to create the correct check digits - but not sure about that yet.

fbuys commented 1 year ago

@stefannibrasil I wonder if we can add the hactoberfest label here too?

thdaraujo commented 1 year ago

@stefannibrasil I wonder if we can add the hactoberfest label here too?

done, thanks for the help!

stefannibrasil commented 1 year ago

hi, sorry I missed this! Thanks for reporting this issue @elkesrio and @fbuys for the help!

I haven't had the time to reproduce this yet. I am not sure this is happening for all country codes or "fr" only.

It would be helpful to have a reproduction script for this issue. If anyone could do that before me, that would be great!

elkesrio commented 1 year ago

Hi @stefannibrasil, you can reproduce it using the ibandit gem

require 'ibandit'
while true do
  iban = Faker::Bank.iban(country_code: "fr")
  raise StandardError, iban unless Ibandit::IBAN.new(iban).valid?
end

# StandardError: FR007626128613KJOM8X38HG512
srcoley commented 1 year ago

The issue can occur with multiple country codes. In addition to the IBAN check digit, some countries also make use of a "national check digit". France is one such country that not only makes use of a national check digit, but also uses their own variation of ISO 7064 MOD-97-10. For more information, read here.

I believe I have this issue solved for France specifically - currently generating 100k valid French IBANs. While my solution is not yet generic enough to drop-in national check digits for other countries, I'd like to continue working on this so others can add them later more easily.

srcoley commented 1 year ago

I've opened a pull request to solve the issue of some generated IBANs not being valid according to ibandit. While this change would solve the issue with ibandit specifically it does not guarantee the IBANs are actually valid. ibandits validation is rudimentary and does not take into account the national check digit that some countries use. This means that a Faker generated IBAN might be valid according to ibandit, but invalid to more strict validators such as https://www.iban.com/iban-checker.

More detail on this in description of the pull request tagged above.

stefannibrasil commented 1 year ago

hi @srcoley thank you for opening a PR! What is the status of this issue? Are there next steps? Thank you!

srcoley commented 1 year ago

@stefannibrasil #2591 partially resolves the issue and has been merged into main.

What it solves

Invalid IBAN checksums

What it does not solve

Invalid national checksums

Next steps

Resolving invalid national checksums will take more effort because the portion of countries that implement them use varying algorithms for generation and validation. Check this table to see what I mean.

Additionally, ibandit(and no other library I could find) does not properly validate national checksums. The lack of an "authoritative" validator means several dozen would need to be written for Faker in order to test that the generators are working properly.

In regard to the web validator mentioned in the original issue(https://www.ibancalculator.com/iban_validieren.html), even if the national checksum issue is resolved, validation will likely still fail because it also checks that the bank and branch codes actually exist.

I'll leave the maintainers to decide what level of IBAN validity they think is acceptable for Faker's goals.

stefannibrasil commented 1 year ago

@srcoley Thank you so much for your detailed work and response! I haven't had the time to get more context about this until now. The details you provided are constructive.

I will say that fixing invalid IBAN checksums being generated is enough. Although we would like to support all cases and have higher accuracy, it would require too much effort to accommodate.

Adding to the docs that Faker only generates valid IBAN check digits but does not take national check digits used within the BBAN is enough.

If ibandit or other similar libraries can't guarantee to validate national checksums for all edge cases, and if this behavior is critical for an app, I recommend users generate their valid IBAN digits to guarantee their business logic relies on checks they own.

IMHO, the fix you added is enough, and adding variants for all edge cases is too much for Faker. I'd rather have something that serves basic needs well (without national check digits) than provide inaccurate information to users.

The next step to fix this issue would be to update the docs with this information.

@vbrazo @thdaraujo, what do you think?