fix: catch tags, variation selectors, noncharacters and PUA in the hostname screen (#610) - #625
Conversation
…stname screen (#610) Third in the sequence after #603 (bidi controls) and #605 (zero-width). 17 of 18 sampled code points passed is_suspicious_hostname() clean, and all 18 survived into `canonical`. The one that did flag was flagging for the wrong reason, and it is the #605 bug in a class #605 did not cover. U+FDD0 sits in the Arabic Presentation Forms range, so the script detector read it as a letter: `paypal<U+FDD0>.evil.com` reported scripts=['Latin', 'Arabic'] with mixed_script=True. Widening the existing per-label strip fixes that by construction rather than by special case, because that strip already runs before detect_scripts. Reported on the EXISTING has_invisible field rather than four new ones. No binding payload changes: nothing added to the Ruby positional tuple, no edit to the Java jni_sig! string — the two places in this repo whose failure mode is a runtime error in CI rather than a compile error locally. `is_invisible_in_hostname` composes the four class predicates src/invisibles.rs already carried. Private use and the variation selectors are in scope because IDNA2008 (RFC 5892) disallows every class in a hostname. Both have legitimate uses in ordinary text, so a general-text detector needs its own argument for them; that stays out of this change. Measured against the full suite including the adversarial-oracle clean corpus: no false positives. The tag block is why this is a security fix and not tidying. U+E0061-U+E007A spell arbitrary Latin invisibly, and the screen previously called such a hostname clean AND returned the payload intact in `canonical` — the combination that turns a detector into a laundering step. The #605-review drift gate needed restructuring, not just extending. It derived the flagged set by probing every Cf/Zs code point and compared it to a literal list. Three of the four new classes sit outside Cf/Zs (variation selectors are Mn, noncharacters Cn, PUA Co) and probing them exhaustively is ~138k round trips into Rust — PUA alone is 137,468 code points. So the gate stays exhaustive over the format space it can afford, computes its expectation from the zero-width singletons plus the tag block, and says in its docstring that TestRemainingInvisibleClasses covers the other three by representative member. The prose requirement now demands every class be named in all three files that enumerate the set. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
|
📄 Docs preview: https://f22971bd.disarm-docs.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Several updated docs/comments claim RFC 5892 “disallows every class” in has_invisible, but the zero-width subset includes ZWNJ/ZWJ which are CONTEXTJ (conditionally permitted), making the justification text inaccurate/internally inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR closes #610 by extending the hostname screen (is_suspicious_hostname) to treat additional IDNA2008-invalid “invisible-ish” classes (tags, variation selectors, noncharacters, and private-use code points) as has_invisible, stripping them per-label before script detection so they cannot leak into scripts, mixed_script, or canonical.
Changes:
- Widen the per-label “strip-before-script-detect” step in
src/hostname.rsfrom zero-width only to include tags / variation selectors / noncharacters / PUA via a composed predicate. - Update the public API documentation (Rust, Python, docs site, changelog) to enumerate the expanded
has_invisiblecoverage. - Restructure and extend tests to cover the added classes and update the drift gate to remain fast while still enforcing documentation consistency.
File summaries
| File | Description |
|---|---|
src/hostname.rs |
Expands the invisible-stripping predicate used before script analysis. |
src/api/safety.rs |
Updates Rust HostnameAnalysis.has_invisible documentation to enumerate new classes. |
python/disarm/_api.py |
Updates Python is_suspicious_hostname docstring for expanded has_invisible. |
docs/api/predicates.md |
Updates published API docs for HostnameAnalysis.has_invisible. |
tests/test_hn_features.py |
Updates drift gate structure and adds representative tests for the new invisible classes. |
CHANGELOG.md |
Adds release notes describing the security fix and behavior changes. |
Review details
Suppressed comments (1)
src/hostname.rs:249
- This block repeats the same RFC 5892 justification (“disallows every class below”), but the zero-width set being stripped includes ZWNJ/ZWJ which are CONTEXTJ under IDNA2008 (conditionally permitted). Consider rewording to avoid stating they are outright disallowed while keeping the fail-closed policy rationale.
// IDNA2008 (RFC 5892) disallows every class below, so a hostname carrying one is
// malformed whatever its intent and the screen can fail closed on all of them.
// That is why PUA and the variation selectors are included here but would need a
// separate argument in a general-text detector, where both have legitimate uses.
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…wrong (#610) From review on #625, in five places. RFC 5892 puts the four classes #610 adds — tags, variation selectors, noncharacters, private use — in DISALLOWED outright, and that is what justifies folding PUA and the variation selectors into a hostname screen when both have legitimate uses in ordinary text. But the claim was written over the whole `has_invisible` set, which also contains the pre-existing zero-width members, and U+200C/U+200D are CONTEXTJ: permitted in the specific joining contexts Appendix A.1/A.2 describes. Flagging them anyway is a deliberate fail-closed policy chosen in #605 — a spoof screen has no reason to honour a context rule it cannot verify — and stating it as an RFC prohibition misrepresented both the RFC and the decision. Each site now scopes the DISALLOWED claim to the four new classes and names the CONTEXTJ exception as policy. No behaviour change. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
Closes #610. Third in the sequence after #603 (bidi controls) and #605 (zero-width).
The gap
17 of 18 sampled code points passed
is_suspicious_hostname()clean, and all 18 survived intocanonical.canonicalbeforeU+E0000–U+E007F)The one that flagged was flagging for the wrong reason
It is the #605 bug in a class #605 did not cover.
U+FDD0sits in the Arabic Presentation Forms range, so the script detector read it as a letter:Right verdict, wrong evidence — and
scriptsis simply wrong for any caller who reads it. Widening the existing per-label strip fixes this by construction rather than by special case, because that strip already runs ahead ofdetect_scripts.No binding payload changes
Reported on the existing
has_invisiblefield rather than four new ones. Nothing is added to the Ruby positional tuple and the Javajni_sig!string is untouched — the two places in this repo whose failure mode is a runtime error in CI rather than a compile error locally.is_invisible_in_hostnamejust composes the four class predicatessrc/invisibles.rsalready carried (is_tag,is_variation_selector,is_noncharacter,is_pua).Why PUA and variation selectors are in scope here
IDNA2008 (RFC 5892) disallows every one of these classes in a hostname, so the screen can fail closed on all of them with no legitimate-use tradeoff. Both have real uses in ordinary text, so a general-text detector needs its own argument — deliberately out of scope here.
That was the risk flagged when planning this, so it was measured rather than assumed: the full suite passes, including the adversarial-oracle clean corpus. No false positives.
Why this is a security fix and not tidying
The tag block is the ASCII-smuggling channel
src/invisibles.rswas written for.U+E0061–U+E007Aspell arbitrary Latin invisibly, and the screen previously called such a hostname clean and returned the payload intact incanonical— the combination that turns a detector into a laundering step. There is a test for exactly that.The drift gate needed restructuring, not extending
TestInvisibleSetDoesNotDriftFromItsDocs(added in #606 review) derived the flagged set by probing everyCf/Zscode point and compared it to a literal list.Three of the four new classes sit outside
Cf/Zs— variation selectors areMn, noncharactersCn, PUACo— and probing them exhaustively is ~138k round trips into Rust. PUA alone is 137,468 code points, which is minutes rather than the 0.3s the gate is worth.So it stays exhaustive over the format space it can afford, computes its expectation from the zero-width singletons plus the tag block, and states in its docstring that
TestRemainingInvisibleClassescovers the other three by representative member. The gap is written down rather than left implicit. The prose requirement now demands every class be named in all three files that enumerate the set.Verification
Full local gate: 4,731 pytest, 24 Rust test targets,
cargo fmt --check, clippy clean on both feature sets,scripts/perf_lint.shclean, no pyo3 in the pure tree, mypy clean, ruff clean, language-consistency audit clean.Still open from #612
The detection half — nothing reports edge whitespace, and
split_tokensnever emits it, so a check has to be a whole-text pass touching bothhas_anomaliesandinspect_anomalies. That is the next change, together with the twoAnomalyKindTypeScript unions that ship withoutbidi_mixedtoday.