fix: correct broken ISIN checksum (validator accepted any check digit)#464
Open
DucMinhNe wants to merge 1 commit into
Open
fix: correct broken ISIN checksum (validator accepted any check digit)#464DucMinhNe wants to merge 1 commit into
DucMinhNe wants to merge 1 commit into
Conversation
_isin_checksum computed a per-character val in the loop but never added it into check, which stayed 0. So (check % 10) == 0 was always True and isin() accepted any 12-character string with valid characters regardless of its check digit. The existing invalid-ISIN tests all tripped the earlier length or character guards, so the checksum branch was never actually exercised. Reimplement the ISO 6166 algorithm: require a numeric check digit, expand each letter to its two-digit value (A=10..Z=35), then run a right-to-left Luhn sum over the expanded digits. Validated against python-stdnum's isin.is_valid over 100k+ country-code-valid inputs with zero mismatches. Also dropped JP000K0VF054 from the valid samples (it is not a valid ISIN; it only passed because the checksum did nothing) and added wrong-check-digit cases to the invalid set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_isin_checksumnever actually validates the check digit. It computes a per-charactervalinside the loop but never adds it tocheck:So
(check % 10) == 0is alwaysTrueandisin()accepts any 12-character string with valid characters, regardless of the check digit:The existing "invalid" test cases (
010378331005,XCVF,00^^^1234,A000009) are all rejected earlier by the length / character /idx > 1guards, so the checksum branch was effectively untested and the bug stayed hidden.Fix
Implement the actual ISO 6166 algorithm: the check digit must be numeric, expand each letter to its two-digit value (A=10 .. Z=35), then run a right-to-left Luhn sum over the expanded digit string.
I validated the new implementation against
python-stdnum's authoritativeisin.is_validover 100k+ country-code-valid random inputs plus random fuzzing: zero mismatches.Tests
US0378331006,US0004026251,GB0002634947) to the invalid set. These fail onmasterand pass with the fix.JP000K0VF054in the valid set: it is not a valid ISIN (its check digit is wrong), it only passed before because the checksum did nothing. Substituted real valid ISINs (GB0002634946,DE000BAY0017).pytest tests/test_finance.pyis green (25 passed);ruff checkandruff format --checkclean.