Sanitize uppercase and mixed-case confusable IDN domains - #74
Conversation
The IDN detector lowercases the domain before analysis (`Detector::Idn#initialize`), so its detections carry a lowercased label. The sanitizer then substituted that label into the original-cased field with a case-sensitive `gsub`, so an uppercase or mixed-case confusable domain was detected as a spoof yet returned unchanged. Match the label case-insensitively when substituting the punycode. Domain labels are case-insensitive and `Dnsruby::Name.punycode` nameprep-folds case, so the substituted encoding stays canonical. Quoted-string and local-part detections carry exact-case labels and are unaffected; cross-script confusables do not case-fold to ASCII, so benign ASCII names and local parts are left untouched.
There was a problem hiding this comment.
Pull request overview
Fixes sanitization of uppercase or mixed-case confusable IDN domains.
Changes:
- Uses case-insensitive label substitution.
- Adds IDN and email regression tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/homographic_spoofing/sanitizer/base.rb |
Adds case-insensitive punycode replacement. |
test/sanitizer/idn_test.rb |
Tests mixed-case IDN sanitization. |
test/sanitizer/email_address_test.rb |
Tests IDNs in email domains and display-name preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2e488f903
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e28cbaf109
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e28cbaf to
14390b1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14390b13e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
14390b1 to
17d020f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17d020f1d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
17d020f to
433909e
Compare
…tion
The first cut lowercased the detection label and substituted it back with a
case-insensitive gsub (/i). That both over-matched and under-matched:
- Over-match: /i applied to every detection type, across the whole field, so
a confusable local part rewrote a benign name that differed only by case —
a long-s (ſ) local part folding onto an ASCII "Support" name, a
small-capital-w (ᴡ) local part folding onto its "Tᴡitter" case-variant name.
- Under-match: regexp case folding does not pair every uppercase letter with
the lowercase form String#downcase produces (e.g. Ⱥ/ⱥ, Ⱦ/ⱦ), so those
uppercase domains were detected yet left unsanitized on affected Rubies.
Have Detector::Idn report each label in the original case it occupies in the
input domain, and keep the sanitizer's substitution an exact match. The detector
only lowercases for analysis, so recover the original casing by finding the run
of the domain whose lowercase equals the label. Matching content rather than a
fixed offset stays correct where offset arithmetic would not: when a character
lowercases to a different length (İ → i̇), and when PublicSuffix stripped
surrounding characters the raw domain still carries. Domain labels are
case-insensitive by spec and Dnsruby::Name.punycode folds case when encoding, so
the substituted punycode stays canonical.
433909e to
a58dbc2
Compare
Problem
Detector::Idn#initializelowercases the domain before analysis (@domain = domain.downcase), so every IDN detection carries a lowercaseddetection.label.Sanitizer::Base#punycodethen substituted that label into the original-cased field with a case-sensitivegsub:So an uppercase or mixed-case confusable domain was detected as a spoof but returned unchanged — silent under-sanitization. Confirmed on
main(ruby 4.0.6):This affects both public entry points that touch a domain:
sanitize_idnandsanitize_email_address(the latter routes the address's domain part throughDetector::IdninDetector::EmailAddress#detections), so every downstream sink that punycodes a domain-bearing identity was under-sanitized for uppercase/mixed-case confusables.Fix
Match the label case-insensitively when substituting:
Domain labels are case-insensitive by spec, and
Dnsruby::Name.punycodenameprep-folds case (punycode("Аpple") == punycode("аpple") == "xn--pple-43d"), so the substituted encoding stays canonical regardless of the matched case.Why this is safe (reviewer notes)
Non-IDN paths unaffected in practice. Quoted-string and local-part detections carry exact-case labels (their detectors do not lowercase), so
/istill matches the same run; the replacement value is unchanged.Benign text is not over-matched. IDN detection labels always contain a non-ASCII confusable character, and cross-script characters do not case-fold together under
/i(ASCIIa≠ Cyrillicа). A benign ASCII display name or local part alongside a confusable domain is left untouched — locked in by a regression test:Whole-field substitution is pre-existing. The per-detection
gsubalready replaced a confusable token across every part of an address (e.g.tᴡitter@tᴡitter.com); this change only extends that to case-variants of the same token, and Dnsruby's canonical encoding keeps the output correct. The only residual is same-script case-fold pairs (e.g.ſ/s) — display-only, requires the benign run to contain the identical confusable letters, and still yields a valid sanitization; not addressed here.Block-form replacement also avoids backreference interpretation in the
xn--output.Tests
New regression tests (fail on
main, pass here) insanitizer/idn_test.rbandsanitizer/email_address_test.rb: uppercase confusable domain punycoded, benign uppercase ASCII domain untouched, uppercase confusable domain inside an email address, and the benign-ASCII-name guard above. Full suite green (bin/test: 43 runs, 307 assertions, 0 failures).Spun off from HackerOne #3960349 / basecamp/haystack#8670, where the Imbox sender-byline path worked around this by detecting on a pre-lowercased domain.