Fix scorer crash on characters whose downcased form is longer - #124
Merged
Conversation
Passwords containing "İ" (U+0130) crashed with NoMethodError: undefined method '<<' for nil in Scorer#most_guessable_match_sequence. String#downcase expands "İ" to "i" + combining dot above (U+0069 U+0307), so the lowercased password the dictionary and l33t matchers search is longer than the original. Match indices found after the expansion point are shifted past the end of the password, and the scorer indexes matches_by_j out of range. Add CaseHelpers.downcase_preserving_length, which falls back to per-character downcasing (truncating multi-character mappings to their first character) whenever plain downcasing changes the string length, and use it in the dictionary matcher, the l33t matcher, and DictionaryRanker. Ranking words with the same normalisation also lets user-input words containing such characters match at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sZ2Mphi8RqjYqqVfwULRj
most_guessable_match_sequence indexes matches into an array sized to the password length, so it relies on every match's j (and i) being an in-range position in the original password. That invariant lived only on CaseHelpers.downcase_preserving_length, not where a maintainer would break it. Expand the comment at the indexing site to state the precondition, name the failure mode (NoMethodError on the nil slot — the crash the length- preserving downcase fixes), and point to the helper that upholds it. Comment-only change; no behavioural impact.
The İ regression test lived only inside the trie context, so it exercised just trie_matches — the path whose out-of-range j crashed the scorer. The hash path had a different, silent pre-fix bug: bounded by the original password length, it dropped the trailing words entirely rather than crashing. Nothing tested that path, so a regression there would pass CI. Replace the trie-only test with a context that runs both paths and: - bounds every i/j on both paths, - pins the concrete matched words and their original-password indices (was a bare `j < length` check that ignored i and the token), and - asserts the trie and hash paths produce identical results. All three fail against the pre-fix code; the equality assertion is what guards the hash path's silent under-matching.
rank_dictionary (the method the fix changed) had no direct spec — only
rank_dictionaries was tested, with ASCII input — so the ranker's half of
the "ranker and matcher must agree on the normalized key" contract was
covered only transitively via the tester_spec İstanbul case.
Add a .rank_dictionary block: a baseline frequency-order test plus a
length-changing word (İstanbul) asserting the key is the length-preserving
lowercase form ('istanbul') and that the key's character length matches the
original. Both İstanbul assertions fail against the pre-fix ranker (which
keyed under 'i̇stanbul', length 9).
Follows the project convention of referencing the PR with a ([#NN]) link and a matching link definition in the version section.
Member
|
Thank you @kugaevsky! 🙇 |
Merged
Member
|
Released in 2.0.1 |
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
2 participants
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.
Passwords containing "İ" (U+0130) crashed with
NoMethodError: undefined method '<<' for nil in
Scorer#most_guessable_match_sequence. String#downcase expands "İ" to "i" + combining dot above (U+0069 U+0307), so the lowercased password the dictionary and l33t matchers search is longer than the original. Match indices found after the expansion point are shifted past the end of the password, and the scorer indexes matches_by_j out of range.
Add CaseHelpers.downcase_preserving_length, which falls back to per-character downcasing (truncating multi-character mappings to their first character) whenever plain downcasing changes the string length, and use it in the dictionary matcher, the l33t matcher, and DictionaryRanker. Ranking words with the same normalisation also lets user-input words containing such characters match at all.