Repository navigation
Unknown terminal session_id defaults to a live STANDARD-security terminal #14961
Description
Activity
- added 10 commits that reference this issue
on Aug 24, 2026 Closure audit — staying open. Verified against merged base
Dev_new_gui(2bf56f158, PR #14989)The deny-by-default fix itself is done and well tested. Criterion 3 — the one the issue itself flagged as "needs checking, not assuming" — is not met, and the change makes that gap user-visible rather than latent.
# Acceptance criterion Evidence on the merged base Verdict 1 A session_idnever created is rejected; noTerminalWebSocketconstructed, no shell startsautobot-backend/api/terminal.py:731-733—config = session_manager.session_configs.get(session_id)thenif config is None: ... return None; caller closes1008beforeaccept()(terminal.py:843-848). Testautobot-backend/api/terminal_websocket_auth_test.py:92-124assertsclose(code=1008),accept.assert_not_awaited(),_init_terminal_handlernever awaitedMet 2 No code path applies a default SecurityLevelto an unknown session_init_terminal_handlernow takes an already-validatedconfigparameter (terminal.py:740-744); theSecurityLevel(config.get(...))fallback at:765is reachable only for a session that exists and is owned. The old.get(session_id, {})shape is gone. The test pins this discrimination specifically: it asserts the "unknown session_id" log line is present and the "is not the owner" line is absent (terminal_websocket_auth_test.py:121-123), so reverting to.get(id, {})fails rather than passing through the owner branchMet 3 A session created on one worker and attached from another still connects (or the cross-worker lookup is fixed as part of this work, not deferred) Neither half happened. session_manager.session_configsis still a plain in-process dict —autobot-backend/api/terminal_handlers.py:1306(self.session_configs = {}), single instance at:1504;create_terminal_sessionwrites to it in-process (terminal.py:241). No shared-state lookup was added. The deployed backend runs uvicorn with 4 workers plus--limit-max-requests 1000worker recycling (autobot-infrastructure/autobot-backend/templates/autobot-user-backend.service:18-24)Not met 4 A test asserts the unknown-session case is rejected, driving the real route, checking presence of the rejection terminal_websocket_auth_test.py:92-124invokes the decorated route callable imported fromapi.terminaland asserts the close code plus the distinguishing log lineMet at function level (see caveat) Why criterion 3 matters more after this change than before
Before, an absent config silently fell through to a default-configured terminal — the bug this issue is about, but it also meant a cross-worker or post-recycle attach happened to "work". Now absence is a hard
1008. On a 4-worker deployment the create (POST /sessions) and the WebSocket attach routinely land on different workers, and--limit-max-requestsrecycling dropssession_configsfor every session on that worker. So the correct security behaviour has been added on top of process-local state, which converts the old silent-permissive path into a silent-broken one for legitimate users.This is exactly the risk the issue text called out. It must be resolved here rather than deferred: move the session-config lookup to shared state (the pattern used for the same problem in
autobot-backend/takeover_manager.py:9,189), or demonstrate with evidence that this router is only ever served single-worker.Caveat on criteria 1 and 4
The guard tests call
terminal_websocketdirectly. On the merged base the route cannot be reached through the ASGI stack at all:terminal.py:172-175still carriesdependencies=[Depends(check_admin_permission)], andcheck_admin_permissionisRequest-typed (autobot-backend/auth_middleware.py:972), which FastAPI cannot resolve in a WebSocket scope — the 500-on-every-handshake outage tracked in #14998 (open). The rejection logic is correct; a real client currently gets a 500 rather than the1008. That does not by itself unmeet criteria 1/2/4, but it does mean criterion 3's "still connects" cannot be satisfied until #14998 is fixed either.What is outstanding
- Cross-worker (and post-worker-recycle) session-config lookup — fix it here, or prove single-worker operation with evidence.
- bug(terminal): both terminal WebSocket routes 500 on every handshake — a router-level Depends cannot resolve in a WS scope #14998 fixed, so "connects" is testable end to end at all.
Criteria 1, 2 and 4 have landed. Re-audit for closure once the above are addressed.
- added a commit that references this issue
on Aug 26, 2026 Closure check — staying open: 3 of 4 criteria met, criterion 4 outstanding
Verified per criterion against merged
Dev_new_gui(16c104be5in history), not against the PR
diff. #15100 is deliberatelyRefs, notCloses, and that turns out to be right.# Criterion (verbatim, abridged) Verdict Evidence in merged base Evidence kind 1 A WebSocket connection naming a session_idthat was never created is rejected; noTerminalWebSocketis constructed and no shell process starts.Met api/terminal.py:725-728—session_manager.session_configs.get(session_id)with no default;Nonelogs"unknown session_id"and returnsNone. The route refuses at:843-846with1008, beforeaccept()at:848;_init_terminal_handler(the only construction site,:851) is never reached. Route-level proof that the route really executes this lookup pre-acceptcomes from the sibling non-owner test,api/terminal_websocket_route_test.py:377-392.merged code; the unknown-session case itself is exercised only handler-level ( api/terminal_websocket_auth_test.py:116-146)2 No code path applies a default SecurityLevelto an unknown session.Met The single SecurityLevel(config.get("security_level", ...))in the module isapi/terminal.py:761, inside_init_terminal_handler, which by contract (:740-763) receives an already-existence- and ownership-validated config — so the default now covers only a field missing from a real record. No.get(session_id, {})remains on any terminal WS path; the one surviving default-dict lookup,api/terminal_handlers.py:1451, is a stats reporter behind an existence check at:1441and applies noSecurityLevel.merged code (static) 3 A session created on one worker and attached from another still connects (or the cross-worker lookup is fixed as part of this work, not deferred). Met, via the second branch The lookup was fixed, not deferred: services/terminal_session_store.pyreplaces the process-local dict with a Redis-backed, dict-protocol store on the canonical client, fail-closed on an outage; wired in production atapi/terminal_handlers.py:1303. Proven atservices/terminal_session_store_test.py:41-56(two independent clients, one backing store: worker B resolves worker A's session, round-tripped rather than sharing an object) and:83-87(twoTerminalManagerinstances, same result).unit + production wiring 4 A test asserts the unknown-session case is rejected — driving the real route, and checking for the rejection's presence rather than the absence of an error. NOT met The only unknown-session test is api/terminal_websocket_auth_test.py:116-146, which calls the decorated handler directly (await terminal_websocket(ws, unknown_session_id)), bypassing routing and dependency resolution entirely. Its assertions are otherwise exactly right —1008,acceptnot awaited,_init_terminal_handlernot awaited, andcaplogseparating the"unknown session_id"branch from the"is not the owner"branch, since both exit through the sameclose(1008)atapi/terminal.py:845.api/terminal_websocket_route_test.pydoes drive the real route viaTestClient, but only for the owner, unauthenticated and non-owner cases (:348,:364,:377) — there is no route-level unknown-session handshake anywhere in the repo.— What is outstanding
Exactly one thing: a route-level test of the unknown-session handshake. Concretely, a case in
api/terminal_websocket_route_test.py::TestTerminalWebsocketRouteHandshakethat connects through
TestClientto/ws/{a-uuid-never-written-to-the-store}as an authenticated user, and asserts
1008on the wire, an accept-spy count of0, and — viacaplog— that the"unknown session_id"
branch fired and"is not the owner"did not.Why this is not a formality: the criterion says "driving the real route" for the same reason
#14960's criterion 2 was rewritten to demand it. #14998 is the precedent — handler-level terminal
WebSocket tests stayed green for the entire period the route500'd insidesolve_dependencies
for every caller, because a test that calls the handler function cannot observe a routing or
dependency-resolution failure. The fixture the route-level file already carries (:324-335, a
fakeredis-backedSessionConfigStore) makes this a short addition.Criteria 1-3 are met and need no further work; only criterion 4 blocks closure.
No sub-issues (
sub_issuesendpoint returns empty).Also worth recording, not a criterion: the merge commit for #15100 showed python-suite shard
11/12 red; base head is green on all 12 shards, so that resolved on the following commits.- added a commit that references this issue
on Aug 26, 2026 Delivered across #15100 (
16c104be5) and #15101 (2a148523d). AC4 was the last gap and is now met.# Criterion Evidence Kind 1 An unknown session_idis refused, not defaultedapi/terminal.py:725-727— no-default lookuproute-level as of #15101 (was handler-only) 2 A default config is applied only to a validated session api/terminal.py:761static 3 Session state resolves across workers services/terminal_session_store.py, wired atterminal_handlers.py:1303; cross-worker testterminal_session_store_test.py:41-56unit + wiring 4 The refusal is proven through the real route terminal_websocket_route_test.py::test_unknown_session_is_refusedroute-level AC4 is the one worth explaining. Before #15101 the unknown-session case existed only as a handler-level test (
terminal_websocket_auth_test.py:116-146, which awaitsterminal_websocket(ws, id)directly). That is not a small gap in rigour — it is the same blind spot that let #14998 hide: the terminal WebSocket route returned 500 to every caller while its handler-level guards stayed green throughout, because calling the handler bypasses routing and dependency resolution entirely.The new test drives a real
TestClienthandshake to a fresh UUID and asserts1008on the wire, an accept-spy count of0,_init_terminal_handler.assert_not_awaited(), and — the load-bearing part — acaplogpair:"unknown session_id"present,"is not the owner"absent. Both branches close1008from the same line (api/terminal.py:845), so without that assertion the test cannot distinguish them, and a regression that turned "unknown" into "not the owner" would pass silently.Two contrast mutations, and the second is the interesting one:
Mutation Result Unknown path returns {}instead ofNoneDID NOT RAISE WebSocketDisconnect— the handshake succeeds and the test fails before reaching the log checksLog branches swapped Only the caplogassertion fails.1008, the accept-spy andassert_not_awaitedall still pass — proving that assertion is what carries the distinction rather than decorating itA trap the store's own design creates, handled explicitly.
terminal_session_storefails closed, so an unreachable Redis answers every lookup withNone— every session looks unknown and the route closes1008for entirely the wrong reason. The test therefore reads the realowned_session_idback out of the store first as a positive witness before asserting the fresh UUID misses. A store that round-trips a real session is demonstrably live, so the miss is a genuine absence rather than an outage. The same fixture supplies the "would have been a valid owner" half, so the rejection cannot be attributed to authentication either.Repaired on the way:
terminal_websocket_route_test.pywas at 613 lines against a 600 max with noKNOWN_LARGEentry — a base-red ratchet violation introduced by16c104be5. #15101 split it along the seam its own docstring already described (structural router/dependency-wiring needs no fixtures, fakeredis or auth stubbing; the handshake tests need all three): 613 → 412 plus a new 264-lineterminal_router_dependency_wiring_test.py. No entry added, no ceiling raised.--audit-ceilingson the merged base now reports4872 files scanned, 509 grandfathered, all live and at size, rc 0. Why that violation reached the base with ten required contexts reading green is tracked separately as #15102.Not proven: deployed behaviour. The running install is more than ten commits behind and still carries the pre-#15085 router-level admin
Depends, so terminal WebSocket handshakes still fail there. No criterion here is worded about deployment, so this closes on merged code — but it is not yet true of the host.
Part of #14958.
Problem
api/terminal.py:837—_init_terminal_handlerresolves a session's configuration with adictionary
.get()that defaults to an empty dict, and then proceeds to build and start alive terminal from those defaults. An unknown, expired, deleted or fabricated
session_idistherefore indistinguishable from a valid one, and silently yields a working terminal at
SecurityLevel.STANDARD.Evidence
api/terminal.py:837-851:There is no existence check between the
.get()and theTerminalWebSocketconstruction, andno ownership check.
SecurityLevel.STANDARDis applied as the fallback for a session that wasnever created.
This is the "an absent result reads as a clean result" failure mode: a missing record and a
valid record take the same code path, and the missing one is the more permissive of the two
because it silently adopts defaults.
Note this is a distinct defect from the missing handshake authentication
(#14960). Fixing authentication alone still leaves an authenticated user able to
attach to an arbitrary, non-existent session id and receive a default-configured terminal;
fixing session validation alone still leaves the socket open to unauthenticated callers. Both
need to land.
Proposed approach
close the socket (
1008, or1003for an unknown resource) and return without constructinga handler.
must come from its creation record.
security/session_ownership.py).Risks / constraints
session_manager.session_configsis process-local; confirm whether a session created on oneworker is visible to the worker terminating the WebSocket before treating "absent" as
"nonexistent". If it is not, absent-here does not mean absent-everywhere and the lookup must
go through shared state — this needs checking, not assuming.
Acceptance criteria
session_idthat was never created is rejected; noTerminalWebSocketis constructed and no shell process starts.SecurityLevelto an unknown session.cross-worker lookup is fixed as part of this work, not deferred).
checking for the rejection's presence rather than the absence of an error.
Implementation Order
Wave: 1
Depends on: none
Unblocks: none — independent defect; no downstream task depends on it