Add opt-in Unicode normalization to StringDetector to resist homoglyph evasion#1938
Closed
feiiiiii5 wants to merge 2 commits into
Closed
Add opt-in Unicode normalization to StringDetector to resist homoglyph evasion#1938feiiiiii5 wants to merge 2 commits into
feiiiiii5 wants to merge 2 commits into
Conversation
StringDetector matched by raw byte comparison after optional lower-casing, with no Unicode normalization. A toxic term written with non-ASCII code points (fullwidth characters, zero-width joiners/spaces, zero-width non-joiners) bypassed every substring-based detector. Add a `normalize` DEFAULT_PARAM with three tiers: - None (default): no normalization, preserves backward compatibility - "NFKC": collapses fullwidth homoglyphs and compatibility decomposition - "NFKC+strip_format": additionally strips Unicode category Cf to catch zero-width insertion attacks that NFKC preserves Validation in __init__ and _apply_normalize rejects unsupported values. Normalization applies after case-folding and before matching, for all three matchtypes (str, word, startswith). Known limitations: neither tier collapses Cyrillic/Latin homoglyphs nor combining marks; these require a confusables table and are documented in the class docstring as out of scope. Closes NVIDIA#1867 Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: fei <3303354867@qq.com>
Address review gaps identified against AGENTS.md:
- Remove test_stringdetector_normalize_default_is_none (AGENTS.md:
"Don't add tests for default values given in configurable plugins").
- Rewrite StringDetector docstring to drop default-value phrasing
(AGENTS.md: "Don't include default values in docstrings").
- Pin documented limitations with tests: NFKC does not collapse
cross-script confusables (Cyrillic 'а') nor combining marks
('a' + U+0301). Issue NVIDIA#1867 comments claimed NFKC handles these;
the tests verify it does not, so the docstring is accurate.
- Cover broader Cf category: soft hyphen (U+00AD) and BOM (U+FEFF)
survive NFKC and are stripped only by NFKC+strip_format.
- Cover case_sensitive=True + normalize composition.
- Cover multi-substring matching with one hit.
- Add end-to-end test using real LDNOOBW detector (named in NVIDIA#1867):
fullwidth variant scores 0.0 with default, 1.0 with NFKC.
Test count: 28 (was 23). No regressions across detectors suite.
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: fei <3303354867@qq.com>
feiiiiii5
changed the title
Collaborator
|
See #1884 |
Author
Thanks for the pointer. I'll review #1884 and leave any suggestions there. |
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.
Closes #1867.
Summary
StringDetectormatches by raw byte comparison after optional lower-casing, with no Unicode normalization. A toxic term semantically present but written with non-ASCII code points (fullwidth, zero-width-interrupted) scores clean. This affects every subclass relying on substring matching — issue #1867 namesunsafe_content.LDNOOBWspecifically.This PR adds an opt-in
normalizeparameter so substring-based detectors can resist Unicode homoglyph evasion.Design
The
normalizeDEFAULT_PARAM accepts three values (see_NORMALIZE_CHOICES):None— no normalization; preserves today's behavior so existing scores and tests are unchanged (non-breaking)."NFKC"— appliesunicodedata.normalize("NFKC", ...)to both the candidate substring and the output text before matching. Catches fullwidth homoglyphs and compatibility decomposition."NFKC+strip_format"— NFKC then strips Unicode categoryCf(format characters: ZWJ U+200D, ZWSP U+200B, ZWNJ U+200C, soft hyphen U+00AD, BOM U+FEFF, ...). Catches zero-width insertion attacks that NFKC leaves intact.Validation in
__init__and_apply_normalizerejects unsupported values. Normalization applies after case-folding and before matching, for all three matchtypes (str,word,startswith).Why opt-in (default None)
Turning NFKC on for every existing
StringDetectorsubclass would silently change every reported score and break downstream tests that pin specific numbers. The fix is adopted deliberately per detector; subclasses that want the hardening setnormalizein theirDEFAULT_PARAMS.Known limitations (verified by tests)
NFKC does not collapse:
а(U+0430) and ASCIIa(U+0061) are distinct code points in different scripts; NFKC has no compatibility mapping between them.a+ U+0301 (combining acute) normalizes to precomposedá(U+00E1), which still does not match ASCIIa.Note: issue #1867 comments suggested NFKC handles these. The tests verify it does not, so the docstring is accurate. Handling these requires a confusables table, out of scope for this parameter.
Out of scope (follow-up)
TriggerListDetector(same file) has the same substring-matching pattern and is also vulnerable to Unicode evasion. Issue #1867 only namesStringDetector, so this PR does not touchTriggerListDetector; a follow-up PR could extend the samenormalizeparameter there if maintainers want it.Changes
garak/detectors/base.py: +import unicodedata,normalizeDEFAULT_PARAM,_NORMALIZE_CHOICES,__init__validation,_apply_normalizestatic method,detect()applies normalization.tests/detectors/test_detectors_base.py: 17 new regression tests.Test results
$ python -m pytest tests/detectors/test_detectors_base.py -v
============================== 28 passed in 0.09s ==============================
$ python -m pytest tests/detectors/
================== 756 passed, 34 skipped in 91.41s ==================
$ black --check garak/detectors/base.py tests/detectors/test_detectors_base.py
All done! 2 files left unchanged.
The end-to-end test loads the real
LDNOOBWdetector, picks a substring from its live wordlist, constructs a fullwidth variant, and verifies:normalize=None(default): scores 0.0 (pins the bug — backward compatible)normalize="NFKC": scores 1.0 (evasion caught)Non-duplication
Searched open PRs; no existing PR addresses Unicode normalization for
StringDetector. Issue #1867 has two commenters who said they would send a PR but neither has in the three weeks since.AI assistance
This PR was authored with assistance from Claude (Anthropic). The design follows the direction proposed in issue #1867 by AUTHENSOR and cschanhniem. Implementation, tests, and limitation verification were reviewed and iterated on by the contributor. No AI-generated content was committed without human review.
Co-authored-by: Claude noreply@anthropic.com
Signed-off-by: fei 3303354867@qq.com