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

Fix national checksum calculation for Belgium #119

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

Olympic1
Copy link
Contributor

The function never validated the national checksum for Belgium correctly. What it did was:

  • Get the checksum from the IBAN
  • Remove the bankID from the BBAN to get the account
  • Get the checksum from the account
  • Calculate the checksum with mod 97
  • Check if the calculated checksum is the same as the checksum from the IBAN (Which will always be true)

I changed it so that it now calculates it following the correct rules.

  • Get the checksum from the IBAN
  • Get the BBAN from the IBAN
  • Remove the checksum from the BBAN
  • Calculate the checksum from the left over BBAN with mod 97
  • Check if the calculated checksum is the same as the checksum from the IBAN

Here is a script with the current and new code
https://onlinephp.io/c/60fa7

The function never validated the checksum correctly.
@globalcitizen
Copy link
Owner

Great catch! What a horrible bug. Thanks for a well documented pull request. The tests currently only verify that a set of example pass, they cannot catch logic errors where the verification code always returns true. I will run a round of further tests on the suite of examples just to verify they all work and modifications fail, then push this out.

@globalcitizen globalcitizen merged commit d474287 into globalcitizen:master Aug 10, 2022
@globalcitizen
Copy link
Owner

globalcitizen commented Aug 10, 2022

Released as v4.1.1. Thanks again!

In case you are interested, we could probably do with:

  • A check of all the other pre-IBAN national checksum algorithms to ensure no similar bugs exist for those implementations.
  • Some new implementations documented over here

@Olympic1
Copy link
Contributor Author

Ok, I'll give it a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants