security: audit lot 1 — XSS, unauth API, shell allowlist, git tool (+ example compose) - #12
Merged
Conversation
escapeHtml() escaped only &, < and >, but its output lands in
attribute position -- inlineMarkdown built <a href="$2"> by direct
regex interpolation. `[x](" autofocus onfocus="location=name)`
therefore rendered as a valid <a> with an injected event handler and
executed with no interaction. A `javascript:` href needed no quote at
all.
This matters beyond the model's own output: the text passed to
formatContent() includes tool results -- web_fetch/research page
content, files:read, sysadmin logs -- so a hostile page reached it.
And the API token lives in localStorage, so the payoff was token
theft, then authenticated /chat calls, then whatever shell/files are
enabled.
Three layers, in order of how much each actually closes:
- escapeHtml now escapes " and ' too (kills the attribute escape)
- link rendering validates the scheme against an allowlist, and
downgrades anything else to plain text (kills javascript:)
- GET / serves a CSP; 'unsafe-inline' is still required by the
single-file UI, so this doesn't stop injected script from running
-- but connect-src 'self' stops it shipping the token off-host
The two JS fixes are covered by source assertions rather than
execution: a regression tripwire, not a proof, and honest about it in
the test module docstring. The headers are tested for real.
API_TOKEN defaulted to empty, which left /chat, /run, /review, /traces, /tools and the memory endpoints open to anyone reaching the port -- and /chat dispatches whatever is in ENABLED_TOOLS. Combined with the Containerfile's `--host 0.0.0.0`, "open by default" means arbitrary tool execution by default. config.py already carried a comment warning about this. A documented unsafe default is still an unsafe default: it's the kind you notice the day you add a published port to a compose file, not before. So the default is inverted rather than re-documented. Starting with no token now raises InsecureConfiguration from the app's lifespan, unless API_ALLOW_UNAUTHENTICATED=true records in .env.local that the open posture is intentional. The error names both ways out -- a refusal that doesn't say how to proceed just gets worked around badly. Nothing changes for the auth path itself: require_token() is untouched, and the existing test suite (bare TestClient(app), which doesn't run lifespan) still passes as-is.
The default was ls,cat,head,tail,wc,grep,find,python3,pip,pytest.
Three of those defeat the allowlist entirely, because only parts[0] is
ever checked:
python3 -c "import os; os.system(...)"
pip install <url> (setup.py runs, plus network egress)
find . -exec <anything> {} \;
An allowlist containing an interpreter is not an allowlist. The
default is now ls,cat,head,tail,wc,grep. Putting python3 back still
works and is a legitimate choice on a trusted local box -- it just has
to be written down in .env.local rather than being what you get by
default, and it now logs a warning at import naming what was switched
off.
Also corrects the module docstring, which claimed cwd=WORKSPACE_DIR
meant "relative paths cannot escape to the host filesystem". The cwd
confines nothing: `cat /etc/passwd` always worked. That sentence is
how an allowlist gets widened "safely", so it's replaced with an
explicit note that the allowlist is the only real boundary here --
and that HOME still points at the real home directory.
…dit E-3) Three things, all in a tool whose docstring said "All operations are read-only": - `stash` was on the allowlist. Bare `git stash` is `git stash push`, which removes uncommitted changes from the working tree, and `stash drop`/`clear` destroy existing stashes. Not exfiltration, but on a project developed live it costs a session. Removed. - Arguments were never inspected, only the subcommand. git has options that write files or run programs regardless of subcommand (--output, --exec, --upload-pack, --ext-diff), which turn a read-only subcommand into a write or an execution. Now refused by prefix, so both --output=x and --output x are caught. - No env was passed to subprocess.run, unlike shell.py and test.py, so the git subprocess inherited the whole API process environment -- API_TOKEN and OPENROUTER_API_KEY included. Now the same minimal env as its sibling tools, plus GIT_TERMINAL_PROMPT=0 so a repo asking for credentials fails instead of hanging until the timeout. Not addressed here: _find_git_root() still walks up from cwd, so inside the container this tool describes Forge's own /app repo rather than the workspace. That's a behaviour change with real UX consequences, so it's left for a separate decision rather than smuggled into a security fix.
…follow-up) The root cause behind an entire patch cycle serving stale UI: the deployment compose lived outside the repo, referenced `image: forge-core` with no `build:` key, so `podman compose build` answered "No services to build" and never rebuilt the image. The container kept serving frozen HTML while the source was patched. This versioned example lives at deploy/compose.example.yaml with `build: context: .` for the forge service, which only resolves correctly when read from the repo root -- the whole point of versioning it here rather than keeping a fragile relative context in an out-of-repo compose. The other three services (forge-embedding, forge-llm built separately; searxng upstream) stay image-only, and the sysadmin bind mounts are commented with a pointer to deploy/README.md so the example runs without the host proxies in place. Machine-specific paths and secrets are placeholders.
This was referenced Aug 13, 2026
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.
First batch of fixes from the security audit of
main(post-mergev3.11). Each fix is atomic and lands with its own tests. Full suite
green (406 tests), ruff check + format clean.
Fixes
C-3 — Stored XSS in the UI markdown renderer
escapeHtmlescaped only&/</>, but its output lands inattribute position (link
href, the trace/drawer templates).[x](" autofocus onfocus="location=name)therefore rendered as an<a>with an injected event handler that fired with no interaction.The text reaching the renderer includes tool output (web_fetch/research
page content, files:read, sysadmin logs), so a hostile page was enough
to reach it — and the API token lives in localStorage, so this was
token theft, not a cosmetic bug.
Three layers: escape
"/', validate link schemes against anallowlist (anything else downgraded to plain text), and a CSP on
GET /.'unsafe-inline'is still required by the single-file UI, sothe CSP does not stop injected script from running — but
connect-src 'self'stops it exfiltrating the token. Verifieddynamically against every vector (attribute escape,
javascript:/data:/vbscript:, raw HTML, inline code,diffblocks): inert everywhere, legitimate links preserved.
C-1 — API unauthenticated by default
An empty
API_TOKENleft/chat(and the tool dispatch it implies)open to anyone reaching the port, with the container bound to
0.0.0.0. The default is now inverted: the app refuses to startwithout a token unless
API_ALLOW_UNAUTHENTICATED=truerecords theopen posture in
.env.local.require_token()itself is unchanged.C-2 — Interpreters in the default shell allowlist
The default included
python3,pip,find— each runs arbitrarycommands through its own arguments, yet only
parts[0]is checked. Newdefault is interpreter-free; putting them back still works but is now a
written-down choice, with a warning logged at startup. Docstring
corrected:
cwd=WORKSPACE_DIRconfines nothing, the allowlist is theonly real boundary.
E-3 — git tool hardening
Removed
stash(mutates the working tree despite the "read-only"label), reject write/exec arguments (
--output/--exec/…), and pass aminimal env to the subprocess (it previously inherited the whole
environment,
API_TOKENincluded).Example deployment (audit follow-up)
Root cause found while validating C-3: the out-of-repo deployment
compose referenced
image: forge-corewith nobuild:key, sopodman compose buildanswered "No services to build" and thecontainer kept serving frozen HTML through the entire patch cycle.
Adds
deploy/compose.example.yaml: the full stack, versioned, withbuild: context: .for the forge service — which only resolvescorrectly when read from the repo root, the point of versioning it
here. Machine paths and secrets are placeholders; sysadmin bind mounts
are commented with a pointer to
deploy/README.md.Deferred (lots 2–3)
Non-root container, pinned dependencies, the read-only podman proxy
DoS, and — most importantly — the indirect prompt-injection hardening
(provenance delimiters in
step_context+ no tool escalation afteringesting external data). The last one should land before v4.