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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(isIBAN): only strip spaces and hyphens before validating IBAN #1268

Merged
merged 1 commit into from Mar 15, 2020

Conversation

GoMino
Copy link
Contributor

@GoMino GoMino commented Mar 11, 2020

It turns out the IBAN validation is not working correctly as commented here #951 (comment)

The following function in isIBAN.js seems buggy:

function hasValidIbanFormat(str) {
  // Strip white spaces and hyphens, keep only digits and A-Z latin alphabetic
  const strippedStr = str.replace(/[^A-Z0-9]+/gi, '').toUpperCase();
  const isoCountryCode = strippedStr.slice(0, 2).toUpperCase();

  return (isoCountryCode in ibanRegexThroughCountryCode) &&
      ibanRegexThroughCountryCode[isoCountryCode].test(strippedStr);
}

We should not keep only digits and A-Z latin alphabetic, because any special char will be stripped away as well. We only want to strip spaces and hyphens.

Indeed we want the following IBANs to be INVALID:

  • FR7630006000011234567890189@
  • 'FR7630006000011234567890189馃槄
  • FR763000600001123456!!馃え7890189@

This PR fix the issue

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @GoMino for your contribution! Could you please clean up your diff, so that it just remains with only changes related isIBAN?

@profnandaa profnandaa added the 馃Ч needs-update For PRs that need to be updated before landing label Mar 12, 2020
@GoMino
Copy link
Contributor Author

GoMino commented Mar 12, 2020

@profnandaa I have just updated my pull request.

It wasn't clear to me why the build artefacts was not added to .gitignore (#1143 (comment)).

I would really appreciate if you could publish a new version of the library once this pull request get merged as we are using this library in one of our project right now.

Thanks for you great work by the way!

@profnandaa profnandaa merged commit 04769b5 into validatorjs:master Mar 15, 2020
@GoMino
Copy link
Contributor Author

GoMino commented Mar 15, 2020

Thanks for merging!

could you tell me when you plan to publish a new release ?

@GoMino
Copy link
Contributor Author

GoMino commented Mar 19, 2020

@profnandaa, @ezkemboi is it possible to publish a new version of the library, which will include those changes ?

@profnandaa
Copy link
Member

@GoMino -- sure, let me check with @chriso; we should be having enough stuff for release now.

@chriso
Copy link
Collaborator

chriso commented Mar 20, 2020

@profnandaa @GoMino done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃Ч needs-update For PRs that need to be updated before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants