feat(detectors): add Unicode normalisation to StringDetector#1884
feat(detectors): add Unicode normalisation to StringDetector#1884PhinehasNarh wants to merge 3 commits into
Conversation
| text = unicodedata.normalize("NFKC", text) | ||
| return "".join(c for c in text if unicodedata.category(c) != "Cf") | ||
| else: | ||
| raise ValueError(f"Don't know how to process normalize: {self.normalize!r}") |
There was a problem hiding this comment.
Raising a ValueError in a base detector can cause the the run to terminate. Looking at the changes in this PR, calls to this _apply_normalize() method need to be wrapped in a try block and the value error should cause the detection to report the appropriate value either 0.0 for no hit or None to indicate that detection could not be preformed.
There was a problem hiding this comment.
Good catch - a ValueError in a base detector would have killed the whole run, which is obviously wrong.
Fixed in the latest push: still raises internally (the error itself is valid), but now wraps the call in a try/except ValueError, logs a warning, and appends None to signal that detection could not be performed for that output. The run continues cleanly.
Updated test_stringdetector_normalize_invalid_value to match: it now asserts results == [None] instead of expecting the exception to propagate. All 17 tests pass.
Closes NVIDIA#1867. StringDetector matched against raw bytes, so a flagged term written in fullwidth homoglyphs (ｈｅｌｌｏ → NFKC → hello) or with zero-width joiners spliced mid-word scored 0.0 (clean) instead of 1.0 (hit). Every detector built on StringDetector — unsafe_content.LDNOOBW, lmrc.Anthro, goodside.RileyIsnt, and ~30 others — shared this false-negative. Add a `normalize` parameter (default None, backward-compatible): - None — current behaviour, no normalisation - "NFKC" — unicodedata.normalize("NFKC", …) on both trigger and output; collapses fullwidth, combining marks, most homoglyphs - "NFKC+strip_format" — NFKC then strip Unicode category Cf characters (zero-width joiners, ZWSP, ZWNJ, BOM, etc.); needed to catch ZWJ-interrupted evasion that NFKC alone preserves Six regression tests are added to test_detectors_base.py, one of which confirms the bypass still succeeds at normalize=None (baseline) and the rest verify each normalisation tier closes the gap. Co-authored-by: Claude Signed-off-by: PhinehasNarh <phinehastettehnarh@gmail.com>
Per reviewer feedback: raising ValueError in a base detector terminates the run. Wrap the _apply_normalize() call on output_text in a try/except; on unknown normalize value log a warning and append None so the caller knows detection could not be performed for that output. Update test_stringdetector_normalize_invalid_value to assert None is returned instead of expecting ValueError to propagate. Signed-off-by: PhinehasNarh <phinehastettehnarh@gmail.com>
PhinehasNarh
force-pushed
the
feat/stringdetector-unicode-normalize
branch
from
6b95d13 to
235f274
Compare
Follow-up to @jmartin-tech's review: the trigger-substring normalization call was still unguarded. While it was only reachable when normalize was valid (the output-text call short-circuits on invalid config), a stricter reading of 'wrap the calls' left it exposed and it re-normalized every substring for each output. Hoist substring normalization out of the loops so it runs once, wrapped in the same try/except: an invalid normalize config now logs a warning and returns None for all outputs. The per-output guard is kept so the empty-substrings edge is still handled. Detection iterates the pre-normalized substrings. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: PhinehasNarh <phinehastettehnarh@gmail.com>
|
Circled back on this @jmartin-tech - the earlier fix only wrapped the output-text call. There was a second Latest push hoists the substring normalization out of the loops so it happens once, wrapped in the same try/except: bad normalize config logs a warning and returns None for all outputs. Kept the per-output guard too so the empty-substrings case is still covered. 17/17 detector tests pass, and I checked the invalid-normalize path directly against the real detector (returns all None, no raise). |
2 participants
What this changes
Adds a
normalizeparameter toStringDetector(defaultNone, fully backward-compatible).When set, both the trigger strings and the model output are normalised before any matching occurs:
None"NFKC"unicodedata.normalize("NFKC", …)— collapses fullwidth code points, combining marks, and most homoglyphs to their ASCII equivalents"NFKC+strip_format"Why this matters
Without normalisation, a term in the trigger list that is also present in a model output but written with non-ASCII code points is not byte-equal to the listed ASCII substring and is missed:
This affects every
StringDetector-based detector (unsafe_content.LDNOOBW,lmrc.Anthro,goodside.RileyIsnt, and ~30 others). garak already recognises this evasion class on the probe side (probes.badcharactersgenerates homoglyph/invisible payloads), so leaving detectors blind to it was an asymmetry, not an intentional scope decision.Closes #1867.
Why this is not a duplicate
No existing open PR addresses
StringDetectorUnicode normalisation. PR #1880 and PR #1882 address a separate bug (unescaped regex metacharacters in word-boundary matching). Thenormalizeparameter introduced here is independent of that fix.Test commands run and results
Six new tests cover:
normalize(confirms the vulnerability)normalize="NFKC"catches fullwidth evasionnormalize="NFKC"does not break plain ASCII detectionnormalize="NFKC"alone does not strip ZWJ (documents the two-tier design)normalize="NFKC+strip_format"catches ZWJ-interrupted evasionnormalizevalue raisesValueErrorAI assistance disclosure
This PR was written with AI assistance (Claude). I reviewed every changed line, ran the tests, and can defend the implementation end-to-end. The design follows the two-tier approach discussed in issue #1867.