Skip to content

fix(exaforce): anchor the P5 and PE3 patterns that match inside unrelated words - #16

Open
smoy wants to merge 1 commit into
mainfrom
smoy/anchor-unanchored-patterns
Open

smoy wants to merge 1 commit into
mainfrom
smoy/anchor-unanchored-patterns

Conversation

@smoy

@smoy smoy commented Sep 15, 2026

Copy link
Copy Markdown

Summary

  • The two highest verdicts in a production fleet scan are regex bugs, and both are one missing boundary: P5 matches kill inside skill -- in a scanner whose entire input domain is skills -- and PE3 matches .env inside process.env in a front-end style guide. Both shipped as 100 / CRITICAL / DO_NOT_INSTALL; neither row contains a security defect.
  • Fixed as fork-local overrides in exaforce/, not as edits to the upstream analyzers, so an upstream merge cannot silently leave a stale correction applied or shadow an upstream fix -- replace_pattern raises PatchDriftError if either pattern is rewritten.
  • Confidence is untouched. These change what a pattern matches, never how much a match is believed.

Tests: 34 in tests/exaforce (19 new), 1820 in tests/unit + tests/nodes, ruff check src/ tests/ clean.

Details

Evidence. From a re-adjudication of 111 shipped findings (100 unique skill content hashes, 50 repos, scanned 2026-09-11). P5 emits at CRITICAL with confidence 0.95 -- 47 of the 50 available CRITICAL points before any other rule contributes -- so a single unanchored match nearly maxes the unit. PE3 fired twice on the same style guide, including on the line reading "Access environment variables safely" and on the CSV's anti-pattern column.

The regexes. \b before P5's verb alternation; (?<![\w.]) before PE3's dot. Verified in both directions: how to kill someone / poison a person / murder people still fire, and cat .env, ~/.env, ./.env, app/.env, .env.production, .env.local and a line-terminal .env all still fire.

PE3's narrowing does not create a blind spot. process.env[...KEY|SECRET|TOKEN|PASSWORD...] and Object.keys(process.env) remain covered by static_patterns_data_exfiltration.py.

The boundary-anchoring is domain-neutral and worth offering upstream to NVIDIA/skillspector. This module is the interim, and PatchDriftError is what makes dropping it safe once upstream lands its own fix.

Self-review summary

/code-review medium, 4 findings, all fixed.

  • The tests would have turned CI red as first written -- ruff B009 on getattr with a constant attribute.
  • The tests asserted against pattern strings, never running analyze(). If upstream refactored to pre-compile patterns at import, replace_pattern would still swap the string, every test would still pass, and the patch would be silently inert in a real scan. Rewritten end-to-end through analyze().
  • A helper compiled with DOTALL where the PE3 analyzer does not, making the positive-direction guarantee weaker than it read.
  • The replace_pattern docstring promised a raise that does not happen when upstream's fix is byte-identical to ours (that branch is what keeps apply() idempotent); it now says so.

Added after the review: test_the_patch_is_what_removes_the_finding. skillspector applies fork patches at package import, so a test that only asserts "no finding" reads identically whether the patch did the work or the analyzer never fired. It restores the upstream pattern, asserts the false positive appears, and puts ours back -- confirmed both fire unpatched and neither fires patched.

Signed-off-by: Steven Moy <smoy@exaforce.com>
@smoy
smoy force-pushed the smoy/anchor-unanchored-patterns branch from b30afa3 to f354595 Compare September 15, 2026 19:20
@smoy
smoy requested a review from will-exaforce September 15, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant