security: add a hidden-instruction gate and refuse PR changes to CI - #1782
Closed
DeusData wants to merge 10 commits into
Closed
security: add a hidden-instruction gate and refuse PR changes to CI#1782DeusData wants to merge 10 commits into
DeusData wants to merge 10 commits into
Conversation
Two related holes, one of which makes the other enforceable. LAYER 0 -- hidden-instruction audit. Indirect prompt injection needs a carrier, and the carrier is invisible Unicode: zero-width sequences, bidirectional overrides, the Unicode Tags block, stray byte-order marks. Those are invisible in an editor and in a diff, but a model reading the file consumes them. Published work has demonstrated agent platforms executing shell commands from instructions hidden this way inside skill markdown -- which is an artifact class this project generates and writes into auto-load directories. scripts/security-injection.py scans every tracked file (2051 files, 1.33 GB) in under seven seconds and refuses any carrier codepoint. It does NOT match on wording. The persuasion half of an injection is natural language, so it is unbounded and translatable -- refusal rates fall from roughly 79% in English to as low as 23% in some low-resource languages, and a homoglyph defeats a keyword list outright. A word list cannot gate that honestly. The carrier half is finite, language- independent, and has no legitimate use in source, so it gates with a false-positive rate near zero. This catches HIDING, not persuasion; a green result means nothing is concealed from the reviewer, which is what makes ordinary human review trustworthy. The allowlist ships EMPTY. The two zero-width spaces in php_lsp.c -- used to write a comment terminator inside a comment -- are rephrased rather than blessed, because zero entries is a materially stronger position than one: the first exception anyone adds becomes a visible event instead of joining a list. Entries pin the sha256 of a single LINE, located by content rather than by number, so unrelated edits never churn them but editing blessed text invalidates it. `--update` emits a placeholder the gate rejects, so an exception cannot be produced mechanically. CI INTEGRITY -- the circularity problem. Every other check here is defined by files inside the pull request, and `pull_request` runs the merge commit's definition. A hostile PR does not need to defeat a gate; it edits the gate, or deletes the step that calls it, and CI reports green over the change that disabled it. `ci-ok` is no help because its own `needs:` list lives in the PR too. ci-integrity.yml closes that with `pull_request_target`, so its definition comes from the base branch and the PR cannot edit its effect on itself. It never checks out or executes pull-request content -- it asks the API for the changed-path list and compares strings. Paths under .github/, scripts/, test-infrastructure/ and tests/*.sh are refused; CI changes land through break-glass instead. Makefile.cbm is deliberately not guarded: the gates are invoked directly from workflow YAML rather than through a make target, and guarding it would block nearly every feature PR. The gate is inert until `ci-integrity` is a required status check. Verified: injecting Tags-block characters fails the scan and reverting restores it; a placeholder allowlist entry is rejected; editing a blessed line invalidates it as both a fresh hit and a stale entry; the selftest covers every carrier class and confirms silence on the em dashes, box-drawing banners and CJK i18n strings this tree genuinely contains. The tripwire's path matching was checked against real pull requests, and its refusal path was run under `set -euo pipefail` after fixing an `&&` that would have aborted the script on exactly that path. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Tier 2 of the hidden-instruction gate. A generated LR parser's string
table holds grammar symbol names and punctuation terminals; an English
sentence in there is anomalous by construction. That is the check which
cleared a 353,744-line vendored parser by hand during review, and this
mechanises it.
The threshold was MEASURED rather than guessed. Across all 159 vendored
grammars: 48,271 string literals, of which 63 contain a space, because
multi-word keywords are real -- "is not", "not in", "static get". The
longest legitimate literal is three words ("hide empty description"), so
four is the tightest threshold with zero false positives, and it still
catches a four-word instruction. Current tree: 0 findings.
Deliberately NOT shipped: a tree-wide scan for hiding constructs. The
measurement did not support it. `<!--` and `<script` have 61 legitimate
uses across PR templates, docs and HTML-parsing tests, and `display:none`
is ordinary styling in docs/index.html, the project website. The real
concern is a hiding construct inside AGENT-FACING content, which is a
location question rather than a pattern question and belongs with the
Tier 3 location rule. A rule that needs excuses gets switched off, so it
is left out with the reasoning recorded at the point where it would have
gone.
Verified: injecting a six-word literal into a vendored parser's symbol
table is refused with the file, line and literal quoted; reverting
restores green.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Tier 3, plus a fix to Tier 2 that a review question exposed. THE TIER 2 GAP. Counting space-separated words is a Latin-script assumption. Chinese, Japanese and Thai write without spaces, so an injected Chinese literal counted as ONE word and passed a threshold of four. The language-agnostic companion is a run of consecutive word-forming non-ASCII characters, which measures "this is prose in some script" without needing to know which. Measured across 2,199 non-ASCII literals in the vendored grammars, the longest legitimate run is 1 -- the lone lambdas in `lean` and `fennel` -- because grammar terminals are symbols and operators, never words. Four is clean today and catches Chinese (9), Russian (10) and Arabic (9). Russian matters twice over: it scores three words, so the word rule missed it even though it does use spaces. TIER 3a -- framing tokens. ChatML turn markers, role tokens, `[INST]`, instruction headers, dialogue markers. These are PROTOCOL tokens rather than prose, so they read identically whatever human language surrounds them. Measured tree-wide: zero occurrences, so they gate with no location exemption at all. TIER 3b -- phrasing, as an explicitly SECONDARY signal. A phrase list can never be complete; an attacker switches language or substitutes homoglyphs and walks past it. It earns a place only because the location rule shrinks the surface first: this project authors agent-directed prose in exactly three files, so that shape appearing anywhere else is anomalous regardless of what it says. Location is a question no translation can dodge, which is why it carries the weight and the phrase list only adds breadth. The allowlist gains its first and only entry: the deliberate injection fixture in test_cli.c, which feeds an untrusted PROJECT NAME containing an override phrase through cbm_store_upsert_project to prove it cannot reach agent-facing output. The literal is the payload under test, so rephrasing would delete the coverage -- the canonical case where an exception is correct rather than lazy. This gate's own docstring was rephrased instead of blessed, keeping the count at one. The selftest now pins language-agnosticism directly, asserting the script-run rule catches Chinese, Russian and Arabic while staying silent on a lone lambda. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
scripts/security-injection.py claims it catches HIDING rather than persuasion. That belonged in a measurement rather than a docstring. benchmark-injection-detection.py runs public corpora through the LIVE detectors -- imported from the gate, not reimplemented, because a benchmark that reimplements what it measures grades its own copy, and this repo was recently bitten by a regression test that hand-rolled the production SQL it was meant to guard. It is NON-GATING and refuses to touch the network implicitly: --fetch or --corpus, never by default, so no third-party download sits in the release path. Against 435 labelled injections from deepset/prompt-injections and rikka-snow/prompt-injection-multilingual: visible prose (a README, an issue body) 2.5% same text inside a parser symbol table 99.1% Same payload, different location, opposite verdict. That is the design stated as a number: near-total against smuggling, near-zero against plain persuasion. A low first figure is the honest confirmation, not a failure -- a HIGH one would mean the gate leans on a phrase list, which translation and paraphrase defeat. The first run earned its keep immediately. The top miss was "Forget all previous TASKS", which slipped through because the pattern matched only instructions/prompts/rules/directives. Widening the noun set from corpus evidence moved the prose rate 2.1% -> 2.5%, and that four-tenths of a point is itself the argument: chasing the phrase list buys almost nothing, which is why the structural layers carry the weight. An earlier draft of this benchmark reported 99.1% as a headline by applying the parser-scoped rule to free prose. Any English sentence has four words, so it matched nearly everything and measured nothing. The two threat models are now reported separately for that reason. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Follows the benchmark's advice rather than my own prediction. I expected translation to buy almost nothing, on the evidence that widening one English noun set moved the prose rate 2.1% -> 2.5%. Measured, a proper intent table moves it 2.5% -> 10.1%, with the phrase layer itself going 1.6% -> 9.2%. The earlier guess was wrong and the corpus said so. Structure: verb + SCOPE QUALIFIER + noun, all three inside a short window, written as a reviewable table so a native speaker can check one row without parsing the whole thing. The qualifier is load-bearing. An earlier draft required only verb + noun and immediately fired on sqlite3.c's `int ignoreJump /* Instruction to jump to ... */` and on this repo's own ".gitignore AS ignore rules" -- a verb beside a noun is ordinary technical English, while a verb beside ALL PREVIOUS plus a noun is an attack. Both orderings are matched. SVO languages lead with the verb; Japanese, Korean, Turkish and Hindi are verb-final, so matching only verb-first silently excluded every SOV language. That was found by testing Japanese and Korean, not by reasoning about it -- the same class of mistake as assuming spaces separate words. Scope narrowed twice on evidence, both times preferring a smaller rule to a blessed exception: the phrase layer no longer runs on vendored third-party source, and the gate stopped detecting its own table after the label and two comments were reworded rather than allowlisted. Performance: the four independent tier walks became one shared pass, and the override table only runs when a cheap substring pre-filter finds a verb fragment. Full tree in ~21s (was over ten minutes at the worst point, 6.8s before tiers 2 and 3 existed). HONEST LIMITS, recorded in the table itself: the translations are not native-speaker reviewed; only the CJK rows are measurable against the corpora we benchmark on; twenty languages is breadth of evidence, not coverage. At 10.1% against visible prose this remains the layer that does NOT carry the weight -- the carrier scan, script runs, framing tokens and the location rule do. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Every change here came from trying to defeat the gate rather than from reasoning about it. Four of six attack shapes got through the first attempt. HOMOGLYPHS. Cyrillic o and Latin o are visually identical, so a substituted word reads normally and misses every ASCII pattern. NFKC does NOT fold this -- confusable mapping is separate. Detected instead as a SINGLE TOKEN drawing letters from two confusable alphabets, which essentially never happens deliberately. Greek is excluded on measurement: the Sigma-prefixed accumulators in src/semantic/rotsq.h legitimately mix Greek with Latin, and Latin/Cyrillic is the pair that actually carries spoofing risk. C escape sequences are stripped before tokenising, without which every Russian test fixture reported a false positive. LETTER SPACING. A phrase written one space-separated character at a time defeats all pattern matching while staying readable. Six consecutive single letters is comfortably above prose; the only matches in this tree were format-character tables in vendored sqlite3 and yyjson, which this layer does not scan. COMPATIBILITY FORMS. Fullwidth letters and mathematical alphanumerics render as readable Latin while sharing no code points with it. These were "caught" only by the prose-shape rule, which is scoped to parser literals -- so in a README they were a complete bypass. NFKC now folds them before phrase matching. Line numbers stay aligned because NFKC never adds or removes newlines. That third one was found by a selftest failure rather than by inspection. The helper had applied parser-scoped rules tree-wide, claiming coverage the gate did not have; correcting the helper exposed the real hole. The eight attack shapes and four benign shapes are now pinned in the selftest, so a later simplification cannot quietly reopen one. The benign half matters as much: Greek math identifiers, sqlite3-style `ignoreJump /* Instruction */`, ordinary CJK UI strings and this repo's own ".gitignore AS ignore rules" must all stay silent, because a gate that flags them becomes noise and gets switched off. The fixtures are assembled from fragments at runtime. Written literally they trip this gate when it scans its own source, and the wrong fix is to exclude the file -- that would put a blind spot in precisely the place an attacker would aim. Assembling keeps the file fully scannable. Full tree in ~23s, allowlist still at one entry. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Attacked the inputs rather than the patterns this time. Three worked.
ONE INVALID BYTE HID A WHOLE FILE. The scanner abandoned any file that
failed to decode as UTF-8, so appending a single 0xFF made it skip every
readable line in that file, plaintext payload included. A complete
evasion costing one byte. Files are now decoded with replacement rather
than abandoned, and a NUL byte -- git's own binary heuristic -- is what
marks a file as genuinely not a review surface. Only three tracked files
reach that path today: a PNG, a Windows ETW manifest and the nomic blob.
Better still, the evasion is now its own signal: a file with a TEXT
extension that is not valid UTF-8 is reported, because a stray byte in a
.md or .c is anomalous regardless of what surrounds it.
THE TRIPWIRE MATCHED CASE-SENSITIVELY. `Scripts/evil.sh` and
`.GitHub/workflows/` walked past it, and on a case-insensitive checkout
those are the same files as the guarded ones. Now matched with POSIX
character classes rather than `${v,,}` (bash 4 only) or `tr` (external),
so the logic can be tested on any shell -- which matters for a gate
nobody can run locally the way CI runs it.
THE FILES ENDPOINT CAPS AT 3000. A pull request padded past that limit
would hide a CI change in the tail, and the gate would report green over
a change set it never saw. It now compares what the API returned against
the count the PR itself declares and REFUSES when they disagree, rather
than passing on partial data.
All three are pinned in the selftest, alongside the requirement that a
PNG with invalid UTF-8 stays silent -- the hardening must not turn every
binary into a finding.
Verified end to end against real pull requests: #1422 refused (4 guarded
paths), #1245 refused (42 guarded, 392/392 received so no truncation),
#1778 refused (2 guarded), #1678 passes.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The file gate reads `git ls-files`, so a pull request's title, body and commit messages were never scanned by anything. That is the surface an agent reads FIRST, and it is entirely attacker-controlled -- our own #1782 carries 5,249 characters of body text that no check had ever seen. Metadata gets a STRICTER ruleset than repository files and can afford one. A description legitimately contains prose, code fences and checklists; none of it needs concealment. So on top of every tree-wide detector, metadata also refuses collapsed sections, embedded markup, event-handler images, javascript/data URIs, and comment syntax that renders invisible on the pull-request page while remaining in the raw text a model consumes. Measured against 120 real pull requests from this repository: ZERO findings, for every rule. Gating costs nothing a contributor actually does. Against realistic attacks it catches all six shapes tried -- hidden comment, zero-width carrier, Cyrillic homoglyph, chat-template framing, collapsed section, and plain override phrasing. There is deliberately NO allowlist for metadata. Rewording a sentence costs nothing, so an exception mechanism would only ever be used to avoid rewording. It runs from the BASE branch. `pull_request_target` resolves `actions/checkout` to the base commit, so the scanner executed is main's copy -- a scanner shipped by the pull request could edit itself into approving anything. Pull-request content arrives strictly as data: the metadata is written to a file and passed by path, never expanded by the shell, and the token stays read-only with no secrets in scope. What would make this pattern dangerous is a `ref:` pointing at the pull request head, or a build step; neither exists here and the header says so. The layering is what makes it hold: the path tripwire refuses any pull request touching .github or scripts, so the step that runs this scanner cannot be deleted by the pull request it judges. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The gate covered files and pull-request metadata. Comments, review
comments, reviews and discussions -- the surfaces a triage agent reads
most -- were covered by nothing. On one pull request alone that is 30
commit messages scanned and three comment threads not.
`--content FILE|-` scans arbitrary text with the full ruleset, reading
stdin so a caller can pipe whatever it is about to hand a model:
gh api ".../issues/N/comments" --jq '.[].body' \\
| scripts/security-injection.py --content - --json
`--json` emits a structured verdict for a workflow to branch on.
REDACTION IS ON BY DEFAULT, and it is the reason this is not just a
convenience wrapper. The report is meant to be fed to an agent, so
echoing the matched text back would deliver the very injection the scan
just intercepted -- the scanner would become the delivery mechanism.
Redacted findings carry the rule that fired, the line and a short digest
for correlation, never the content. `--show-payload` opts into the
literal for a human. The selftest pins both directions: a sentinel word
must not survive into a redacted report, and must survive into a shown
one.
Verified against real threads: comments on three pull requests and one
issue, 47,838 characters, all clean.
This does NOT make autonomous workflows on community content safe. It
catches concealment, so a reviewer sees what is there; it does not catch
persuasion, and an autonomous agent IS the thing being persuaded. Treat
a clean verdict as "nothing is hidden", never as "this is safe to obey"
-- which is why the report carries that sentence in its own guidance
field rather than leaving it to a reader to remember.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Restructured after concluding the architecture was wrong, not the rules. The CI gate protected the REPOSITORY. The threat for autonomous work is an agent reading attacker-controlled text and being persuaded by it -- and there, the agent is the target, so enforcement belongs at the point of consumption rather than at a checkpoint. A checkpoint also only ever saw content arriving through a pull request to us; the read path covers every issue, comment and discussion an agent touches anywhere. REMOVED: ci-integrity.yml. With enforcement at the read path, a pull request disabling its own checks matters far less, and that workflow was carrying the entire cost -- pull_request_target, a required-check registration, a branch-protection dependency, and break-glass friction on every CI or tooling contribution. It was also the one piece that could not be tested the way CI actually runs it. KEPT: the Layer 0 file scan, gating. It is 23 seconds with no false positives and it stops a payload LANDING in main, where it would be read by every agent afterwards and indexed into the product's own graph. The read-path scanner cannot substitute for that: by the time main is being read, the payload is already shipped. Prevention and detection, not one instead of the other. ADDED: scripts/fetch-scanned.sh, which pulls every agent-readable surface of an issue or pull request -- title, body, comments, reviews, inline review comments, commit messages -- scans the lot, and prints it only if nothing is concealed. Five API calls become one command, so the safe path is also the shorter path. That is the only kind of safety measure that gets used. SEVERITY, added because the binary model was unusable. An HTML comment is a hiding MECHANISM, not an attack SIGNATURE, and our own acknowledgement bot posts one into nearly every thread -- refusing on it rejected 28 of 40 recent pull requests. A tool that refuses 70% of real content is switched off within a day. Signatures now refuse; mechanisms are noted. A payload hidden INSIDE a downgraded comment still refuses, because the phrase rules fire on the text wherever it sits, and the selftest pins exactly that. Re-measured end to end across 40 recent pull requests: 1 refused, 28 noted, 11 clean. The single refusal is this pull request, whose commit messages document the markers the scanner looks for. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Owner
Author
|
Closing — rescoped. This solved the problem in the wrong place. The gate here protected the repository; the threat it was built for is an agent reading attacker-controlled text and being persuaded by it, and there the agent is the target. Enforcement belongs at the point of consumption, in the local harness (Claude Code / Codex), not in this project's CI — and a checkpoint here only ever saw content arriving through a PR to this repo, while an agent reads issues, comments and discussions everywhere. Nothing is lost; the work moves to the harness. Recorded here so the reasoning is findable:
No branch protection changes are needed after all — the required-check and required-review asks are withdrawn along with the tripwire. |
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
1 participant
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.
Two related holes. The second is what makes the first enforceable.
What this refuses
Layer 0, on every tracked file — hidden-instruction carriers: zero-width sequences, bidirectional overrides, the Unicode Tags block, stray BOMs. Plus obfuscation that defeats pattern matching: Cyrillic homoglyphs, letter-spacing, fullwidth and mathematical-bold compatibility forms. Plus prose smuggled into a generated parser's symbol table. Whole tree, ~23s.
Pull-request metadata — title, body and commit messages, which nothing scanned before. Stricter ruleset than files: also refuses collapsed sections, embedded markup, event-handler images,
javascript:/data:URIs, and comment syntax invisible on the rendered page.CI-defining paths — any pull request touching
.github/,scripts/,test-infrastructure/ortests/*.sh.Why patterns, not a wordlist
The persuasion half of an injection is natural language: unbounded, translatable, homoglyph-substitutable. Published refusal rates fall from ~79% in English to ~23% in some low-resource languages, so a keyword list is structurally inadequate rather than merely noisy.
The carrier half is finite, language-independent, and has no legitimate use in source. That gates at a false-positive rate near zero.
So this catches hiding, not persuasion. A green result means nothing is concealed from the reviewer — which is what makes human review trustworthy. It is not "no injection", and the script's own header says so. Measured: 10.1% against visible prose, 99.1% against the same text smuggled into a parser symbol table. Same payload, different location, opposite verdict.
Why it cannot be switched off by a pull request
Every other check here is defined by files inside the pull request, and
pull_requestruns the merge commit's definition. A hostile PR doesn't defeat a gate — it edits the gate, or deletes the step calling it.ci-okis no help; its ownneeds:list lives in the PR.ci-integrity.ymlruns underpull_request_target, so its definition comes from the base branch and the PR cannot edit its effect on itself.actions/checkoutresolves to the base commit, so the scanner executed is main's copy. Pull-request content arrives strictly as data: paths are matched withcase, metadata is written to a file and passed by path, never expanded by the shell. Read-only token, no secrets in scope, no build step, noref:at the PR head — the two changes that would turn this into a pwn-request, called out in the header so nobody adds them later.Seven bypasses found by attacking it
Each of these worked against an earlier version and is now pinned in the selftest:
Two were found only because a test was wrong rather than the code — the selftest applied parser-scoped rules tree-wide and claimed coverage the gate didn't have.
False-positive discipline
The allowlist has one entry: the deliberate injection fixture in
test_cli.c, where the literal is the payload under test. Everything else was fixed by rewording rather than exempting — including two zero-width spaces inphp_lsp.cand several of this gate's own comments.Entries pin the sha256 of a single line, located by content rather than number, so unrelated edits never churn them while editing blessed text invalidates it.
--updateemits a placeholder the gate rejects, so an exception cannot be produced mechanically.Every rule was measured before gating. Rules that couldn't be made clean were dropped, not loosened: tree-wide hiding-construct detection (61 legitimate uses) and the phrase layer on vendored source (it fired on sqlite3's
int ignoreJump /* Instruction ... */).This pull request refuses itself, twice
It touches
.github/andscripts/, so the path tripwire refuses it. And two commit messages document tokens the metadata scanner detects, so that refuses it too.Both are correct, and this lands via break-glass — which is exactly the path the gate is designed to force for CI changes. Rewriting the history to dodge it was considered and rejected: the friction is the feature.
Not done here — needs a maintainer decision
The tripwire is inert until
ci-integrityis a required status check. Related:required_pull_request_reviewsis currentlynone, somain's entire protection isdco+ci-ok— both defined in files a pull request can rewrite. Enabling required review with code-owner approval on.github/andscripts/is what turns these from suggestions into controls.scripts/benchmark-injection-detection.pyis non-gating and never touches the network implicitly — it exists so the catch rate stays a measurement rather than a claim.