search

konfirm / node-iso13616

ISO 13616-1:2007 - International Bank Account Number
MIT License
3 stars 0 forks source link

Checksum 02 doesn't pass validation (along with checks 00, 01) #1

Closed joallard closed 3 years ago

joallard commented 3 years ago

Hello @rspieker, thanks for this clean library!

I am researching an eventual IBAN for Canada, and I was playing around with the following string:

ISO13616.generate("0131-31678-000006512388", "CA", true)
// "CA99 0131 3167 8000 0065 1238 8"

Validating the low checksum will however return false:

ISO13616.validate("CA02 0131 3167 8000 0065 1238 8")
// false

I'm interested to hear your comments here, but according to the info I have, the low range should be allowed under mod97. From Wikipedia:IBAN:

The ECBS document replicates part of the ISO/IEC 7064:2003 standard as a method for generating check digits in the range 02 to 98. Check digits in the ranges 00 to 96, 01 to 97, and 03 to 99 will also provide validation of an IBAN, but the standard is silent as to whether or not these ranges may be used.

It further says: (I haven't followed this recommendation comes from where)

  1. Subtract the remainder from 98, and use the result for the two check digits.

Meaning this would yield the 02 to 98 range.

Here's a table of what the library seems to accept and what not:

XX00-... mod 97 98 - last Library result
GB97 WEST 1234 5698 7600 21 1 97 true
GB00 WEST 1234 5698 7600 21 1 97 false
GB98 WEST 1234 5698 7600 03 0 98 true
GB01 WEST 1234 5698 7600 03 0 98 false
GB99 WEST 1234 5698 7600 82 96 2 true
GB02 WEST 1234 5698 7600 82 96 2 false

For testing purposes, integers 6597/6500, 9798/9701, 3202/3299 can be used (they are all mod 97 = 1). This last one equals integer 32142829123456_98760082_1611_02.

It seems to me:

rspieker commented 3 years ago

Hello @joallard thanks for the detailed report!

I've started looking into this, and it's actually a very good question on which I don't have an immediate answer. Judging from comments I've left in the code, at the time of writing I knew about this

// there are three edge cases which pass validation with two different checksums
// 97/00, 98/01, 99/02 would all pass
// we never provided the 00, 01 nor 02 value and always provide the higher values

That leads into the question; was that an assumption or is there something in the specification that lead to this decision?

A quick review of the ISO documentation I have followed (ISO 13616-1:2005(DIS)) gave me the following:

6,2,6 Check digits with the value of ‘01’ or ‘’00’ are invalid. To resolve an anomaly in the algorithm, values ‘01’ and ‘00’ are equivalent to ‘98’ and ‘99’, respectively, and the latter must be used.

I can't legally link the document, though I did manage to find a copy of that same specification file by searching for: "6,2,6" iso13616 (that paragraph actually stands out in the document as it's the only one using comma's to separate the digits)

I've noticed there's a new revision of the specification from just last month (2020-09), so I'll obtain that and see what it states now.

As it stands it appears to be correctly treating the 00 and 01, though I will follow up on this.

rspieker commented 3 years ago

The ISO 13616-1:2020(E) document is actually more explicit and has a proper note on the character range

6.3.3 Apply the check character system MOD 97-10 (see ISO/IEC 7064). NOTE From this, the check digits can only be in the range [02..98].

So in summary, you are correct that 02 is valid, though 99 will not be. I'll be updating the code to reflect this and somehow mention something about it in the documentation.

rspieker commented 3 years ago

@joallard thank you for putting in the effort of filing an excellent issue!

Regarding the expected outcome

The library should accept all of these

This would actually contradict the ISO13616-1:2020 (and prior) specification, which (as of the 2020 version) states

the check digits can only be in the range [02..98]

It might be beneficial to create a method/option that will actually allow for a correction in case of 00, 01 and 99 checksum, though according to the ISO specification that would not be correct.

I've looked into the ECBS documentation, which actually does not refer to the ISO-7064:Mod97-10 implementation but to ISO13616-1 as standard to follow. As ISO7064 would actually allow for the equivalent checksums (00 / 97, 01 / 98 and 02 / 99), I feel like the ISO13616-1 specification should be followed, and therefor rule out 00, 01 and 99 as being valid.

It should generate by default checks 02 rather than checks 99.

Yes, this was an oversight on my part, where I mistakenly added 02 to be "rotated" the same as 00 and 01. This has now been corrected.

As the library previously would incorrectly treat 02 as invalid and 99 as valid, I've released a new major version (2.0.0) as the fix changed the behavior.

iban checksum valid behavior changed
GB00 WEST 1234 5698 7600 21 00 false no
GB97 WEST 1234 5698 7600 21 97 true no
GB01 WEST 1234 5698 7600 03 01 false no
GB98 WEST 1234 5698 7600 03 98 true no
GB02 WEST 1234 5698 7600 82 02 true yes (was false)
GB99 WEST 1234 5698 7600 82 99 false yes (was true)

Please update to the latest (2.0.0) version of the package

$ npm install --save @konfirm/iso13616@latest

Again, thanks for taking the time to report the issue!

joallard commented 3 years ago

A pleasure to read your thorough response, excellent dive! I wasn't able to access the ISO standards, so it is very informative.

Following ISO13616-1:2005 and forward seems appropriate. Seems like en.Wikipedia is a tad inaccurate then. On the off-chance that :1997 allowed checks 00 and 01, I would leave it to the user to fork the library if they so need.

I've noticed there's a new revision of the specification from just last month (2020-09), so I'll obtain that and see what it states now.

Whew, fresh off the press!

Range 02..98 seemed to reflect general usage (anecdotally, I've tried a few online validators, most would find -00 and -01 invalid, though not all.) as well as match the 98-minus algorithm. As the issue title suggests, -02 was the important case in my opinion.

Once again, thanks for this very thorough research. And for the very quick fix!

Aside: You seem to have a very good commit game, very atomic! If you're interested in some advice: I find it really useful to precede the commit title by the context (module, module-method, or file name). Other tip, when possible, I distill the whole commit in a few words, such as the diff could be reconstructed from the message only. (something something entropy)

Some examples:

and so on.

(Coming from Ruby, I use the notation Module#method / Module.static_method, but that could very well be something else like Class.{p,proto,prototype}.method)

Feel free to leave it, but I sense you'd be the kind who could put it to good use or at least derive some inspiration out of it.

Cheers!