Security audit, batch 3: prompt injection and honest boundaries - #15
Merged
Conversation
… E-2)
A tool result is appended to step_context and re-injected into the
prompt that decides which tool to call NEXT (orchestrator.py). So the
text of a web page (web_fetch, research), a file (files:read) or a
system log (sysadmin) reaches the router's decision surface. A page
containing a plausible {"tool":"files","content":{"action":"write"...}}
has a real chance of being followed.
Each entry is now wrapped in explicit provenance markers, with a
header stating that everything between them is quoted data and that
only the user's message decides the next tool.
The markers are stripped out of the tool output before it is
inserted. Without that they are decorative: a page that simply
contains the closing marker ends the untrusted block early, and
everything it writes after that reads as Forge's own instructions.
The steering hints (files-read, web_search) stay outside the block
for the same reason in reverse -- they are Forge's instructions and
must not be labelled untrusted by the framing meant to protect them.
This is the cheap half of E-2 and it is only a nudge. Prompt wording
has already failed three separate times on this project (the
web_search saga in this same file, review's JSON envelope, memory's
recall hint), so nothing rests on it. The half that holds is the
deterministic escalation guard in the next commit.
… E-2) The other half of E-2, and the half that doesn't depend on the model cooperating. Once a step has dispatched web_fetch, web_search, research or sysadmin, no later step of the same run may dispatch a mutating tool -- shell, test, or files with anything but a read/list action. Checked before dispatch, so the tool does not run; the run ends with a refusal naming which tool brought the outside data in. Why deterministic rather than a prompt rule: this project has three separate recorded cases of prompt wording failing to steer this model (the web_search chaining saga, review's JSON envelope, memory's recall hint). A rule in orchestrator.py cannot be talked out of by the page it is protecting against. files:read is deliberately not treated as ingest. It reads inside WORKSPACE_DIR, and read-then-write is the one legitimate multi-step flow this project actually uses (v3.9: "remplace X par Y dans hello.go"). Tainting it would break that real flow to defend against the user's own workspace. The residual path -- hostile content written in one turn, read back in another -- is a real limit of a per-run taint, stated in the next commit's SECURITY.md rather than papered over here. _is_mutating() fails closed on files: anything not demonstrably a read or a list counts as a write. files.py's own parser is more forgiving than this check, and the gap between the two parsers is exactly where an escalation would live. Inert at the default MAX_STEPS=1, where there is never an earlier step. ALLOW_MUTATION_AFTER_EXTERNAL_DATA=true switches it off for anyone who has a genuine fetch-then-write flow, logging a warning at import like shell.py's allowlist tripwire.
The docstring called this tool "Sandboxed" and listed a working directory as the layer that stops commands escaping to the host -- the same claim shell.py's docstring already retracted about itself, still standing here. Reading it that way is how an allowlist gets widened "safely". What is now stated instead: running pytest is executing the Python code in the workspace. That is what a test runner is. `pytest test_x.py` imports test_x.py and runs whatever sits at module level, and pytest auto-loads conftest.py from the rootdir before collecting anything, so even an invocation naming one innocuous file executes a conftest.py placed next to it. `files` + `test` is therefore equivalent to `shell` regardless of SHELL_ALLOWED_COMMANDS. The allowlist here restricts which binary starts, not what that binary then executes. Making that genuinely safe needs a disposable container per run, which this process cannot do today. So: say it plainly, and log a warning at import when both tools are enabled -- same idiom and same reasoning as shell.py's allowlist tripwire, a configuration fact belongs in the startup log rather than in every run's output. One real bound added alongside the honesty, since it was cheap: arguments that point outside WORKSPACE_DIR (absolute, or climbing out with "..") are refused before anything runs, so this tool can't lint /etc or collect tests from the host. Flags are skipped so "--tb=short" still works, but a flag's VALUE is checked -- `pytest -p /tmp/plugin` is exactly the shape this catches. Where the executed code may come from is now bounded; that it executes is not, and the docstring says so rather than implying the check is more than it is. Unlike files.py's _safe_path, an absolute path is rejected rather than reinterpreted as workspace-relative: there the router genuinely emits "/hello.go" meaning the workspace file, here "ruff check /etc" is most likely exactly what it looks like, and rewriting it silently would turn a refusal into a surprise.
The audit's model was "single user, WireGuard exposure, public repo", and every judgement in it -- what counted as critical, what was acceptable, what was out of scope -- rested on that. It lived in the report and nowhere in the repository, so anyone reading the code had to reconstruct it from the code. Writes it down: what Forge is built to resist (an unauthenticated caller on the port, a hostile web page or log line reaching the prompt that picks the next tool, the model itself being wrong), what it deliberately is not (a malicious operator, a compromised host, supply-chain auditing beyond pinning, denial of service), and the boundaries actually enforced in code with a pointer to each one. The limits section is the part with a shelf life. It states plainly that `files` + `test` is equivalent to `shell`, that the escalation guard is per-run so content written in one turn and read back in another is not covered, that files:read deliberately doesn't taint, that the provenance markers are a nudge and not a guarantee, and that MAX_STEPS=1 is doing real work at the default. A limit written down is a decision; the same limit undocumented is a surprise. Timed before the Evolution Runtime rather than after: a system that finds and fixes its own faults is by construction one where external data can influence code that gets written, and that is a much harder document to write once the machinery exists.
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.
Closes the audit. Follow-up to #12 (batch 1) and #13 (batch 2).
Four commits on top of
main(091b757). 514 tests green (478before, +36),
ruff checkandruff format --checkclean. Verifiedby
git amon a pristine clone ofmain.This batch is the one the audit called architectural and least
urgent — and the most expensive to add later, once an Evolution
Runtime is writing code on its own.
E-2 — indirect prompt injection
A tool result is appended to
step_contextand re-injected into theprompt that decides which tool to call next. So the text of a
fetched web page, a search result, or a system log reaches the
router's decision surface. A page containing a plausible
{"tool":"files","content":{"action":"write",...},"done":false}hada real chance of being followed.
Fixed in two halves, of unequal weight.
Provenance markers (
router/prompt.py). Each entry is wrapped inexplicit delimiters, with a header stating the block is quoted data
and that only the user's message decides the next tool. The markers
are stripped out of the tool output before insertion — without that
they are decorative, since a page containing the closing marker ends
the block early and everything after it reads as Forge's own
instructions. The steering hints stay outside the block for the
symmetric reason.
Escalation guard (
orchestrator.py). Once a step has dispatchedweb_fetch,web_search,researchorsysadmin, no later step ofthe same run may dispatch
shell,test, orfileswith anythingbut
read/list. Checked before dispatch, so the tool never runs;the run ends with a refusal naming which tool brought the outside
data in.
The second half is the one that matters. Prompt wording has failed to
steer this model three separate recorded times on this project (the
web_searchchaining saga,review's JSON envelope,memory'srecall hint), so nothing here rests on the first.
Two deliberate calls:
files:readdoes not taint. Read-then-write is the one legitimatemulti-step flow this project actually uses (v3.9). Tainting it
would break a real flow to defend against the operator's own
workspace.
_is_mutating()fails closed onfiles: anything not demonstrablya read or a list counts as a write.
files.py's own parser is moreforgiving than this check, and the gap between two parsers is
exactly where an escalation would live.
Inert at the default
MAX_STEPS=1, where there is never an earlierstep.
ALLOW_MUTATION_AFTER_EXTERNAL_DATA=trueswitches it off foranyone with a genuine fetch-then-write flow, warning at import like
shell.py's allowlist tripwire.E-1 —
files+testisshellThe docstring called this tool "Sandboxed" and listed a working
directory as what stops commands escaping to the host — the same
claim
shell.pyalready retracted about itself, still standing here.Running pytest means executing the Python code in the workspace. That
is what a test runner is.
pytest test_x.pyruns whatever sits atmodule level, and pytest auto-loads
conftest.pybefore collectinganything, so even an invocation naming one innocuous file executes a
conftest.pynext to it. Write a file, then run it.No code fixes this without a disposable container per run, which this
process cannot do today. So it is stated plainly instead, and a
warning is logged at import when both tools are enabled.
One real bound added alongside, since it was cheap: arguments
pointing outside
WORKSPACE_DIRare refused before anything runs, sothe tool can't lint
/etcor collect tests from the host. Flags areskipped so
--tb=shortstill works; a flag's value is checked,which is what catches
pytest -p /tmp/plugin. This bounds where theexecuted code may come from, not that it executes — and the docstring
says so rather than implying the check is more than it is.
Unlike
files.py's_safe_path, an absolute path is rejected ratherthan reinterpreted: there the router genuinely emits
/hello.gomeaning the workspace file, here
ruff check /etcis most likelyexactly what it looks like.
F-4 —
SECURITY.mdThe audit's threat model — one operator, one machine, WireGuard
exposure, public repo — is what every judgement in it rested on, and
it lived in the report and nowhere in the repository. Now written
down, along with what is enforced in code versus asked of the model,
and a limits section that states the residual gaps rather than
leaving them to be discovered: the per-run scope of the escalation
guard, the
files+testequivalence, that the markers are a nudge,and that
MAX_STEPS=1is doing real work at the default.Testing
tests/test_prompt_provenance.py(7) andtests/test_orchestrator_escalation.py(23) are new; the latterincludes the case that the read-then-write flow is untouched and that
the taint does not survive into the next run.
tests/test_test_tool.pygains 7 for argument confinement and the tripwire.
Verified live in the deployed container, not only in CI:
dispatch path (
web_fetch→files:write,MAX_STEPS=2): thefetch runs, the write is refused before dispatch,
pwn.txtisnever created. Stubbing the router rather than prompting for it is
deliberate — this model does not chain steps reliably, which is why
research/recall/sysadminare deterministic graphs, so noprompt reaches the second step dependably. The guard is a backstop
for a model or a
MAX_STEPSthat does, and it has to be verifiedas one.
files+testtripwire, in the startup log with both toolsenabled.
Two pre-existing bugs surfaced while testing this branch and are
not caused by it — both reproduce on
main, tracked separately:_all_json_objectsinrouter/parser.pycounts braces withouttracking string context, so a valid router object whose
contentholds file text with unbalanced braces (Go, C, Rust, JS, a nested
dict) is discarded and the run falls through to the plain-text
chat fallback.
hello.pyworks only becauseprint('...')has nobraces.
escaping, which this model does not hold across multi-line file
content — the inner payload fails with an invalid control
character even when the outer object parses.
Together they mean
files:writethrough the router fails for mostreal file content. Fixed on a separate branch.
Not in this batch
The two
router/parser.pyfaults described above — brace countingthat ignores string context, and the double-escaping a nested JSON
payload requires — are their own branch. They break
files:writethrough the router for most real file content, and they predate this
work.
The four
sysadminhonesty defects found during batch 2'spre-merge testing (silent
journalctl -kfallback, unfiltered.deviceunits bloating the prompt,sysadmin/researchroutingambiguity, confident-and-wrong synthesis with no "say so if the logs
don't answer" instruction) are separate work, and higher value than
anything left in the audit.