Repository navigation
fix(tinyredact): reject an IBAN candidate with non-ASCII digits instead of panicking - #315
shaurya703 wants to merge 1 commit into
Conversation
…ad of panicking IBAN_RE matches with Unicode \d, so a run such as "DE४४1234567890123456" (Devanagari check digits, which normalization does not fold) reaches valid_iban. It sliced the check digits as cleaned[2..4] by byte offset, which splits the three-byte digit and panics. redact_pii and has_likely_pii both run the IBAN rule, so any text of that shape crashed the redactor. An IBAN is ASCII by definition: reject any candidate that is not ASCII alphanumeric after whitespace removal, before the slicing.
Tiny Sweeper reviewReviewed head: Verdict: Ready for maintainer review · Priority: none Diagnostics and supporting detailsFull summaryPull request purpose (title): fix(tinyredact): reject an IBAN candidate with non-ASCII digits instead of panicking. `valid_iban` in tinyredact could panic on an IBAN candidate whose check digits come from a non-Latin script: `IBAN_RE` uses Unicode `\d`, normalization does not fold Devanagari/Bengali digits, and `valid_iban` sliced `cleaned[2..4]` by byte offset, splitting a multi-byte digit. This pull request adds an ASCII-alphanumeric guard before length checks and slicing so such candidates are rejected and the text passes through unchanged, with new tests at the validator level and on the public redaction/boundary paths. Review snapshot
Completeness: Complete What changed`valid_iban` in `crates/tinyredact/src/pii/checks.rs` now returns false for any candidate containing a character that is not ASCII alphanumeric after whitespace removal, placed before the length check and the byte-offset slicing that previously assumed ASCII. A comment documents the Unicode-`\d` mismatch in `IBAN_RE`. Two tests are added: `iban_rejects_non_ascii_digits_without_panicking` in `crates/tinyredact/src/pii/checks_tests.rs` calls the validator directly with Devanagari check digits (leading and mid-string), and `iban_with_non_ascii_digits_does_not_panic` in `crates/tinyredact/src/pii/mod_tests.rs` asserts the text is left unchanged and `has_likely_pii` is false. Valid IBANs are unaffected. Features
Tests
FindingsNo active actionable findings. Before mergeNone. Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
ASCII validation and regression coverage crates/tinyredact/src/pii/checks.rs, crates/tinyredact/src/pii/checks_tests.rs, crates/tinyredact/src/pii/mod_tests.rs |
valid_iban rejects non-ASCII-alphanumeric candidates before length checks and byte-offset slicing. Tests check rejection of Devanagari digits, unchanged input, and a false result from has_likely_pii. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Low
Suggested reviewers: senamakel
Merge Risk: 
70e22
-
No actionable merge-blocking risk remains.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly states the main change: reject IBAN candidates with non-ASCII digits to prevent a panic. It is specific, concise, and directly matches the implementation and regression tests. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the IBAN line,
And keeps each byte within its sign.
No split digit can cause a fall,
The tests now guard the path for all.
The rabbit hops, pleased with the call.
Comment @coderabbitai help to get the list of available commands.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
tinyredact's IBAN validator panics on a candidate whose check digits come from a non-Latin script, which crashesredact_pii,has_likely_piiand the default sanitize policy on ordinary user text.Bug.
IBAN_RE([A-Z]{2}\d{2}(?:[\s]?[A-Z0-9]){11,30}) uses Unicode\d, so it matches Devanagari, Bengali and other decimal digits. Normalization only folds fullwidth and Arabic-Indic digits, so these reachvalid_ibanunchanged. Reproduction on currentmain:Root cause.
valid_ibanchecks the length in bytes and slicescleaned[2..4]by byte offset, assuming the candidate is ASCII. A multi-byte digit at position 2 makes index 4 fall inside a character.Fix. An IBAN is ASCII by definition, so
valid_ibannow rejects any candidate that is not ASCII alphanumeric after whitespace removal, before the slicing. Valid IBANs are unaffected.Related issue
None. Found while reading the PII validators.
API or behavior changes
None for valid input. Text that used to panic now passes through unchanged.
Validation
cargo fmt --all -- --check: cleancargo clippy -p tinyredact --all-targets --all-features -- -D warnings: cleancargo test -p tinyredact: 211 passed (the two new tests panic without the fix)Tests
pii::checks::tests::iban_rejects_non_ascii_digits_without_panicking: calls the validator directly with Devanagari check digits.pii::tests::iban_with_non_ascii_digits_does_not_panic: runs the publicredact_pii(content) andhas_likely_pii(boundary) paths on the same text and expects it left alone.Documentation
Not needed. A comment on
valid_ibanexplains the guard.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit