Skip to content

Reject over-length codes in ABA, CUSIP, ISBN-10 and EAN-13 check digits#427

Merged
garydgregory merged 1 commit into
apache:masterfrom
sahvx655-wq:checkdigit-reject-over-length
Jul 20, 2026
Merged

Reject over-length codes in ABA, CUSIP, ISBN-10 and EAN-13 check digits#427
garydgregory merged 1 commit into
apache:masterfrom
sahvx655-wq:checkdigit-reject-over-length

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

Following up on #426, I went back through the rest of the check digit package looking for the same length hole. ModulusCheckDigit.isValid never checks the code length, and calculateModulus weights each character by rightPos = length minus index, which does not move when a character is prepended. So the routines that weight purely on rightPos accept an over-length code whenever the prepended character lands on a no-op weight: ABANumberCheckDigit validates 0123456780, CUSIPCheckDigit validates 0037833100, and EAN13CheckDigit validates a genuine thirteen digit code carrying a leading zero. ISBN10CheckDigit is worse because it is modulus 11, so the leading position weight is a multiple of the modulus and any prepended digit is swallowed, for example 51930110995. Each of these singletons is a public entry point reachable without its wrapper validator, so a caller running a code straight through the routine treats the padded string as genuine.

The guard sits in each routine's isValid and rejects a code whose length is not the fixed length before the check digit test, the same shape merged for ISSNCheckDigit and the length guard already present in IBANCheckDigit. Keeping it in the routine covers the direct callers, not just the wrapper paths that already enforce length through CodeValidator. I deliberately left ISINCheckDigit alone since it is length agnostic by design (the 3133EHHF3 fixture from VALIDATOR-422 is a valid check digit on a non ISIN length), and SedolCheckDigit already caps its length; CAS, EC, Verhoeff and Luhn are either regex length checked or variable length and so are not affected. Each routine gets a regression test that fails before the guard and passes with it.

@raboof

raboof commented Jul 20, 2026

Copy link
Copy Markdown
Member

Seems reasonable to me. I wonder if this should even be an (optional) feature of ModulusCheckDigit, as I suspect it's common for patterns that use a modulus check to have a constant length

@garydgregory garydgregory changed the title reject over-length codes in ABA, CUSIP, ISBN-10 and EAN-13 check digits Reject over-length codes in ABA, CUSIP, ISBN-10 and EAN-13 check digits Jul 20, 2026
@garydgregory
garydgregory merged commit 0632d5e into apache:master Jul 20, 2026
19 checks passed
@garydgregory

Copy link
Copy Markdown
Member

It could be optional but it seems it would only apply to 5 of the 11 subclasses.

@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

That matches what I found when I surveyed the package: only the five fixed-length routines would use a base-class guard (these four plus ISSN), while ISIN is length agnostic by design, Sedol already caps its length, and the rest are regex checked or variable length, which is why I kept it per routine. Thanks for merging this.

@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

Gary's count matches what I found when I swept the package: the fixed-length routines come out at five, these four plus ISSN from #426. ISIN is deliberately length agnostic (the 3133EHHF3 fixture from VALIDATOR-422 is a valid check digit on a non ISIN length), Sedol's calculateModulus already throws on anything over seven characters, CAS and EC are length checked by their regexes, and Luhn and Verhoeff are genuinely variable length. An optional expected-length field on ModulusCheckDigit (constructor argument, unset meaning no check) would fold the five overrides into one guard and give any future fixed-length routine the check for free, at the cost of a wider constructor surface for the six that don't want it. I can put that refactor together as a follow-up if that's the preferred shape.

Thanks for merging this one.

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.

3 participants