-
Notifications
You must be signed in to change notification settings - Fork 162
fix(ui): stop any web page from driving the Agent UI backend - #3366
fix(ui): stop any web page from driving the Agent UI backend #3366kovtcharov wants to merge 4 commits into
Conversation
Visiting a page was enough to control the local Agent UI. Its backend binds loopback with no auth while the tunnel is off, and 41 of its 92 mutating routes took no JSON body -- so a plain cross-origin form POST reached them. A body-less POST to /api/tunnel/start returned 200 and published the machine to the internet; /v1/email/send, the document uploader (persistent prompt injection into the user's RAG) and the memory admin routes were reachable the same way. The X-Gaia-UI CSRF guard existed but was opt-in per route: four copies of the same function, applied to 24 decorators. A route author had to remember it, and most did not. It is now one middleware over every non-GET /api|/v1 request, so a route added tomorrow is covered on the day it lands, plus a route-table test that fails if a mutating route ever ships outside the guarded prefixes. The header alone was not enough. CORS trusted every *.ngrok-free.app and *.use.devtunnels.ms origin with credentials and allow_headers=["*"], and those are self-service namespaces -- anyone can rent a subdomain, and their preflight was approved *including* X-Gaia-UI, which is the entire basis of the guard. It was also dead config: the tunnel GAIA starts issues *.ngrok-free.dev, and the mobile flow serves the SPA from the tunnel origin, so those requests are same-origin and CORS never applies. Deleted. Two more layers behind it: a mutating request whose Origin is neither this server, loopback, nor the live tunnel is refused, so a future CORS mistake is not sufficient on its own; and a request whose Host is a name the server does not know is refused, which closes DNS rebinding against the read side (GET /api/files/preview). IP literals still pass, so --host 0.0.0.0 LAN access is unaffected. Every non-browser client was inventoried and updated to send the header: the React renderer (now in apiFetch, so no call site can forget, plus the three raw fetch sites that bypass it), gaia mcp's agent_ui_mcp server, and the two eval harness callers. The Electron main process already sent it. The Go TUI does not talk to this backend at all -- it reaches the daemon and Lemonade on their own ports. Breaking for third-party /v1/email integrators: those routes now require X-Gaia-UI: 1 like the rest. docs/guides/email-integration.mdx is updated and the curl examples carry it. Leaving them open was not an option -- "send mail as the user" was the most alarming entry on the forgeable list.
Running the UI suites with the unit conftest's Windows-only socket guard lifted -- the shape CI actually runs on Linux -- surfaced three things the local run could not see. Four hub-router tests assert the guard REFUSES an unheadered request. They share a client fixture with the positive tests, so the conftest default made them pass for the wrong reason. They now use a fixture that opts out, which is the only way a negative test here stays honest. A tunnel flagged active before its URL is minted returned a non-string from get_status(), which the Host check fed to urlsplit and turned every request into a 500. Guarded on type. The guard's own tests fired at /api/memory/prune and /api/tunnel/start. Those were safe only while the guard worked -- a regression would have pruned the developer's real memory DB or opened a public tunnel from a unit test. They now fire at a route that writes only to the in-memory database, and the route table pins coverage of the dangerous ones.
Verdict: Approve with suggestions.
This closes a real hole. Before it, any page the user happened to have open could drive their local Agent UI backend — start a public tunnel, index attacker content into their RAG, send mail as them. The fix moves the CSRF check out of per-route decorators (applied to 24 of 92 mutating routes) into one middleware, deletes a CORS rule that trusted every rented *.ngrok-free.app subdomain, and puts Host and Origin checks behind it. The design is sound and the evidence backs it up; two things are worth cleaning up.
Worth fixing:
- The Agent UI stress suite will now 403 on every write. The description says every non-browser client was inventoried before landing, but the stress test still talks to the backend without the new header, so its session-create, chat-send and delete calls all fail. One line.
- The test that guarantees no route escapes the guard doesn't run on the oldest FastAPI the project supports. It reaches into FastAPI internals that only exist in recent releases, so on the declared minimum it errors out instead of checking anything. That test is the only thing standing between a future route and an unguarded one, so it should work everywhere the project claims to run.
Nit: the 400 a user gets when reaching the UI by machine name instead of IP doesn't mention the setting that fixes it.
Real-world evidence
No evidence-bundle.md in the tree, but the PR description carries HTTP-API evidence matched to the surface — real curl runs against a live gaia.ui.server on a scratch port, before and after:
BEFORE (main @ abe87edc)
OPTIONS /api/memory/prune Origin: https://someone-else.ngrok-free.app
-> 200, access-control-allow-headers: x-gaia-ui,content-type (the CSRF header, approved)
POST /api/memory/prune Origin: https://evil.example, text/plain
-> 200 {"tool_history_deleted":0,...}
GET /api/health Host: evil.example -> 200
AFTER
OPTIONS (same) -> 400, no access-control-allow-origin
POST (same) -> 403 {"detail": "Cross-origin request rejected"}
POST Origin: https://evil.example + X-Gaia-UI: 1 -> 403 (a forged header is not enough)
GET Host: evil.example -> 400 {"detail": "Invalid Host header"}
Each first-party client shape was re-checked against the same live server:
SPA Origin http://127.0.0.1:4277 + header POST /api/memory/prune -> 200
Electron Origin null (file://) + header POST /api/memory/prune -> 200
gaia mcp no Origin + header POST /api/memory/prune -> 200
vite dev Origin http://localhost:5174 OPTIONS preflight -> 200
EventSource GET, no header GET /api/health -> 200
LAN access Host 192.168.1.9:4277 GET /api/health -> 200
The Agent UI screenshot is marked N/A with a reason (no UI change), which is fair — the SPA's behaviour is proven by the rows above rather than pixels. My verdict rests on this plus static review; the stress-suite breakage is a static finding the evidence run wouldn't have surfaced, since that suite needs a live model backend.
🔍 Technical details
🟡 Important
1. tests/stress/test_agent_ui_stress.py:1359 — every mutating call in the stress suite now gets 403
The suite drives a live backend on :4200 with httpx.AsyncClient() and no default headers: create_session (POST /api/sessions), send_message_streaming / send_message_nonstreaming (POST /api/chat/send), delete_session (DELETE /api/sessions/{id}). All are /api/* mutations, so UIRequestGuardMiddleware refuses them. It's not in the unit CI lane, which is why it survived the test plan — but the PR claims the client inventory is complete, and this is the one that got missed.
async with httpx.AsyncClient(headers={"X-Gaia-UI": "1"}) as client:
I checked the rest of the inventory and it holds up: memoryApi.ts:13 and CustomAgentsSection.tsx:101,169 already sent the header, api.ts:666 (/chat/attach) is a GET, electron/src/services/mcp-client.js targets the MCP bridge on :8765 not this backend, and tests/integration/test_scheduler_e2e.py builds a bare FastAPI() without the middleware. Only the stress suite is affected.
2. tests/unit/chat/ui/test_ui_request_guard.py:115 — the route-coverage walk binds to FastAPI private internals
from fastapi.routing import _EffectiveRouteContext, _IncludedRouter are undocumented names that only exist in recent FastAPI. setup.py:160 declares fastapi>=0.115.0, where they don't — so _all_api_routes() raises ImportError and the four tests that depend on it (test_route_walk_finds_the_whole_surface, test_every_mutating_route_is_covered_by_the_guard, test_no_mutating_route_is_exempted, test_high_value_routes_are_in_the_covered_set) error rather than run. CI installs the latest, so it's green there.
Those four are the durable half of this PR — the part that catches a mutating route mounted outside GUARDED_PREFIXES next quarter. Suggest a fallback so the walk degrades to the plain app.routes shape on older FastAPI (test_route_walk_finds_the_whole_surface already fails loudly if the walk under-collects, so a fallback can't silently pass), or raise the fastapi floor in setup.py to the first release that has these names.
🟢 Minor
3. src/gaia/ui/security.py:235 — the Host rejection doesn't name its own escape hatch
--host 0.0.0.0 LAN access by hostname (http://my-pc.local:4200, http://workstation:4200) now 400s on the HTML itself, and the body gives the user nothing to act on. GAIA_UI_ALLOWED_HOSTS is documented in docs/spec/agent-ui-server.mdx, but nobody hitting this reads the spec first. Per CLAUDE.md, an actionable error names what to do next:
await _send_rejection(
send,
400,
f"Invalid Host header. Add this hostname to {ENV_ALLOWED_HOSTS} "
"(comma-separated) if you reach the Agent UI by name.",
)
4. src/gaia/ui/security.py:99 — _hostname() mangles a bracket-less IPv6 authority
_hostname("::1") returns ":" (the rsplit(":", 1) branch), so bare-IPv6 Host/Origin values fail the loopback check and the "::1" entry in _LOOPBACK_NAMES is only ever reached via the bracketed path. Brackets are required by RFC 3986/7230 so nothing correct sends this today, but a hostname.count(":") > 1 early-return would remove the trap.
Strengths
- The fix is structural, not a patch. Moving the guard from 4 duplicated decorator helpers to one outermost ASGI middleware means the failure mode ("author forgot the decorator") can't recur, and
test_every_mutating_route_is_covered_by_the_guardturns that into a build failure rather than a convention. Pure ASGI instead ofBaseHTTPMiddlewareis also the right call in front of the SSE routes. - Defence in depth with the reasoning written down.
security.py's module docstring explains whyOrigin: nulland absentOriginare accepted (Electronfile://, non-browser clients) and why that isn't a hole — layer 1 stops the cross-site case and CORS refuses a null-origin preflight. That's the kind of comment that survives a refactor. - Empty
CSRF_EXEMPT_PATHSasserted by a test, with the one flow that would have needed an entry (the OAuth loopback callback) traced to its own aiohttp server ingaia.connectors.flow. That's the detail most PRs like this get wrong. - The negative-control run — the same 25 tests against unfixed
server.pyproducing 7 failures — is what makes them regression tests rather than assertions that pass either way.
The client fixture rename in the previous commit pushed that call past the line limit.
Review found the durable half of this change was inert on the oldest
supported FastAPI. The route walk imported _IncludedRouter and
_EffectiveRouteContext, which only exist in recent releases, so on the
declared floor (fastapi>=0.115.0) it raised ImportError and the four
tests that guarantee no mutating route escapes the guard errored instead
of checking anything. A guard that does not execute is not a guard.
The walk now probes for those shapes by attribute name instead of
importing them, so it materialises lazy includes on new FastAPI and
falls through to the flat app.routes list on old. Verified on both:
30 passed on 0.115.0 and on 0.141.1, and with a mutating route
temporarily mounted outside the guarded prefixes the coverage test
fails on both.
Added a cross-check against app.openapi(), which is public and stable on
every supported version: every mutating route the schema knows about
must appear in the walk. It caught a real gap while being written -- the
walk keeps FastAPI's {agent_id:path} convertor where openapi() reports
{agent_id} -- so the comparison normalises convertors.
Also from review:
The Agent UI stress suite drives a live backend through one httpx client
with no default headers, so its session-create, chat-send and delete
calls would all have 403'd. It sends the header now. My original client
inventory swept src/, tui/ and the unit and integration test trees but
never looked at tests/stress/, which is why it was missed.
A rejected Host now names GAIA_UI_ALLOWED_HOSTS and echoes the hostname,
so someone reaching the UI as http://my-workstation:4200 is told what to
set rather than just refused.
_hostname("::1") returned ":" -- rsplit on the last colon treats a bare
IPv6 authority as host:port -- so a bracket-less IPv6 Host failed the
loopback check. Brackets are mandatory per RFC 3986 so nothing correct
hits this, but the trap is gone.
kovtcharov
commented
Sep 4, 2026
All four addressed in e24a5aa8.
The route-coverage test now runs on the FastAPI floor. You were right that this was the
one that mattered — it was the durable half of the PR and it was inert on >=0.115.0. The
walk no longer imports _IncludedRouter / _EffectiveRouteContext; it probes for those
shapes by attribute name, so it materialises lazy includes on new FastAPI and falls through
to the flat app.routes list on old. Verified rather than assumed: 30 passed on 0.115.0
and on 0.141.1, and with a mutating route temporarily mounted at /agents/rogue-action the
coverage test goes red on both — AssertionError: mutating routes outside ('/api/', '/v1/'): ['/agents/rogue-action'].
I took the fallback rather than raising the floor, and added a second belt: a cross-check
that every mutating route app.openapi() reports appears in the walk. openapi() is public
and stable on every supported version, and it earned its place immediately — it failed the
first time I ran it, because the walk keeps FastAPI's {agent_id:path} convertor where the
schema reports {agent_id}. Real gap in the comparison, found by the check rather than by a
reviewer.
The stress suite sends the header. And you were right to push on the claim, not just the
line: my sweep covered src/, tui/, and the unit and integration test trees but never
looked at tests/stress/. The PR body now says that explicitly instead of claiming a
completeness it did not have. I re-swept every file in the repo that names port 4200 or
builds a backend URL, checking each for a mutating call — tests/stress was the only miss,
which matches what you found independently.
Both nits taken. The Host rejection now echoes the hostname and names
GAIA_UI_ALLOWED_HOSTS. And _hostname("::1") returning ":" was a real trap — a
count(":") > 1 early return fixes it, with a parametrised test over bare, bracketed and
non-loopback IPv6 authorities.
One thing worth recording for whoever reviews the rest of this batch: the four hub-router
tests I broke were invisible to me locally. The unit conftest's _block_network guard
breaks the socketpair() the Windows asyncio loop opens, so every TestClient in those
directories errors in setup — the files look like they ran and reported the same failure
count before and after. I only found them by re-running with the guard lifted. The Linux
lane is the real signal for those files; a green Windows run is not evidence. That is now
called out in the test plan.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Any web page the user visited could drive their local Agent UI backend. This closes that
with one middleware over every mutating route, deletes a CORS rule that trusted two
rent-a-subdomain namespaces, and adds
Host/Originchecks behind both.Why
The Agent UI backend binds loopback and has no authentication while the tunnel is off, so
the only thing standing between it and a page in another tab was the
X-Gaia-UICSRFheader — and that header was opt-in per route. Four copies of the same guard function,
applied to 24 decorators out of 92 mutating routes. 41 of the remaining ones take no
JSON body, so a plain cross-origin form POST reached them:
POST /api/tunnel/startpublished the machine to the public internet,
POST /api/documents/uploadindexed attackercontent into the user's RAG (persistent prompt injection), and the whole
/v1/email/*surface including
sendwas open. The rest were protected only because FastAPI happens to422 a
text/plainbody — a parser, not a security control.Fixing that alone would not have held. CORS trusted every
*.ngrok-free.appand*.use.devtunnels.msorigin withallow_credentials=Trueandallow_headers=["*"]. Thoseare self-service namespaces: anyone can rent a subdomain, and their preflight was approved
including
X-Gaia-UI— the header the guard is built on. It was also dead config; thetunnel GAIA starts issues
*.ngrok-free.**dev**, which the regex never matched, and themobile flow serves the SPA from the tunnel origin, so those requests are same-origin and
CORS never applies. It granted a capability to attackers and nothing to users.
After this, a route added tomorrow is covered on the day it lands, and a route-table test
fails the build if a mutating route ever ships outside the guarded prefixes.
Linked issue
Closes #3365
Changes
The guard is now a middleware, not a decorator you have to remember.
UIRequestGuardMiddleware(src/gaia/ui/security.py) enforcesX-Gaia-UI: 1on everynon-GET
/api//v1request. Registered outermost, so it runs beforeTunnelAuthMiddleware'stunnel-inactive passthrough and its rejections carry no CORS headers. The four duplicate
guard functions collapse into one import.
Two layers behind it. A mutating request whose
Originis neither this server,loopback, nor the live tunnel is refused, so a future CORS mistake is not sufficient on
its own. A request whose
Hostis a name the server does not know is refused, whichcloses DNS rebinding against the read side (
GET /api/files/preview). IP literals stillpass, so
--host 0.0.0.0LAN access is unaffected;GAIA_UI_ALLOWED_{HOSTS,ORIGINS}arethere for a proxied deployment.
allow_origin_regexdeleted. Nothing needed it.Every non-browser client updated to send the header: the React renderer (set in
apiFetchso no call site can forget, plus the three rawfetchsites that bypass it),gaia mcp'sagent_ui_mcpserver, both eval-harness callers, and the Agent UI stresssuite. The Electron main process already sent it. The Go TUI does not talk to this backend
at all — it reaches the daemon and Lemonade on their own ports.
Correction: the first version of this PR said the inventory was complete, and it was
not — review found
tests/stress/test_agent_ui_stress.py, whose singlehttpx.AsyncClientcarried no headers, so its session-create, chat-send and delete calls would all have 403'd.
My sweep covered
src/,tui/, and the unit and integration test trees but never lookedat
tests/stress/, and that suite needs a live model backend so no test run would havecaught it. I have since re-swept every file in the repo that names port 4200 or builds a
backend URL, checking each for a mutating call:
tests/stresswas the only miss.Breaking for third-party
/v1/emailintegrators. Those routes now requireX-Gaia-UI: 1like the rest;docs/guides/email-integration.mdxsays so and its curlexamples carry it. Exempting them was the alternative, and "send mail as the user" was the
most alarming entry on the forgeable list.
Test plan
pytest tests/unit/chat/ui/test_ui_request_guard.py -v— 25 new tests: route-tablecoverage, the rented-ngrok preflight, the four rejection paths, and every first-party
client shape. Run in a venv built exactly like the CI lane
(
.github/workflows/test_unit.yml,Unit Tests (py3.12),.[api]extras,GAIA_MEMORY_DISABLED=1) to confirm they run there rather than skip — the lane'spathsfilter matches on bothsrc/**andtests/**.The same 25 tests against the unfixed
server.py: 7 fail, including bothpreflight tests and every rejection path. They are regression tests, not assertions
that pass either way.
The route-coverage tests were verified on the declared FastAPI floor, not just my
venv.
fastapi==0.115.0(setup.py): 30 passed, no collection error. Same 30 on0.141.1. And with a mutating route temporarily mounted at/agents/rogue-action,outside the guarded prefixes,
test_every_mutating_route_is_covered_by_the_guardgoesred on both versions:
AssertionError: mutating routes outside ('/api/', '/v1/'): ['/agents/rogue-action'].Reverted after. The walk no longer imports FastAPI private names; a new
test_walk_covers_everything_openapi_knows_aboutcross-checks it againstapp.openapi(),which is public and stable on every supported version — it caught a real gap while being
written.
pytest tests/unit/connectors tests/unit/chat/ui tests/unit/test_hub_router.py tests/unit/test_memory_router.py tests/unit/test_goals_router.py tests/unit/test_scheduler_api.py tests/unit/test_onboarding_router.py— 1860 passed.pytest tests/unitin full — 10385 passed, 100 failed, 15 errors. Every one of thosefailures reproduces unchanged on
upstream/main(macOS-installer, launcher, scorecardand packaging suites that do not run on this box), verified by reverting the patch and
re-running the affected files: identical 76-line failure list before and after.
pytest tests/integration/test_chat_ui_integration.py tests/integration/test_documents_router.py tests/integration/test_files_router.py tests/integration/test_folder_indexing.py— 200 passed, 2 failed. Both failures reproduce on
upstream/mainunchanged (theembedding model is unreachable on this box), verified by reverting the patch and
re-running.
npm run build,tsc --noEmit, andvitest runinsrc/gaia/apps/webui— 279 testspassed, clean typecheck, build succeeds.
npx jest test_agent_process_manager.jsintests/electron— 114 passed.python util/lint.py --all— Black, isort, Flake8, Bandit, agent conventions allpass. The one failure is a pre-existing Windows-only Pylint false positive
(
os.killpginsrc/gaia/daemon/sidecars/), in files this PR does not touch.Not run:
gaia eval agent. This PR touches no LLM-affecting surface — no systemprompts, tool registration, tool docstrings, JSON tool schema, error classification,
or default model. It changes HTTP request admission only.
Evidence
user-visible effect is that the SPA keeps working unchanged, which the "first-party
clients" block below exercises directly against the real server.
agent_ui_mcp.pynow sends the header on every call;covered by the "no Origin + header" row below, which is the exact request shape it
issues.
gaiasubcommand changed.gaia.ui.serveron a scratch portwith a throwaway
HOME, before and after.POST /api/memory/prunestands in forPOST /api/tunnel/start: same body-less shape, and demonstrating the tunnel routewould have opened a real public tunnel.
Before (
upstream/main@abe87edc):After:
Every first-party client path still works — the part that matters for not breaking users:
Adjacent paths checked, since the header is only a defence if nothing re-widens the
origin set and nothing routes around the middleware:
Known-flaky, unrelated
Test GAIA CLI on Linux (Full Integration)failed on an earlier push because Hugging Facereturned
429pullingunsloth/Qwen3-0.6B-GGUF, so Lemonade never started and the rest ofthe log is retries against a server that never came up. Nothing in it reaches CSRF, CORS or
this middleware. Deliberately not "fixed" — a retry or fallback around the model pull is the
silent-degradation patch CLAUDE.md forbids, and it is out of scope here.
Checklist
Closes #3365).python util/lint.py --all,pytest tests/unit/).(
docs/guides/email-integration.mdx,docs/spec/agent-ui-server.mdx).