fix(terminal): stop the router-level admin Depends 500ing both terminal WS handshakes

[mrveiss]

Thinking Path

The issue reports autobot-backend/api/terminal.py:172-175 declaring
dependencies=[Depends(check_admin_permission)] at router level, and
check_admin_permission (autobot-backend/auth_middleware.py:972, not
autobot_shared/auth/auth_middleware.py -- that path in the issue body doesn't
exist in this tree) being Request-typed. I read FastAPI's own
add_api_websocket_route/add_api_route (fastapi/routing.py): both do
current_dependencies = self.dependencies.copy() and pass that into the route
constructor -- so a router-level dependencies= genuinely does apply to
@router.websocket routes exactly like HTTP ones, unconditionally, at
registration time. A Request-typed dependency then fails FastAPI's
solve_dependencies for a WS scope (no Request object exists there),
raising TypeError before the handler is ever invoked. I reproduced this
empirically in isolation with TestClient before touching the fix (see the
contrast-mutation test below) -- confirmed, not assumed.

check_admin_permission's own auth logic is not the reported bug: it already
runs correctly for the router's HTTP routes. The two @router.websocket
routes (terminal_websocket :824, ssh_terminal_websocket :871) both already
call enforce_ws_origin/enforce_ws_authentication (and is_admin_role on
the SSH route) explicitly before accept() -- that per-route guard landed in
#14989/#14991 and is what the fix needs to lean on, not remove.

What Changed

autobot-backend/api/terminal.py:

• Split the router in two: router = APIRouter(tags=["terminal"]) (no
  dependencies -- owns both @router.websocket routes, which keep their
  existing explicit per-route auth) and
  admin_router = APIRouter(dependencies=[Depends(check_admin_permission)]).
• Every HTTP endpoint decorator (@router.get/post/delete(...), 20 routes)
  became @admin_router.get/post/delete(...) -- a rename only, same paths,
  same handlers, same admin gate.
• router.include_router(admin_router) at the end of the file, once every
  admin_router route exists, merges the HTTP routes back into router so
  the module still exports one router object for the loader
  (initialization/router_registry/terminal_routers.py).
• Trimmed the ssh_terminal_websocket docstring, which described the
  router-level Depends as declared-but-inert; that clause is now false (it's
  not declared on router at all), so it's rewritten and shortened to net
  zero lines against terminal.py's grandfathered 1299-line ceiling
  (verified: file is exactly 1299 lines after the change, no ceiling edit
  needed).

HTTP routes keep their gate

router.include_router(admin_router) merges admin_router's routes into
router using FastAPI's own include_router/add_api_route, which combines
current_dependencies = self.dependencies.copy() + route.dependencies --
since admin_router's own routes already carry Depends(check_admin_permission)
at decoration time, the merged routes on router carry it too. Verified two
ways:

• Structural: TestTerminalRouterDependencyWiring inspects
  route.dependencies on the live api.terminal.router object and asserts
  check_admin_permission is present on the merged / route and absent from
  both WS routes, by identity -- independent of any auth-middleware mocking.
• Behavioural: TestTerminalHttpRouteAdminGate drives GET / through a real
  TestClient, using FastAPI's dependency_overrides to force
  check_admin_permission to deny/allow, and asserts 403 vs 200.

Verification

Existing test that was green against a broken route: none of the WS routes
had a passing handshake test at all in the true sense -- but
api/terminal_websocket_auth_test.py and api/ssh_terminal_websocket_auth_test.py
call terminal_websocket/ssh_terminal_websocket as plain functions
(await terminal_websocket(ws, session_id)), bypassing FastAPI routing and
dependency resolution entirely. Both suites were green on Dev_new_gui while
every real handshake 500'd -- exactly the blind spot the issue describes. They
required zero changes here (the functions/module-level names they patch never
moved), and still pass.

New file autobot-backend/api/terminal_websocket_route_test.py drives the
real routes via starlette.testclient.TestClient (the same pattern as
tests/test_websocket_auth_smoke.py):

• TestTerminalWebsocketRouteHandshake / TestSshTerminalWebsocketRouteHandshake:
  authenticated caller connects (WebSocket.accept spied via a real function,
  not a Mock, so it stays a proper bound-method descriptor -- asserted called
  once), unauthenticated caller is refused with close(1008) before
  accept() (spy asserts zero calls), for both WS routes.
• TestTerminalHttpRouteAdminGate: non-admin caller on the HTTP surface gets
  403, admin gets 200.
• TestRouterLevelDependsBreaksWebSocketScope (the contrast mutation): an
  isolated router built with the exact broken shape (APIRouter(dependencies= [Depends(<Request-typed fn>)]) owning a @router.websocket route) raises
  TypeError on TestClient.websocket_connect -- the same failure
  solve_dependencies produces in the real bug. The split-router shape this
  fix uses, built the same way, does not raise. I could not reuse
  check_admin_permission itself for this: this repo's conftest.py stubs
  auth_middleware.check_admin_permission with a no-arg signature
  specifically to dodge an unrelated FastAPI validation issue (feat(ui/slm): pluggable theme packages — upload + install themes via /slm (Phase 2 of #10461) #10472), so it
  can't reproduce a Request-typed WS failure in this harness -- the mutation
  test builds a matching-shape dependency directly instead, so it targets the
  real FastAPI mechanic rather than the test harness's own auth stub.

python3 -m pytest autobot-backend/api/terminal_websocket_route_test.py \
  autobot-backend/api/terminal_websocket_auth_test.py \
  autobot-backend/api/ssh_terminal_websocket_auth_test.py \
  autobot-backend/api/vnc_proxy_websocket_auth_test.py \
  autobot-backend/api/simple_terminal_e2e_test.py \
  autobot-backend/services/agent_terminal/session_manager_owner_test.py -q
# 22 passed, 1 skipped

python3 scripts/check_python_file_size.py --audit-ceilings
# python-file-size ceilings: 4859 files scanned, 509 grandfathered, all live and at size.  rc=0

python3 -m bandit -c .bandit -q autobot-backend/api/terminal.py autobot-backend/api/terminal_websocket_route_test.py
# clean

python3 -m py_compile autobot-backend/api/terminal.py autobot-backend/api/terminal_websocket_route_test.py
python3 -m black --check --line-length 120 autobot-backend/api/terminal.py autobot-backend/api/terminal_websocket_route_test.py
python3 -m isort --check-only autobot-backend/api/terminal.py autobot-backend/api/terminal_websocket_route_test.py
# all clean

This also discharges #14960's AC3 ("an authenticated owner still connects
normally") in the sense that it is now checkable at all through a real route --
test_authenticated_owner_connects in the new file proves it. I am not
closing #14960 here; its other three ACs (unknown-session-id rejection,
non-owner rejection, guard-test-fails-on-removal) are already covered by
api/terminal_websocket_auth_test.py and need their own verification pass on
that issue, not a claim riding along on this PR.

Discovered problem, filed separately

While confirming which HTTP routes on this router "genuinely need" the admin
gate, I found api/terminal_tools.py's router (/install-tool,
/check-tool, /validate-command, /package-managers) carries no auth
dependency at all, and is mounted twice (once included into terminal.py's
router, once as its own top-level entry in
initialization/router_registry/terminal_routers.py). install_tool runs
arbitrary system commands via SystemCommandAgent. Different router, different
failure shape (never declared vs. declared-but-unresolvable), different risk
(unauthenticated command execution vs. functional 500) -- filed as
#15084 rather than bundled here.

Model Used

Claude Opus 5

Closes #14998

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Holding this back one commit: as pushed, the split would have removed the admin gate from four routes that run system commands. Fixed at c43dc7f27.

api/terminal_tools.py declares no dependency of its own — /terminal/install-tool, /terminal/check-tool, /terminal/validate-command and /terminal/package-managers have only ever been protected by inheriting the parent router's admin check. On Dev_new_gui today router carries dependencies=[Depends(check_admin_permission)] and terminal.py:197 does router.include_router(tools_router, ...), so they are gated.

This PR moves every HTTP route onto a new admin_router and leaves router dependency-free for the WebSocket handshakes — correct — but the tools include site stayed on router. The four routes would have come out the other side ungated and anonymously reachable, while every check in the PR stayed green: they were never in the diff, and nothing asserted their gate.

That is the security carve-out in miniature. Union the checks, never the allowances; a refactor that fixes one surface must not quietly widen another. It also sharpens #15084, which I had understood as "the tools router has no auth" — accurate about the module, but incomplete about the deployed state. Their protection is entirely positional, which is exactly why a router split is the thing that breaks it.

Fix: the include site moves with the routes — admin_router.include_router(tools_router, prefix="/terminal").

And a guard so position can't silently change again. terminal_websocket_route_test.py now asserts, by identity against the live merged router, that each of the four carries check_admin_permission, plus a non-vacuity test that fails if the paths move (otherwise a rename would leave the gate assertion inspecting an empty list).

• Contrast mutation: reparent the include back onto router → 4 failed, one per route, each naming the path. Restore → 15 passed.
• Direct probe against the merged router object before and after, same result.

api/terminal.py is grandfathered at 1299 and may not grow, so the explanatory comment was trimmed to fit rather than the ceiling being raised — final count is exactly 1299, --audit-ceilings rc 0.

The rest of the PR stands and is good work: the router-level Depends really was resolved for @router.websocket routes (add_api_websocket_route copies self.dependencies), the TypeError reproduces empirically, and the new tests drive real handshakes instead of calling handlers — which is precisely why the existing terminal_websocket_auth_test.py and ssh_terminal_websocket_auth_test.py were green on a route that 500'd for every caller.

One correction to the issue text I wrote: check_admin_permission is at autobot-backend/auth_middleware.py:972, not autobot_shared/auth/auth_middleware.py — that path does not exist.

Verified: 15 passed · ratchet rc 0 · bandit rc 0 · black and isort clean · 0 behind base.

[mrveiss]

Addressed the shard-12 red (job 98074498060, 6 failures in terminal_websocket_route_test.py).

Root cause of the test failure (not the fix's correctness): api.terminal.router.routes enumerated empty when read in-process under that shard, despite passing in isolation and in every local repro I tried (serial full-shard run, -n 2 --dist loadscope against the same --shard-splits 12 --shard-group 12 module set). Local repro didn't reproduce -- CI runs Python 3.14.7, this box only has 3.10.12 available (no venv/install permitted here), and xdist's loadscope worker assignment depends on collection order/counts that differ across that gap. I did not pin the exact polluting test before switching strategy per direction; filed as #15087 (unidentified polluter, cross-linked to #15076 and #15069 as the same "shared interpreter state" family).

Fix (016739e2d): moved every structural route/dependency assertion (TestTerminalRouterDependencyWiring, and the tool-route gate tests renamed into TestTerminalToolRoutesKeepTheirGate) off the in-process, collection-time-bound router object and onto a clean-interpreter subprocess dump, mirroring autobot_shared/user_management/models/core_test.py's per-class subprocess pattern. The subprocess imports api.terminal fresh and reports each route's path plus its dependencies' fully-qualified names (auth_middleware.check_admin_permission), immune to whatever an earlier test in the worker left behind, regardless of mechanism. Non-vacuity is preserved: the terminal_route_spec fixture asserts the enumeration is non-empty before any test consuming it runs, and fails loudly (not skips) if it is.

TestTerminalHttpRouteAdminGate (dependency_overrides) and the handshake tests (TestClient.websocket_connect) stay in-process, since they need object identity with the live app/router under test -- they were not among the failures and can't move to a subprocess.

Contrast mutation re-verified after the rewrite: temporarily reparented tools_router from admin_router back onto router (undoing c43dc7f27) --

autobot-backend/api/terminal_websocket_route_test.py .FFFF               [100%]
FAILED ...test_tool_route_keeps_the_admin_dependency[/terminal/check-tool] - assert 'auth_middleware.check_admin_permission' in []
FAILED ...test_tool_route_keeps_the_admin_dependency[/terminal/install-tool] - assert 'auth_middleware.check_admin_permission' in []
FAILED ...test_tool_route_keeps_the_admin_dependency[/terminal/package-managers] - assert 'auth_middleware.check_admin_permission' in []
FAILED ...test_tool_route_keeps_the_admin_dependency[/terminal/validate-command] - assert 'auth_middleware.check_admin_permission' in []
4 failed, 1 passed

then restored (git diff against the committed terminal.py came back empty) and reran clean.

Verification:

python3 -m pytest autobot-backend/api/terminal_websocket_route_test.py \
  autobot-backend/api/terminal_websocket_auth_test.py \
  autobot-backend/api/ssh_terminal_websocket_auth_test.py -q
# 22 passed

python3 scripts/check_python_file_size.py --audit-ceilings
# 4860 files scanned, 509 grandfathered, all live and at size — rc 0
# api/terminal.py unchanged in this commit, still exactly 1299 lines

python3 -m bandit -c .bandit -q autobot-backend/api/terminal.py autobot-backend/api/terminal_websocket_route_test.py
# clean
python3 -m black --check --line-length 120 ...  &&  python3 -m isort --check-only ...
# both clean

Branch is a fast-forward push (c43dc7f27..016739e2d), no force needed -- 0 behind origin/Dev_new_gui.

[mrveiss]

Pushed `ea37833dc`: the dump no longer swallows a partial/anomalous result.

What changed:

• `_run_terminal_route_dump()` now checks the subprocess exit code explicitly (was `check=True`, which only surfaced `CalledProcessError` with no stdout/stderr shown) and raises `_TerminalRouteDumpError` with the subprocess's full stdout+stderr attached on non-zero exit.
• Any route entry with `path is None` is rejected rather than folded into the `path -> dependencies` mapping (previously a dict comprehension silently coalesced every such entry into one `None` key, hiding how many there really were) -- its presence now fails the whole fixture with the raw route list attached.
• The subprocess (`_dump_routes_main`) now reports each route's concrete `type(route).name` alongside its path/dependencies, plus the resolved `sys.version`, `fastapi.version`, `starlette.version`, all in the same JSON payload -- no second round-trip needed to get version info.
• Simulated the CI failure shape locally (monkeypatched `subprocess.run` to return the reported 3-route/`None`-path/type=`Mount` payload) and confirmed the new message surfaces exactly what's needed:

```
terminal route dump returned 3 route(s), of which 1 had no identifiable path -- ...
raw routes: [
{"path": "/ws/{session_id}", "type": "APIWebSocketRoute", "dependencies": []},
{"path": "/ws/ssh/{host_id}", "type": "APIWebSocketRoute", "dependencies": []},
{"path": null, "type": "Mount", "dependencies": []}
]
python: '3.14.7 (CI simulated)'
fastapi: '0.141.9-simulated'
starlette: '1.2.3-simulated'
subprocess exit code: 0
--- subprocess stdout ---
...
--- subprocess stderr ---
```

On the local fastapi/starlette version gap: this box has `fastapi==0.135.2`/`starlette==1.0.0` installed, which does not satisfy `requirements.txt`'s `fastapi>=0.141.1` -- so my local `include_router` behavior may simply not match what a fresh `pip install -r requirements.txt` resolves under CI's Python 3.14 runner. I cannot install packages on this box to confirm directly; the next CI run's `fastapi`/`starlette` fields plus the per-route `type` will say definitively whether this is a version-behavior change in `include_router` (in which case I'll fix the wiring or file it as its own cross-linked issue per the earlier direction) or something else entirely.

Re-verified before pushing:

• Contrast mutation (reparent `tools_router` onto `router`): the 4 named tests fail (`assert 'auth_middleware.check_admin_permission' in []`); restored, `git diff` on `terminal.py` empty, tests green again.
• `python3 -m pytest terminal_websocket_route_test.py terminal_websocket_auth_test.py ssh_terminal_websocket_auth_test.py -q` → 22 passed.
• `api/terminal.py` untouched, still exactly 1299 lines; `black --check --line-length 120`, `isort --check-only`, `bandit -c .bandit -q` all clean on the changed file.
• Fast-forward push `016739e2d..ea37833`, 0 behind `origin/Dev_new_gui`, no force needed.

[mrveiss]

Root cause found, and it was never pollution — it is a FastAPI behaviour change the loud diagnostics named in one run.

CI job 98088603289 reported fastapi 0.141.1 / starlette 1.6.0 and this route list:

[APIWebSocketRoute /ws/{session_id}, APIWebSocketRoute /ws/ssh/{host_id}, _IncludedRouter path=null]

Under 0.141.1, include_router() does not flatten the child's routes into the parent. It appends a deferred _IncludedRouter whose own path is None, and the real routes live on .original_router.routes. This box runs fastapi 0.135.2, where inclusion is eager — hence 26 routes locally against 3 in CI. Nothing was ever broken at runtime; the guard was interrogating the wrong object.

My first correction — mount a FastAPI() app and read app.routes — was also wrong, and CI said so plainly (job 98107587675): 1 route(s), of which 1 had no identifiable path. Mounting defers too.

The repo already had the answer, in one file. llc/tests/test_roles_routes_registered.py:53 documents this verbatim — "This FastAPI version defers inclusion: router.routes holds _IncludedRouter wrappers with no .path, and the real routes live on .original_router.routes. Walking the top level alone finds one route out of thirty-eight and reads as 'nothing is mounted'." — and names two other files carrying the same idiom by hand. Copied it rather than inventing a fourth variant, with the recursion accumulating prefixes, and dependencies read from route.dependant.dependencies (the set FastAPI actually resolves, already merged with anything inherited through the routers a route was included through) rather than the un-merged route.dependencies.

That four files now carry this traversal by hand is its own finding — a true fact about the framework living in three test files and nowhere a fourth author would look. A sweep for other router.routes readers is running; api/long_running_operations.py does manager.router.routes[0].endpoint(...) at four sites and is not a test.

Worth stating: the loud-diagnostics change is what made this solvable. Two CI runs, each naming the exact object and the resolved versions, replaced an unreproducible bisect. The earlier guard swallowed its subprocess's stderr and reported "'/terminal/check-tool' missing from a clean import", which is the symptom, not the cause.

Verified at 1671dd164: 15 passed in the route suite, 22 across all three terminal WS suites. Contrast mutation — reparent tools_router onto router — fails exactly 4 tests, one per command-running route; restored. api/terminal.py untouched at exactly 1299. black, isort, bandit, ratchet all clean. 0 behind base.

Related: #15091 (this box below the declared fastapi>=0.141.1 SECURITY floor — no longer hypothetical, it has now produced a CI-only failure invisible locally) and #15087.

[mrveiss]

Fourth and final shape, at 92b13d0b1. I stopped trying to recover the prefix and asked what the guard actually needs to prove.

Three CI runs established that under fastapi 0.141.1 the prefix passed to include_router is not recoverable by introspection: it is on neither the deferred _IncludedRouter wrapper's .prefix nor the original router's, and is applied at request-routing time. Jobs 98110125325 and 98112466169 both returned /install-tool where /terminal/install-tool was expected, with both prefix candidates empty.

Chasing it a fourth time would have been the wrong instinct. The prefix is incidental to the property under guard — that the four routes which install packages and run system commands carry check_admin_permission. So the match is now by suffix, with a uniqueness assertion doing the work the exact path used to: each suffix must resolve to exactly one route, because two routes ending the same way would let a gated one vouch for an ungated one. That is the loophole a looser match would otherwise open, and it is closed explicitly rather than assumed away.

The non-vacuity assertion is kept and strengthened — an empty dump now fails with "every assertion below would pass vacuously" rather than silently matching nothing. That property is what caught this whole class in the first place.

Track record of this PR, worth recording because the method is the point: four CI runs, four different causes, each named outright by the guard's own diagnostics rather than guessed at.

1. 98088603289 — 0.141.1 defers include_router; router.routes holds an _IncludedRouter with path=null. Local 0.135.2 is eager, hence 26 routes vs 3.
2. 98107587675 — mounting a FastAPI() app and reading app.routes also defers. My correction was wrong too.
3. 98110125325 — recursion over .original_router works; the prefix does not come with it.
4. 98112466169 — nor is it on the original router. Hence this change.

An unreproducible local bisect cost hours and produced nothing; a guard that prints its subprocess's stderr, the route types and the resolved versions produced a cause per run. That is the durable lesson, and it is why the diagnostics stay in even though the assertion no longer depends on them.

Verified: 15 passed in the route suite, 22 across all three terminal WS suites. Contrast mutation — reparent tools_router onto the dependency-free router — fails exactly 4 tests, one per command-running route; restored. api/terminal.py untouched at exactly 1299. black, isort, bandit, ratchet clean. 0 behind base.

Related: #15093 (the repo carries this traversal by hand in four places and guards it in none — and one copy is version-locked the other way) and #15091 (this box runs below the declared fastapi>=0.141.1 SECURITY floor, which is why none of this was visible locally).