Repository navigation
Terminal WebSocket accepts unauthenticated connections #14960
Description
Activity
- added 10 commits that reference this issue
on Aug 24, 2026 7 remaining items
- added 4 commits that reference this issue
on Aug 25, 2026 Closure audit — staying open. Verified against merged base
Dev_new_gui(2bf56f158, PR #14989)Most of the fix has landed and is good. One criterion is not merely unproven — it is contradicted by the merged base, so this does not close yet.
# Acceptance criterion Evidence on the merged base Verdict 1 Unauthenticated handshake with no Originto/api/terminal/ws/{id}rejected1008; noTerminalWebSocketconstructed, no shell startsautobot-backend/api/terminal.py:839-841—enforce_ws_authenticationbeforeaccept()(:848). Testautobot-backend/api/terminal_websocket_auth_test.py:71-89assertsclose(code=1008),accept.assert_not_awaited(),_init_terminal_handlernever awaitedCode met; the 1008a real caller sees is not — see below2 An authenticated user connecting to another user's session id is rejected terminal.py:711-738_lookup_terminal_sessioncomparesconfig.get("owner")touser.get("username")and returnsNone; owner is stamped at creation from the authenticated caller, never from the client-suppliedrequest.user_id(terminal.py:220-226, viabuild_owner_metadata). Testterminal_websocket_auth_test.py:127-144drivesmalloryagainstalice's session and asserts1008plus the distinct "is not the owner" log lineMet 3 An authenticated owner still connects normally Unit-level only: terminal_websocket_auth_test.py:146-165calls the route function directly and assertsaccept()is reached. Through the real ASGI stack the owner does not connect — see belowNot met 4 Guard test drives the real route and fails if the auth call is removed Test imports the decorated callable from api.terminal import terminal_websocket(:28) and asserts_lookup_terminal_sessionis never even reached (:87), so removing the auth gate fails distinctly from an ownership rejectionMet at function level; it cannot see the routing failure below 5 Test asserts the rejection is observed, not that no exception was raised :85-86asserts the close codeMet Why criterion 3 is not met
autobot-backend/api/terminal.py:172-175still declares:router = APIRouter( tags=["terminal"], dependencies=[Depends(check_admin_permission)], )check_admin_permissionisRequest-typed (autobot-backend/auth_middleware.py:972). FastAPI cannot resolve an HTTP-Requestdependency against a WebSocket scope, so the handshake fails beforeterminal_websocketis entered — every caller, owner included, gets a 500 on the upgrade. That is the outage tracked in #14998, which is still open. This PR deliberately did not touch it (removing the router dependency first would have turned a 500 into a genuinely open socket), which was the right sequencing — but it leaves criterion 3 false on the merged base, and it also means the1008in criteria 1 and 2 is what the function returns, not what a client on the wire observes.The guard tests call the route function directly, so they pass while the route serves nobody. A decorator being present is not the same as a gate that runs.
Second, smaller gap (shared with #14961)
session_manager.session_configsis a plain in-process dict (autobot-backend/api/terminal_handlers.py:1306, instance at:1504). The backend service template runs uvicorn with 4 workers and--limit-max-requestsworker recycling (autobot-infrastructure/autobot-backend/templates/autobot-user-backend.service:18-24). A session created on one worker is invisible to the worker terminating the WebSocket, so "owner still connects normally" additionally fails across workers and across a worker recycle. Tracked in detail on #14961 (its criterion 3).What is outstanding
- bug(terminal): both terminal WebSocket routes 500 on every handshake — a router-level Depends cannot resolve in a WS scope #14998 fixed and verified — both terminal WS routes must actually reach their endpoint function, so an authenticated owner connects and an unauthenticated caller receives
1008on the wire rather than a 500. - A test that exercises the route through the ASGI/router layer, not just the decorated callable — the current guard suite cannot distinguish "guard runs and denies" from "route unreachable".
- Cross-worker session visibility (see Unknown terminal session_id defaults to a live STANDARD-security terminal #14961 criterion 3) before "connects normally" is true for a multi-worker deployment.
Everything else on this issue has landed. Re-audit for closure once #14998 is closed with evidence.
- bug(terminal): both terminal WebSocket routes 500 on every handshake — a router-level Depends cannot resolve in a WS scope #14998 fixed and verified — both terminal WS routes must actually reach their endpoint function, so an authenticated owner connects and an unauthenticated caller receives
Closure audit — staying open, one criterion short. Verified against merged base
Dev_new_gui(4f7c5e299)The blocker from the previous audit is gone. #14998 is closed (PR #15085):
api/terminal.py:174
is nowrouter = APIRouter(tags=["terminal"])with no dependencies owning both@router.websocket
routes,:175admin_router = APIRouter(dependencies=[Depends(check_admin_permission)])carrying
the admin gate for all 20 HTTP routes,:1299router.include_router(admin_router). Criterion 3,
which the merged base previously contradicted, is now met — and met at route level.One gap remains, and it is the same class of gap this issue has been the textbook case of.
# Acceptance criterion Evidence on the merged base Evidence kind Verdict 1 Unauthenticated handshake with no Originrejected1008; noTerminalWebSocketconstructed, no shell startsterminal.py:836enforce_ws_origin,:839enforce_ws_authentication(closes1008—ws_security.py), both beforeaccept()at:848. Testterminal_websocket_route_test.py:348drives a realTestClienthandshake — which sends noOriginheader — and assertsWebSocketDisconnect.code == 1008plus that the patchedacceptrecorded zero calls, so_init_terminal_handler(:851, afteraccept) never runsRoute-level Met 2 An authenticated user connecting to another user's session id is rejected Fix is merged: terminal.py:725-737_lookup_terminal_sessionreturnsNoneunlessconfig["owner"] == user["username"];:846closes1008. Owner is stamped at creation from the authenticated caller viabuild_owner_metadata(:225-229), never from client-supplied input. But the only test isterminal_websocket_auth_test.py:129test_non_owner_is_rejected, which awaits the handler directlyHandler-level only Not met — see below 3 An authenticated owner still connects normally terminal_websocket_route_test.py:332test_authenticated_owner_connects— realTestClienthandshake as the session owner, asserting a genuineaccept()(the spy calls through to the realWebSocket.accept)Route-level Met 4 A guard test drives the real route and fails if the auth call is removed terminal_websocket_route_test.py:348goes throughTestClientagainst a realFastAPIapp mounting the production router. With the auth call removed the caller would reachaccept()and the zero-calls assertion fails.TestRouterLevelDependsBreaksWebSocketScope(:428) additionally pins the router shape behaviourallyRoute-level Met 5 The test asserts the rejection is observed, not merely that no exception was raised :360asserts the close code carried by theWebSocketDisconnectthe client receivesRoute-level Met Why criterion 2 is not yet discharged
A rejection criterion is a statement about what a client experiences. The only test covering it
awaitsterminal_websocket(ws, session_id)directly, bypassing routing and dependency resolution
entirely — the exact test shape that stayed green through the whole of #14998, while every real
handshake500d before the handler was ever entered. A test that exercises the handler cannot see
a routing failure, so it cannot be the evidence for a criterion about the wire.Criteria 1, 3, 4 and 5 all got that route-level treatment in #15085. The non-owner case was the one
left behind.PR #15095 adds it: a
TestClienthandshake on/ws/{session_id}as a non-owner against another
user's session, asserting1008on the wire, thataccept()never ran, and — viacaplog— that the
ownership branch fired rather than #14961's unknown-session branch, since both close1008from
the same line. Contrast mutation (ownership comparison replaced with a never-taken branch): exactly
one test fails, the new one, and the captured log shows a real PTY being allocated for the
non-owner. The other 18 route tests pass throughout — they could not see it.Merging #15095 discharges criterion 2 and nothing else is outstanding on this issue.
What I could not prove
- No host evidence for any criterion. The deployed tree still carries the pre-fix(terminal): stop the router-level admin Depends 500ing both terminal WS handshakes #15085 router
declaration (dependencies=[Depends(check_admin_permission)]at router level), because its last
code-sync predates the fix(terminal): stop the router-level admin Depends 500ing both terminal WS handshakes #15085 merge by roughly two and a half hours. That is ordinary deployment
lag, not a defect — but it means the live system currently still500s every terminal WS
handshake, so criterion 3 is not true of the running deployment yet. Re-check after the next sync. - Cross-worker session visibility remains unfixed.
session_manager.session_configsis still a
plain in-process dict (terminal_handlers.py:1306, instance at:1504) while the backend service
template specifies--workers 4. A session created on one worker is invisible to the worker
terminating the WebSocket, so an owner can be refused1008by the ownership check for a session
that is genuinely theirs. The currently running backend happens to use a single worker, so this
does not bite right now. Tracked as criterion 3 of Unknown terminal session_id defaults to a live STANDARD-security terminal #14961, which is open — not a blocker here,
but "connects normally" is not unconditionally true in a multi-worker deployment until it is fixed.
No sub-issues (
sub_issuesis empty), so nothing else gates closure.Re-audit once #15095 is merged.
- No host evidence for any criterion. The deployed tree still carries the pre-fix(terminal): stop the router-level admin Depends 500ing both terminal WS handshakes #15085 router
- added a commit that references this issue
on Aug 26, 2026 Closure verification — every criterion against merged
Dev_new_guiVerified against the merged base (
3b32f7b9ein history), not against the PR diff or CI.
Evidence kind is stated per criterion, because a handler-level test cannot see a routing
failure — that is exactly how #14998 stayed invisible while this route 500'd for every caller.# Criterion (verbatim) Verdict Evidence in merged base Evidence kind 1 An unauthenticated handshake with no Originheader to/api/terminal/ws/{id}is rejected with close code1008; noTerminalWebSocketis constructed and no shell starts.Met api/ws_security.py:54-65returnsTruewhenOriginis absent, so a no-Origincaller falls through toapi/terminal.py:839→api/ws_security.py:116-123closes1008. Construction happens only atapi/terminal.py:851, downstream ofaccept()at:848.api/terminal_websocket_route_test.py:364-375drives the real ASGI app throughTestClient(which sends noOrigin), asserts1008on the wire and an accept-spy count of0.route-level 2 An authenticated user connecting to another user's session id is rejected. Met Ownership branch: api/terminal.py:729-736; refusal at:843-846, beforeaccept()at:848.api/terminal_websocket_route_test.py:377-392— real handshake asmalloryagainst a session owned byalice:1008, accept count0, andcaplogasserts the"is not the owner"line fired while"unknown session_id"did not. That distinction is load-bearing: both branches close1008from the same line:845, so the wire code alone cannot tell them apart.route-level 3 An authenticated owner still connects normally. Met api/terminal_websocket_route_test.py:348-362— real handshake as the owner, accept-spy count1(the realWebSocket.acceptruns, not a short-circuit). Ownership is stamped from the authenticated creator atapi/terminal.py:225-240viabuild_owner_metadata, never from the client-supplieduser_id.route-level 4 A guard test drives the real route and fails if the auth call is removed. Met The whole of api/terminal_websocket_route_test.pygoes throughstarlette.testclient.TestClientagainst aFastAPIapp mounting the production router (:104-115), so route matching and dependency resolution really run. Deletingapi/terminal.py:839-841leaves:843with nouserto pass, and the clean1008the test asserts no longer happens. Removal-sensitivity is additionally pinned atapi/terminal_websocket_auth_test.py:91-112, whosemock_lookup.assert_not_called()separates an auth refusal from a same-outcome ownership refusal.route-level drive; the mutation-distinction assertion is handler-level 5 The test asserts the rejection is observed, not merely that no exception was raised. Met api/terminal_websocket_route_test.py:373-375and:389-392assertexc_info.value.code == 1008and an accept count of0— a positive observation of the refusal, not the absence of an error.route-level No sub-issues (
sub_issuesendpoint returns empty).Stated plainly: this closes on merged code. The deployed backend has not yet taken this
change, so the running system still behaves as it did before the fix — that is deployment lag
on an already-merged fix, and none of the five criteria above are worded about deployed
behaviour. Rollout remains to be done.Closing.
Part of #14958.
Problem
api/terminal.py:897—terminal_websocketaccepts any WebSocket handshake withoutauthenticating the caller, then attaches the socket to a live terminal session.
The REST surface of the same module is thoroughly authenticated —
create_terminal_session(
:325),list_terminal_sessions(:394),get_terminal_session(:426),delete_terminal_session(:458),setup_ssh_keys(:499),execute_single_command(
:647),send_terminal_input(:689),send_terminal_signal(:720) all carrycurrent_user: dict = Depends(get_current_user). The WebSocket that actually carries theinteractive session does not.
Evidence
api/terminal.py:911-916is the complete gate:enforce_ws_originis anOrigincheck, not authentication — seeapi/ws_security.py, whichdocuments that authorisation is "decided separately by each endpoint's own auth logic" and that
clients omitting
Originare deliberately allowed through. There is no app-level ASGI authmiddleware to catch the omission (
AuthenticationMiddleware,auth_middleware.py:44, is aservice singleton consumed via
Depends, not registered on the app).This compounds with #14961: an unknown
session_idis not rejected either, so thecaller does not even need a session id belonging to someone else.
Reproduction
Connect to
/api/terminal/ws/<any-string>with noOriginheader and no credentials. Thehandshake is accepted and a terminal handler is constructed and started.
Proposed approach
Authenticate before
websocket.accept(), closing with1008on failure, usingauthenticate_websocket()(auth_middleware.py:1061) or the router-level dependency fromTask 4 (#14963).
Then assert ownership: the authenticated user must be the owner of
session_id. Sessionownership machinery already exists in
security/session_ownership.py— reuse it rather thanadding a parallel check.
Risks / constraints
default to deny and let the client re-create.
security/session_ownership.pyconsultsservices/feature_flags.pyfor its enforcementmode; confirm which mode applies here rather than assuming enforcement is on.
Acceptance criteria
Originheader to/api/terminal/ws/{id}isrejected with close code
1008; noTerminalWebSocketis constructed and no shell starts.Implementation Order
Wave: 1
Depends on: none
Unblocks: #14963, #14964