Skip to content

test(review): add adversarial prompt-injection regression fixtures - #3565

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
test/prompt-injection-adversarial-fixtures
Jul 5, 2026
Merged

test(review): add adversarial prompt-injection regression fixtures#3565
loopover-orb[bot] merged 1 commit into
mainfrom
test/prompt-injection-adversarial-fixtures

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #2998

Summary

  • Test coverage gap: src/services/ai-review.ts sends untrusted PR title/body/diff/changed-file-paths into the AI reviewer prompt (defanged only when the opt-in features.safety flag is on, per src/review/safety.ts). The existing test/unit/prompt-injection.test.ts had exactly one example per detection category — no adversarial variations, no documented limitations, no diff-realistic fixture.
  • Added: a comprehensive adversarial-fixture suite grouped into three sections:
    • Caught — realistic obfuscation techniques an attacker could plausibly try today: line-break splitting, HTML-comment hiding (renders invisibly on GitHub), markdown emphasis wrapping, all-caps shouting, fake role-label prefixes ("SYSTEM:"), request-phrased (not command-phrased) manipulation, a diff-comment-hidden instruction, a fake commit-trailer line, a zero-width-space insertion, and roleplay framing.
    • Documented limitation — three genuinely open-ended attack classes the current regex-based approach does not and should not chase in this PR: Unicode homoglyph substitution, synonym wording outside the fixed verb/noun lists, and a gap exceeding the bounded character budget. Pinned as explicit, named tests so a future change to this behavior is a deliberate, reviewed decision rather than silent drift.
    • No false positives — realistic benign PR content that superficially resembles trigger phrases (a PR about this exact feature, an admin "approve" endpoint, a .gitignore change, rubric documentation).
  • Real bug found and fixed while writing the fixtures: the injection regex's bounded gaps ([^.\n]{0,40} etc.) excluded newlines entirely, so "ignore all previous\ninstructions" — a single line break, well within the character budget — slipped past detection. Removed the newline exclusion in src/review/prompt-injection.ts (keeping the period-boundary and character-count bounds, which are what actually prevent an over-broad match spanning two unrelated sentences). No ReDoS risk: every gap stays a fixed, small, bounded quantifier ({0,24}{0,40}), never unbounded.
  • Also added: one end-to-end wiring test through the real runGittensoryAiReview pipeline (test/unit/safety-wiring.test.ts), using the existing capturingAiEnv helper, for the issue's specific "hidden instruction embedded in a diff comment" scenario — confirms the manipulation span is redacted while the surrounding legitimate diff content reaches the model untouched.
  • Confirmed out of scope, documented in code: the issue also names "a commit message crafted to manipulate the reviewer's output" as a threat scenario. GittensoryAiReviewInput.diff is typed as "a bounded unified-diff-ish string built by the caller (filenames + patches)" — commit messages are a separate GitHub API concept (GET /commits) that this codebase never fetches into the AI review input. This attack vector has no path to the model today; noted as a comment in the test file rather than exercised as a redundant test.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run typecheck (clean)
  • npx vitest run test/unit/prompt-injection.test.ts test/unit/safety-wiring.test.ts test/unit/safety.test.ts test/unit/ai-review.test.ts — 178/178 passing
  • Scoped coverage check (vitest --coverage --coverage.include=src/review/prompt-injection.ts): 100% statements/branches/functions/lines on the one touched source file.
  • npm run test:workers / npm run build:mcp / npm run test:mcp-pack / npm run ui:openapi:check / npm run ui:build — not run individually; no worker/MCP/OpenAPI/UI surface touched.
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — see Summary; both sides of the fixed newline-gap branch are covered (caught now, plus a still-bounded-and-still-rejected multi-newline-beyond-budget case), and the existing period-boundary behavior is re-verified unchanged.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A; no auth/session/CORS surface touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no request/response contract change.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI change.
  • Visible UI changes include a UI Evidence section below. — N/A, no visible UI change.
  • Public docs/changelogs are updated where needed. — N/A, internal review-engine hardening, no documented/user-facing behavior change.

Expand test/unit/prompt-injection.test.ts with realistic reviewer-
manipulation techniques (line-break splitting, HTML-comment hiding,
markdown emphasis, all-caps shouting, fake role labels, diff-comment
hiding, commit-trailer framing, zero-width-space insertion, roleplay
framing) plus a documented-limitation section pinning what the current
pattern list deliberately does not catch (Unicode homoglyphs, open
vocabulary, over-budget gaps) and a false-positive section over
realistic benign PR content.

Fix a real gap the fixtures found: the injection regex's bounded gaps
excluded newlines entirely, so "ignore all previous\ninstructions" (a
single line break) slipped past detection even though the character
budget was still respected. Removing the newline exclusion (keeping
the period-boundary and character-count bounds, which is what
actually prevents over-broad matches) closes it with no ReDoS risk
(all gaps stay fixed-upper-bounded) and no new false positives.

Also adds one end-to-end wiring test through the real
runGittensoryAiReview pipeline for the specific "hidden in a diff
code comment" scenario, confirming the manipulation span is redacted
while the surrounding legitimate diff content is untouched.

Closes #2998
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 5, 2026
@loopover-orb

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-05 17:22:47 UTC

3 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR fixes a real, traced bug in the prompt-injection regex: the bounded gaps (`[^.\n]{0,40}` etc.) excluded newlines, so a trivially line-wrapped instruction like "ignore all previous\ninstructions" evaded detection despite being well within the character budget — removing `\n` from the exclusion (keeping the `{0,N}` bound as the real safeguard) is a root-cause fix, not a symptom patch. The new adversarial fixtures exercise the real exported `hasPromptInjection`/`neutralizePromptInjection` functions directly (not a reimplementation) and add one genuine end-to-end test through `runGittensoryAiReview` proving a diff-hidden instruction is actually redacted in the model-bound prompt. The change is narrowly scoped to the linked issue (#2998), applied consistently across all three affected patterns, and directly validated by the new multi-line-split test cases.

Nits — 6 non-blocking
  • src/review/prompt-injection.ts: widening the three gaps to cross newlines slightly increases the chance of a false match spanning two unrelated clauses separated only by a line break (no terminal period) rather than an intentional continuation — bounded by the existing 24-40 char caps so low severity.
  • test/unit/prompt-injection.test.ts: the 'documented limitation' block pins current non-detection of homoglyphs/synonyms as permanent test assertions — consider linking a follow-up issue for NFKC normalization rather than just an inline comment.
  • test/unit/safety-wiring.test.ts: several fixtures assemble secrets/tokens at runtime via string concatenation to dodge this repo's own secret scanner — a reasonable necessity, already explained by an adjacent comment.
  • Consider adding a fixture where a gap crosses a newline AND exceeds the character budget together, to pin the exact boundary where the newline-inclusion fix stops helping.
  • The 'no false positives' section is solid — consider one more case with a benign multi-paragraph PR body (line breaks, no periods) to directly validate the cross-clause false-match nit above.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2998
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 56 registered-repo PR(s), 46 merged, 521 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 56 PR(s), 521 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 56 PR(s), 521 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.11%. Comparing base (e4ec0ca) to head (c25ff00).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3565   +/-   ##
=======================================
  Coverage   93.11%   93.11%           
=======================================
  Files         304      304           
  Lines       31530    31530           
  Branches    11508    11508           
=======================================
  Hits        29359    29359           
  Misses       1517     1517           
  Partials      654      654           
Files with missing lines Coverage Δ
src/review/prompt-injection.ts 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 678f394 into main Jul 5, 2026
10 checks passed
@loopover-orb
loopover-orb Bot deleted the test/prompt-injection-adversarial-fixtures branch July 5, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(review): add adversarial prompt-injection regression fixtures for the AI reviewer

1 participant