feat(ENG-1084): defender hardening, detect-and-gate refactor + review fixes#73
Open
hiskudin wants to merge 10 commits into
Open
feat(ENG-1084): defender hardening, detect-and-gate refactor + review fixes#73hiskudin wants to merge 10 commits into
hiskudin wants to merge 10 commits into
Conversation
…riting it Defender no longer mutates tool-result content. defendToolResult().sanitized now returns the original value (optionally boundary-wrapped); role-marker stripping, pattern/encoding redaction, and in-place [CONTENT BLOCKED] replacement are removed from the returned payload. Blocking is expressed solely via allowed: false; detection metadata (detections/fieldsSanitized/ patternsByField) is now sourced directly from the detector, so every detected pattern is reported (fixes previously-dropped medium-severity matches). Boundary annotation and all Tier 1/2/3 detection/scoring are unchanged. BREAKING CHANGE: result.sanitized is no longer redacted/normalized. Consumers must gate on result.allowed rather than relying on rewritten content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Tier 1 encoding detector runs on every risky field with no length cap,
and its Morse regex `(?:[.-]+[ ]){4,}[.-]+` backtracked catastrophically on
long dot/dash runs (200k dots ≈ 30s of blocked event loop). A remotely
supplied email/document body could freeze the host process.
- Linearize the Morse regex by bounding each symbol group to {1,8} (longer
runs are not valid Morse anyway).
- Bound the markdown_hidden_instruction regex with negated char classes
instead of lazy `.*?` spans (defense-in-depth; it was already capped by the
detector's 50k input limit).
- Add `maxFieldAnalysisLength` (default 50000): cap the text Tier 1 runs heavy
regex/encoding detection over; flag `metadata.analysisTruncated` past the cap.
- Fix the size-metrics bypass: risky-field strings now count toward the
traversal `maxSize` budget (they previously skipped updateSizeMetrics).
Regression test: a 200k-char dot field now completes in <2s (was ~30s).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tier 2 is on by default, but onnxruntime-node / @huggingface/transformers are optional peer deps. Previously, if they were missing the model load failure was swallowed to a per-string skipReason with no top-level signal — consumers who set blockHighRisk believed they had ML defense but silently ran Tier 1 only. - Add `result.tier2Available` — set to `false` when Tier 2 is enabled but the model/runtime can't load, so monitoring can alert on degraded ML defense. - Warn once per instance (was: once per failed call — removed the per-call warn in onnx-classifier.loadModel; the caller now owns messaging). - Add `requireTier2` option: when true, defendToolResult and warmupTier2 throw instead of degrading (fail-closed). - warmupTier2 now fails open by default (warn) rather than throwing, matching the lazy path; it throws only under requireTier2. - README: add a Requirements section documenting the peer deps + requireTier2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rray data (H1, H6) - H1: object KEYS are now scanned by the Tier 1 detector (detect-only — keys are never rewritten, which would change the object shape). An injection hidden in a key now contributes to detections/risk/allowed; previously keys were copied verbatim and invisible to every tier. - H6: large arrays (> largeArrayThreshold) no longer drop items. The previous behavior returned only the first 100 items plus a notice, silently losing the rest of the payload. Now all items are returned; only Tier 1 detection is capped to the first 100, and coverage is flagged. - Add result.coverageDegraded: true when Tier 1 detection coverage was reduced (field over maxFieldAnalysisLength, large-array partial scan, or a depth/size traversal limit). Content is always returned in full. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- M7: remove the now-inert blockHighRisk from the sanitizer layer (detect-and-gate
made it dead). The only remaining flag is PromptDefenseConfig.blockHighRisk,
resolved once; config:{blockHighRisk} and the blockHighRisk option now gate
identically.
- M15: guard Tier2Classifier.classify() against a NaN logit (matches the batch
paths) so a broken model output isn't silently treated as benign.
- L1: overallRiskLevel now propagates a per-field critical (via a raiseOverallRisk
max-helper) instead of capping at high; cumulative escalation raises rather than
overwrites.
- L3: riskLevel floor is now low (was medium), so benign payloads report low and the
full low..critical range is reachable. riskLevel is diagnostic only; blocking
logic is unchanged.
- L2 (already fixed in Phase 0): regression test that a medium-severity match appears
in detections.
- README: update the risk-level section/table (starts at low; drop stale
stripped/redacted language).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er 3 registry (M9/M10/M11)
- M9: load the optional peers (onnxruntime-node, @huggingface/transformers) via a
shared dynamicImport() helper that passes a runtime specifier to the real
import() operator, so bundlers (webpack/esbuild) don't try to resolve them at
build time. Refactor the SFE fasttext.wasm load onto the same helper — its old
`new Function("return import(spec)")` trick threw "A dynamic import callback was
not specified" and silently failed open, so SFE never actually loaded; it works
now.
- Because the peers are no longer statically imported, tsc lost the transitive
@types/node; declare @types/node explicitly and add "node" to tsconfig types.
- M10: per-condition types in the exports map so ESM consumers resolve
dist/index.d.mts instead of the CJS-flavored .d.cts.
- M11: back the Tier 3 provider registry with a globalThis Symbol slot so an app
that loads both the CJS and ESM builds shares one registry (a module-level
singleton gave each build its own, so a registered provider was invisible to the
other realm).
Verified: build emits no literal peer specifiers in dist; dist/index.d.mts present
and wired; 353 tests + typecheck + lint green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Detect-and-gate (Phase 0) unwired the entire content-mutation path. This removes the now-unreachable code — traced to spec-only usage; none of it is exported from the package entry, so it's non-breaking for consumers and was already tree-shaken out of the shipped bundle. Deleted (whole files, the mutation path): - src/sanitizers/sanitizer.ts (Sanitizer, createSanitizer, sanitizeText, suggestRiskLevel) - src/sanitizers/pattern-remover.ts (removePatterns + variants) - src/sanitizers/role-stripper.ts (stripRoleMarkers, containsRoleMarkers, findRoleMarkers) Trimmed dead exports from files that stay: - encoding-detector: redactAllEncoding, decodeAllEncoding, containsEncodedContent - normalizer: containsSuspiciousUnicode, analyzeSuspiciousUnicode - boundary: generateXMLBoundary Docs (M18): fix the tier3_only line (T1 detects, doesn't strip) and the Risky Field Detection section (clarify Tier 1 value scoping vs Tier 2 scan-all + key scanning). Specs updated to drop deleted-symbol tests; detection coverage (normalizer / leet / encoding-detection) retained. 313 tests + typecheck + lint + build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Defender by completing the “detect-and-gate” refactor: tool-result content is no longer rewritten/redacted, and instead Defender detects/scorers threats and gates forwarding via allowed (with optional boundary annotation). It also incorporates review-driven security/performance fixes (notably ReDoS guards), improved Tier 2 availability signaling, and packaging/doc/test updates to match the new model.
Changes:
- Refactors Tier 1 from mutation to detection-only: preserve original content in
result.sanitizedwhile recording detections/risk and gating viaallowed. - Adds ReDoS/DoS protections (bounded Morse + markdown-link regex, per-field analysis cap) and expands Tier 1 coverage (object key scanning, large-array handling + degraded-coverage signaling).
- Makes Tier 2 optional-peer dependency failures explicit (
tier2Available, warn-once,requireTier2fail-closed), adds bundler-safe dynamic imports, and updates exports/types + docs/tests.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds Node types for build/test typing support. |
| src/utils/dynamic-import.ts | Bundler-resistant helper for optional peer dependency loading. |
| src/utils/boundary.ts | Removes XML boundary helper; keeps UD boundary wrapping path. |
| src/types.ts | Adds analysisTruncated metadata to surface capped Tier 1 coverage. |
| src/sfe/preprocess.ts | Switches optional peer import to dynamicImport for bundler safety. |
| src/sanitizers/sanitizer.ts | Removes mutation-based composite sanitizer implementation. |
| src/sanitizers/role-stripper.ts | Removes role-stripping mutator utilities (now detection-only model). |
| src/sanitizers/pattern-remover.ts | Removes pattern redaction utilities (now detection-only model). |
| src/sanitizers/normalizer.ts | Removes unused suspicious-unicode analysis helpers. |
| src/sanitizers/index.ts | Stops exporting removed mutation utilities; keeps analysis helpers needed by detectors. |
| src/sanitizers/encoding-detector.ts | ReDoS-hardens Morse detection and trims mutation-oriented helpers. |
| src/core/tool-result-sanitizer.ts | Implements detect-and-gate, analysis capping, key scanning, large-array behavior, and risk aggregation changes. |
| src/core/prompt-defense.ts | Updates API semantics/docs, Tier 2 availability handling (tier2Available, requireTier2), and coverageDegraded reporting. |
| src/config.ts | Adds default per-field analysis cap constant for Tier 1. |
| src/classifiers/tier3-orchestrator.ts | Makes Tier 3 provider registry dual-package-safe via globalThis Symbol slot. |
| src/classifiers/tier2-classifier.ts | Guards against NaN/non-finite logits in single-text classify path. |
| src/classifiers/patterns.ts | ReDoS-hardens a markdown-link injection regex (bounded, linear). |
| src/classifiers/onnx-classifier.ts | Uses dynamicImport for optional peer deps; reduces repeated warning noise on load failure. |
| specs/utils.spec.ts | Updates tests after XML boundary removal. |
| specs/tier3.spec.ts | Adds regression test for global Symbol provider registry behavior. |
| specs/tier2-availability.spec.ts | Adds fail-open/fail-closed coverage for Tier 2 unavailability. |
| specs/sanitizers.spec.ts | Removes tests for deleted mutation utilities; retains detector/normalizer coverage. |
| specs/integration.spec.ts | Updates expectations to “preserve content + gate” and adds ReDoS/coverage regression tests. |
| README.md | Updates public contract/docs to emphasize detect-and-gate and Tier 2 availability semantics. |
| package.json | Refines conditional exports/types mapping and adds @types/node. |
| package-lock.json | Lockfile updates for dependency graph changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hiskudin
changed the title
`npm install --save-dev @types/node` in the packaging commit bumped @types/node (25.3.3→25.9.4), removed @emnapi/core + @emnapi/runtime, and bumped an unrelated transitive — none of which was intended. Revert package-lock.json to match main and drop the @types/node devDependency. Node types for the typecheck come from the already-transitive @types/node via the `tsconfig types: ["node"]` entry added in Phase 4, so no declared dep or lockfile change is needed. typecheck + 313 tests + lint + build stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hiskudin
changed the title
hiskudin
changed the title
…R review) Copilot flagged that large-array items past the detection scan limit were returned verbatim (`return item`), bypassing not just Tier 1 detection but also prototype-pollution key stripping (DANGEROUS_KEYS), depth/size limits, and boundary wrapping — a dangerous key could hide in a tail element and reach the caller unprocessed. Thread a `detect` flag through the traversal: tail items are still fully traversed for structural protections but skip only the expensive per-string Tier 1 analysis. Data is still never dropped; reduced detection coverage stays flagged via `analysisTruncated`. Regression test: a __proto__ key in a tail item (index 1400, past the 100-item scan limit) is still stripped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hiskudin
changed the title
…output Addresses two PR review comments: - types.ts: the `analysisTruncated` docstring only mentioned the per-field length cap, but the flag is also set when a large array is partially scanned. Broaden the doc so observability consumers aren't misled. - Tier2Classifier.classify(): a non-finite logit (NaN/Infinity) set score 0, which yields confidence 1.0 (max) — a broken inference looked like a max-confidence benign classification. Report it as skipped:true with a skipReason instead of a fake confident-benign result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Summary
Addresses the findings from a full review of the package. The centerpiece is an architectural change: Defender no longer mutates tool-result content. It now detects, scores, and gates —
result.sanitizedreturns the original content (optionally boundary-wrapped) andallowed: falseis the block signal. Content rewriting (role-stripping,[REDACTED],[ENCODED DATA],[CONTENT BLOCKED]) was trivially bypassable, corrupted legitimate data, and was where almost every bug lived — so removing it fixes several findings by construction and deletes a large, buggy surface.7 focused commits, each independently green (tests + typecheck + lint + build). 353 → 313 tests (the drop is the deletion of tests covering removed mutation code; net new regression tests were added throughout).
What's in it
detect-and-gatesanitizedreturns original content; detections sourced from the detector; boundary annotation keptReDoSTier 2 availabilityresult.tier2Availableflag + warn-once + opt-inrequireTier2fail-closed. Missing peer deps no longer silently disable ML defensekey detection + arraysallowed:true); large arrays no longer silently drop items;coverageDegradedflag addedrisk reportingriskLevelnow spanslow..critical;classify()NaN guard; removed theblockHighRisksplit-brainpackagingdead-code + docsBreaking changes
result.sanitizedis no longer redacted/normalized — it's the original content (optionally boundary-wrapped). Consumers must gate onresult.allowedrather than relying on rewritten output.riskLevelnow starts atlow(wasmedium) and can reachcritical. It remains diagnostic-only; block logic is unchanged.Sanitizer,removePatterns, etc.) — none were exported from the package entry, so this is non-breaking for consumers importing@stackone/defender.Deferred
Phase 5 (evasion hardening) — S7 role-marker anchoring, chunk-truncation/density/pattern-gap fixes — is intentionally not here. Those change detection behavior and must be measured against the FP/FN benchmark datasets before shipping, so they belong in a separate benchmark-gated PR.
Verification
npm test(313 passing),npm run test:typecheck,npm run lint, andnpm run buildall clean. New regression tests cover: the ReDoS guard, content-preservation-while-gating, key-injection detection, the Tier 2 fail-open/fail-closed paths, large-array preservation, and the risk-level range.🤖 Generated with Claude Code