Repository navigation
bug(terminal): terminal-tools router (install-tool/check-tool/validate-command) has no auth at all #15084
Description
Activity
Severity escalation. This is not "gated only positionally" — there is a second mount that is not gated at all, and it is live on the deployed host.
I filed this describing a fragility: the four command-running routes are protected only by which parent router includes them. That was true of the mount at
api/terminal.py:197. It missed a second one.autobot-backend/initialization/router_registry/terminal_routers.py:19registersapi.terminal_toolsas a top-level router with an empty prefix, loaded by the application factory. Verified on the merged base and on the deployed tree:Check Result terminal_routers.py:19("api.terminal_tools", "", ["terminal-tools"], "terminal_tools")— registered directly, not through the gated parentthe loader adds no dependencies=of any kinddeployed api/terminal_tools.py:32router = APIRouter(tags=["terminal-tools"])— no dependencies,grep -c dependenciesreturns 0service-auth middleware does not cover these — SERVICE_ONLY_PATHSlists only/api/npu/*and/api/ai-stack/*internal endpointsSo
/api/install-tool,/api/check-tool,/api/validate-commandand/api/package-managersare reachable without the admin check, and they install packages and run system commands. Theapi/terminal.pymount being correctly gated hides this: anyone auditing that file sees a protected router.Why it stayed invisible. Every guard around these routes asserted the gate on the terminal.py mount. None enumerated where else the router is mounted. A route can be gated on one mount and open on another, and a test that only knows about the first will stay green — the same shape as the handler-level tests that were green while the terminal WebSocket route 500'd for every caller (#14998).
The fix in PR #15096 is the right one for this, and now for a stronger reason than I gave it: putting the dependency on
terminal_tools.py's own router means it travels with the router to every mount, present and future. That is what makes placement stop mattering. Its new test mountsterminal_tools.routeralone — precisely the previously-unauthenticated shape — and asserts a 403 per route, with a non-vacuity partner so a 404 cannot make the refusal pass for the wrong reason.Deployment note: the running install carries the ungated copy, so this is live until a deploy carries the fix. I have not touched the host.
Follow-up worth its own issue: nothing checks that a router is mounted only where intended. A guard that enumerates every mount point per router — and fails when one lacks the gate its siblings carry — would have caught this at the point the second registration was added.
Guard for the defect class filed as #15098 and implemented in #15099 (
repo_tests/router_mount_parity_test.py). It derives the mount graph from source and fails when a router is reachable through a mount lacking protection its other mounts carry — with the exemption for this router removed it reproduces exactly this finding, naming both mount sites. The exemption is self-retiring: the moment #15096 merges, that test file goes red until the line is deleted.- added a commit that references this issue
on Aug 26, 2026 Delivered by PR #15096, merged to
Dev_new_guias6cc5505ef.The gate now lives on the router itself, verified on the merged base:
autobot-backend/api/terminal_tools.py:44 router = APIRouter(tags=["terminal-tools"], dependencies=[Depends(check_admin_permission)])That is the whole point. A dependency declared on the
APIRouterconstructor applies at every mount, present and future. The previous arrangement depended on which parent happened to include it — and it was included in two places:Mount Gate api/terminal.py:197viaadmin_router(declared:175)gated initialization/router_registry/terminal_routers.py:19, empty prefix, included byapp_factory.py:82-84asapp.include_router(router, prefix=f"/api{prefix}", tags=tags)none So
/api/install-tool,/api/check-tool,/api/validate-commandand/api/package-managers— which install packages and run system commands — were reachable without the admin check.SERVICE_ONLY_PATHS(middleware/service_auth_enforcement.py:106) covers only/api/npu/*,/api/ai-stack/*and/api/browser/*, so no middleware caught them either.Verification is behavioural, not structural. Under fastapi 0.141.1 an inherited
dependencies=is not visible on the route object at all (#15093), so a structural assertion would have proved nothing. The newapi/terminal_tools_test.pymountsterminal_tools.routeralone — precisely the previously-unauthenticated shape — and asserts 403 per route, each paired with a non-vacuity partner asserting the response is not a 404, because an unrouted path never reaches a dependency and a 404 would make the refusal pass for the wrong reason.Contrast mutation, removing the dependency and reparenting the include onto the dependency-free router, reproducing the near-miss exactly:
TestTerminalToolsStandaloneMountIsGated FFFF.... TestTerminalToolRoutesKeepTheirGate FFFF.... 8 failed, 8 passedAll eight refusal tests failed with 422/200 instead of 403; all eight non-vacuity partners stayed green, which is what tells you the failures were real rather than routing noise.
On the double declaration.
admin_routerstill carries the check, so it is now declared twice on that path. Measured rather than assumed:Depends()defaults touse_cache=Trueand the cache key is(callable, security_scopes), identical for both, sosolve_dependenciesresolves it once per request. Defence in depth at no runtime cost, and the router's own declaration is what closes the hole.The exposure is bounded, and that was checked rather than hoped. A sweep across 340
APIRouterdefinitions and 423 mount edges found exactly one router whose mounts disagree — this one. Every other gated router declares its gate on its own constructor.terminal_toolswas the only router in the codebase protected purely by inheritance from a parent mount, which is why it was the one that broke.Why no existing guard caught it: every guard asserted the gate on the
terminal.pymount. None enumerated where else the router is mounted. That class is now filed as #15098 with a static mount-parity guard in #15099 — static rather than runtime, because a runtime guard reads differently in CI than locally under the version gap in #15091, and a guard that disagrees with itself is worse than none.Deployment: the running install still carries the ungated copy, so this remains open on the host until a deploy carries
6cc5505ef. The host was not touched.
What
While fixing #14998 (router-level
Dependscannot gate a WS route), I confirmedapi/terminal_tools.py's router carries no auth dependency at all:None of the four take
current_user/Depends(get_current_user)orDepends(check_admin_permission).install_toolconstructs aSystemCommandAgentand callsinstall_tool(...), i.e. this is anunauthenticated arbitrary-package-installation surface.
This router is mounted twice:
api/terminal.py:197--router.include_router(tools_router, prefix="/terminal"),so it inherits
terminal.py's router-leveldependencies=only if FastAPIapplied it to sub-routers automatically. It does not:
include_router()onlyapplies dependencies passed explicitly to the
include_router(...)call, oralready baked into the child router's own routes at decoration time -- and
tools_routerwas never givenDepends(check_admin_permission)of its own(confirmed against
fastapi/routing.py'sinclude_router/add_api_route).initialization/router_registry/terminal_routers.py:19-- registered again asits own top-level entry,
("api.terminal_tools", "", ["terminal-tools"], "terminal_tools"), with an empty prefix.So these four endpoints are reachable, unauthenticated, from at least one of the
two mount points (need to confirm the effective final path under the live
nginx/FastAPI prefix chain for both).
Why separate from #14998
#14998 is the WS-scope
Dependsresolution bug onapi/terminal.py's ownrouter. This is a different router (
api.terminal_tools), a different failureshape (dependency never declared, not one that fails to resolve), and touches
unauthenticated command execution, not a functional WS outage -- different
risk profile, needs its own review rather than riding along in that PR.
Suggested fix
Add
Depends(get_current_user)(orDepends(check_admin_permission), giveninstall_toolruns arbitrary system commands) to each of the four routes, oradd
dependencies=[...]toapi.terminal_tools.router's constructor. Confirmboth mount points pick up the fix, since the router object is shared between
them.
Acceptance criteria
terminal_toolsendpoints require authentication (at minimum);install_toolrequires admin, given it executes arbitrary install commands.non-admin/unauthenticated caller is rejected on both mount points.
the process today, and note it in the fix PR.