fix: compute ISIN checksum instead of always accepting - #474
Conversation
_isin_checksum never accumulated into check, so any 12-character ISIN-shaped string validated as True. Expand letters and apply Luhn.
eeshsaxena
left a comment
There was a problem hiding this comment.
Heads up that #473 is the same fix, opened ten days earlier and referencing the issue (#440). Whichever one lands, the two differ in ways worth deciding deliberately rather than by merge order.
The fixture removal here needs a note. JP000K0VF054 is not a valid ISIN: expanded it is 19250002003115054, and the Luhn sum comes to 29, so it fails the check digit. It only sat in the valid list because the old _isin_checksum never accumulated into check and so returned True for anything with the right character classes. Dropping it silently reads like the new implementation could not handle it. #473 keeps the case and repairs it to JP000K0VF055 (sum 30), then adds the original to the invalid list, which documents the old bug rather than hiding it.
The other difference is lowercase. This PR keeps the a-z branch, so isin('us0378331005') stays True; #473 removes it, so lowercase now fails. Keeping it matches _cusip_checksum, which also accepts lowercase, so I think this PR is on the right side of that one, but it is a behaviour choice that deserves a line in the description either way.
Minor: isin() already does len(value) == 12 and _isin_checksum(value), so the len(value) != 12 guard here is unreachable. The value[-1].isdigit() half is doing real work though, and str.isdigit() is true for characters like ² and Arabic-Indic digits, which then fall through to the else: return False in the loop. Fine as it stands, just not for the reason it looks like.
I checked both implementations against US0378331005, US0004026250, US5949181045, GB0002634946, AU0000XVGZA3, IE00B4BNMY34, CA0679011084, US30303M1027, DE0005557508, FR0000120271, NL0000009165, CH0012032048 plus a few deliberately broken check digits, and they agree with a reference Luhn everywhere except the lowercase case.
validators.finance.isin hits false-accept when isin checksum never accumulates; any 12-char alnum ISIN-shaped string returns True. This PR fixes the regression with a focused test covering the case.