fix(remote-control): authenticate the VNC and terminal websockets, deny unknown terminal sessions

[mrveiss]

Thinking Path

Three review passes extended this PR's original scope (#14959/#14960/#14961) with a fourth
finding and several correctness gaps in the ownership gate itself, plus two CI failures caught
by a later review. Each is addressed below; the reasoning that mattered most:

Correction carried over from review, stated plainly so this body does not misstate it:
terminal.py's router carries a router-level dependencies=[Depends(check_admin_permission)]
that does not resolve for WebSocket routes -- check_admin_permission is typed to take an
HTTP Request, which a WebSocket handshake cannot supply, so FastAPI's dependency solver
raises a TypeError before either the check or the route body runs. Verified independently
three ways including a live probe: this currently makes every handshake to
terminal_websocket and ssh_terminal_websocket fail with HTTP 500, for every caller,
authenticated or not. These two routes are not open doors in production today -- they are
broken in a different way, tracked separately as #14998, whose own text requires that the
routing fix not land before this PR does (fixing #14998 first would turn today's 500 into a
genuinely open route the moment this PR's guards are not already there to catch it).
api/vnc_proxy.py's router is a bare APIRouter() with no router-level dependency --
that route (#14959) is unaffected by any of this and was, and remains, genuinely and currently
reachable and unauthenticated. terminal.py is the only WS-bearing router in the backend with
a router-level Depends.

#14991: ssh_terminal_websocket had the identical unauthenticated-accept() shape as
terminal_websocket, and is wired to the primary chat UI (SSHTerminal.vue mounted in
ChatTabContent.vue) -- currently inert per the #14998 500 above, and SSHTerminalWebSocket
itself is an inert deprecation stub regardless (#729/#3383: SSH moved to slm-server). Fixed
anyway, because #14998 explicitly depends on this PR landing first. There is no per-host
permission model anywhere in this codebase to scope a host_id against a caller's permitted
set, and #14958's own umbrella explicitly rules out inventing one here. The strictest reading
available without inventing a new capability model is the router's own already-declared intent
(Depends(check_admin_permission) at router level) -- which is exactly the dependency that
does not resolve for WebSocket routes, per the correction above. So the fix checks admin role
explicitly, in code, for every host_id.

Credential union: authenticate_websocket only reads a JWT from the query string -- the one
credential a browser can present on a WS handshake (no custom headers). A cookie-session,
Authorization-header, or internal-service-key caller had no path through it -- the same
lockout #11016 already fixed for the admin workspace shell. Widened enforce_ws_authentication
to try the query-param JWT first, then get_user_from_request (header/session/dev-header) and
the internal-service key, denying only when all fail. This is a union of the accepted checks
(more ways to prove identity), not a widening of what any one of them accepts. It also
transparently fixed the case of a session created via the internal-service key (owner = "service:slm") becoming permanently unconnectable -- the same key now authenticates the WS
handshake to the same synthetic principal.

Close-code convention: kept close-before-accept() with code 1008 for all three routes,
matching enforce_ws_origin's existing convention in the same file and the original issues'
explicit acceptance criteria ("rejected with close code 1008", "before accept()"). This
coexists with a second convention elsewhere (api/websockets.py, api/live_events.py:
accept-then-close-4001, pinned by tests/test_websockets_auth_reject_12366.py). I did not
unify them -- that is a repo-wide sweep (#14965 territory), and the two conventions serve
different call sites already. The practical cost of keeping close-before-accept: the WHATWG
WebSocket spec requires browsers to normalise the close code (and reason) to 1006 for any
closure before the opening handshake completes, so browser JS can never distinguish "denied"
from "offline" from the close event alone. That is the root cause of the retry-storm finding
below, fixed on the client side instead of by switching conventions.

Ownership-gate correctness (blockers from a second review pass): the gate denies by design
when an entry has no owner, but a second writer of the shared session_configs dict --
SessionManager._register_pty_with_terminal_manager, the Chat Terminal path reached from
POST /api/agent-terminal/sessions -- never stamped one, so it denied even its own creator.
Fixed by threading the authenticated username through create_agent_terminal_session ->
AgentTerminalService.create_session -> SessionManager.create_session down to the write site.
session_configs is a plain in-process dict, wiped on every restart
(api/terminal_handlers.py), so there is no legacy-session migration concern -- every entry
either has today's shape or does not exist yet, which is exactly why deny-by-default is safe.

A related claim -- that the dormant TerminalWindow.vue breaking was "every user, every time"
-- did not hold up: TerminalWindow is exported from a barrel file and used only in Storybook;
nothing mounts it. The TerminalService.ts token fix from the first round is real (reuses the
established buildAuthenticatedWsUrl helper correctly) but is exercised by no live flow --
stated here explicitly rather than implied otherwise.

What Changed

autobot-backend/api/ws_security.py -- enforce_ws_authentication (added in the first
round) now tries three credential sources in order via a new _resolve_ws_user helper:
query-param JWT, the internal-service key, then get_user_from_request (Authorization header /
X-Session-ID / dev header). Denies (close 1008 pre-accept()) only when all three miss.

autobot-backend/api/terminal.py:

• ssh_terminal_websocket (SSH terminal WebSocket accepts unauthenticated connections and starts a live shell on an infrastructure host #14991) -- authenticates via enforce_ws_authentication, then
  requires admin role via is_admin_role (autobot_shared.auth.permissions), before
  accept(). Docstring records why admin-only is the correct default absent a per-host model.
• create_terminal_session continues stamping owner from the authenticated caller
  (first round, unchanged).

autobot-backend/services/agent_terminal/session_manager.py,
autobot-backend/services/agent_terminal/service.py, autobot-backend/api/agent_terminal.py
-- thread owner: str | None (the authenticated creator's username) from
create_agent_terminal_session down through AgentTerminalService.create_session,
SessionManager.create_session, _setup_pty_for_session, to
_register_pty_with_terminal_manager, which now stamps it into the shared
session_manager.session_configs entry api.terminal._lookup_terminal_session reads.

Frontend (autobot-frontend, autobot-plugins, autobot-slm-frontend) -- the SSH terminal
guard needed its live credential-carrying counterpart or it breaks the chat SSH terminal:

• autobot-frontend/src/components/terminal/SSHTerminal.vue (mounted in
  ChatTabContent.vue) -- reuses buildAuthenticatedWsUrl (refactor(frontend/ws): extract shared buildAuthenticatedWsUrl() helper — token plumbing is duplicated and inconsistent across WS services #6700), same helper as the earlier
  TerminalService.ts fix. Refuses to connect client-side with no token.
• autobot-plugins/terminal/src/components/SshTerminal.vue (the shared @autobot/terminal
  package, consumed by both autobot-frontend and autobot-slm-frontend) has no auth store of
  its own -- added an authToken? prop; host apps resolve and pass their own token.
• autobot-frontend/src/components/plugins/TerminalPlugin.vue supplies it from useUserStore.
• autobot-slm-frontend/src/views/tools/admin/TerminalTool.vue supplies it from its own
  stores/auth.ts (a separate JWT domain behind the same SSO backend).

autobot-frontend/src/services/TerminalService.ts -- _handleWsClose no longer
auto-retries a connection that never reached CONNECTED. Browsers cannot surface the real close
code/reason for a pre-accept() rejection (spec-mandated 1006 normalisation), so the fix uses
"did this attempt ever connect" instead of the close code to decide whether a retry makes sense,
removing the 6-attempt storm and the permanently-poisoned reconnectAttempts counter.

autobot-backend/api/simple_terminal_e2e_test.py -- previously connected with a fabricated
session_id and no credential, both now rejected. Creates a real session via
POST /api/terminal/sessions first and presents the SSOT-configured internal-service key (the
canonical credential, reused rather than a parallel path) on both the REST call and the socket.

Docs -- docs/guides/vision-vnc-ui-testing.md, docs/developer/VNC_MCP_ARCHITECTURE.md,
docs/architecture/TERMINAL_ARCHITECTURE_DIAGRAM.md,
docs/architecture/TERMINAL_CONSOLIDATION_ANALYSIS.md -- each documented one of these sockets
with no mention of a credential; added a note describing the authentication (and, for terminal,
ownership) requirement where the socket is documented.

Tests -- api/vnc_proxy_websocket_auth_test.py, api/terminal_websocket_auth_test.py,
api/ssh_terminal_websocket_auth_test.py (new), services/agent_terminal/session_manager_owner_test.py
(new). See Verification.

CI fixes (two red checks caught by review, both on this PR's own additions):

• api/simple_terminal_e2e_test.py -- the session-creation check I added used
  return False, which pytest discards silently; that is a new return-instead-of-assert
  offender and repo_tests/tests_that_return_instead_of_asserting_test.py's
  autobot-backend budget is down-only (was pinned at 75). Converted to
  assert session_id, .... That alone would fail this test on every run with no live
  backend -- CI's python-suite never sets AUTOBOT_TEST_BACKEND_URL -- so it now skips
  loudly first when unconfigured, rather than either silently passing (this file's other,
  pre-existing, untouched checks still do that -- not newly counted, not touched here) or
  failing every unconfigured run. tests_that_return_instead_of_asserting_test.py's
  recorded budget for autobot-backend is lowered 75 -> 73, matching the actual count
  after this fix plus one further pre-existing offender that landed independently via the
  Dev_new_gui merge.
• autobot-frontend/src/components/terminal/SSHTerminal.vue -- editing its <script> for
  the token fix pulled its untouched <style> block into Stylelint's changed-file scope,
  where 10 pre-existing hardcoded hex colors failed color-no-hex. Four already had exact
  matching tokens (--terminal-chrome-bg-alt, --terminal-chrome-bg,
  --terminal-border-subtle, --terminal-text-muted); the other six (connecting/
  connected/error status colors) had none, so extended the existing, deliberately-literal
  TERMINAL COMPONENT TOKENS block in design-tokens.css (tokenise(user): TerminalWindow — 65 hardcoded hexes + 49 px → design tokens (#11515 Task 4.3/4.4) #11859) that already carries
  this exact component family's palette, rather than remapping a connection-status bar onto
  the semantic --color-success/warning/error tokens or hardcoding new values.

Not changed / explicitly decided against:

• Blocker 2 (frontend session-creation wiring for TerminalWindow.vue) -- not fixed. That
  component is unreachable from any live template; wiring it to call createSession() first is
  real work for when it is actually mounted, not a live regression today.
• Finding 3 (internal-service principal WS lockout) -- resolved as a side effect of the
  credential-union fix above, not by a separate change.
• The 14 other unauthenticated @router.websocket routes found in a full sweep -- listed
  below, not touched (per instruction, scoped to Per-route WebSocket auth sweep with a route-level (not file-level) guard #14965). One correction to the sweep the
  coordinator shared: api/advanced_control.py's /ws/desktop/{session_id} is guarded
  (enforce_ws_admin), not unauthenticated -- verified by reading the route directly.

Credential logging fix (flagged by a background security review of this PR's own diff):
before this PR, WebSocket connection URLs carried no credential, so logging them was harmless.
buildAuthenticatedWsUrl (#6700) now puts a JWT in ?token=..., and both useWebSocket.ts
composables (autobot-frontend, autobot-plugins/terminal) and TerminalService.ts log that
full URL on connect and on an invalid-URL error -- turning those into live token leaks into the
browser console.

• New redactUrlForLogging masks the VALUE of token/credential-shaped query params, keeping
  the parameter name visible (?token=REDACTED) so the log line stays useful, and never throws
  -- the invalid-URL branch is exactly where the string may not parse, so a failed new URL()
  falls back to a regex substitution instead of raising. Wired into every log site the review
  flagged, in autobot-frontend/src/composables/useWebSocket.ts,
  autobot-plugins/terminal/src/composables/useWebSocket.ts, and TerminalService.ts.
• Self-initiated during the same check: TerminalService._validateWsUrl throws
  `Invalid WebSocket URL: ${wsUrl}` with the same token-bearing value, and that message
  reaches a further logger.error via _handleConnectCatchError -- redacted the same way.
  baseUrl is always resolved to a valid ws:///wss:// prefix in normal use, so this branch
  is a defensive fix rather than one reachable through connect()'s public happy path today.
• Duplicated (not shared) between autobot-frontend/src/utils/redactUrlForLogging.ts and
  autobot-plugins/terminal/src/utils.ts: @autobot/terminal is a standalone package
  (peerDependencies only) consumed by both autobot-frontend and autobot-slm-frontend, with
  no shared frontend utility package between them to hold one copy in. The plugin's own
  createLogger() is a no-op today (per its own doc comment), so this specific composable has
  no live console exposure yet -- fixed anyway so it cannot start leaking the day that changes.
  Follow-up (flagged by a second, focused security review of the redaction delta itself --
  the redactor's own logic was independently transcribed and run against 11 adversarial inputs and
  confirmed sound): the "not changed" call I made above -- the two composables' catch (err) { logger.error(..., err) } blocks -- was wrong to defer. It is a live leak, not a theoretical one:
  buildAuthenticatedWsUrl does plain string concatenation with no scheme check, so a
  misconfigured base URL (http:// where ws:// was meant) reaches new WebSocket(), which
  throws a SyntaxError embedding the full attempted URL verbatim in every major browser (Chrome:
  "The URL's scheme must be either 'ws' or 'wss'. '' is not allowed."). That raw error, JWT
  included, went to a real console.error.
• New redactErrorForLogging(err) (same two files as redactUrlForLogging) rewrites only an
  Error's .message through the same redactor, preserving .name and .stack so the error
  stays useful for debugging. Wired into autobot-frontend/src/composables/useWebSocket.ts's
  connect() catch (MEDIUM, no upstream scheme validation makes this reachable) and
  TerminalService._handleConnectCatchError (LOW, defensive -- _validateWsUrl already closes
  the wrong-scheme case, but not every WHATWG parse failure). Also applied to the plugin's own
  useWebSocket.ts catch for symmetry, though its createLogger() no-op means it has no live
  exposure yet either, same as its other two call sites.
• Two hardenings to redactUrlForLogging itself, confirmed reachable-in-principle by execution
  during review: (1) a credential in the URL fragment bypassed both branches --
  URLSearchParams never looks at .hash, and the regex fallback required a [?&] prefix, so
  #token=... matched neither. The regex substitution now always runs as a second pass over the
  final string (covering the fragment) rather than only as the new URL() failure fallback. (2)
  "Never throws" was not literally true -- a non-string argument bypassing the TS type system hit
  .replace() on the original uncoerced value in the catch fallback and threw TypeError. Added
  an explicit typeof guard. Both fixes applied to both copies, kept behaviourally identical.

Verification

Local interpreter: Python 3.10.12 (this dev sandbox's /usr/bin/python3; CI is authoritative).

• New/updated tests: pytest api/vnc_proxy_websocket_auth_test.py api/terminal_websocket_auth_test.py api/ssh_terminal_websocket_auth_test.py services/agent_terminal/session_manager_owner_test.py
  -- 11 passed.
• Combined regression run (new tests + existing VNC/desktop-lock/agent-terminal/e2e-smoke
  suites): 91 passed, 0 failed.
• Collection sweep: pytest api/ services/agent_terminal/ -k "terminal or vnc or ws_security or agent_terminal" --collect-only -- 1084 tests collected, no import errors.
• Mutation testing (each guard mutated, tests re-run to confirm failure, then reverted --
  confirmed identical to the pre-mutation file via diff, nothing pushed):
  • ssh_terminal_websocket: removing the auth call and disabling the admin-role check
    independently each fail their respective dedicated tests.
  • _lookup_terminal_session: the existing-session-vs-owner-mismatch discrimination gap a
    reviewer found (both branches exited through the same close(1008), so reverting the
    existence check to .get(id, {}) still passed the old test) is now closed -- the unknown-
    session test asserts the specific "unknown session_id" log line and asserts the "not the
    owner" line is absent; reverting to .get(id, {}) now fails it correctly.
  • SessionManager._register_pty_with_terminal_manager: reverting to the pre-fix shape (no
    owner key written) fails both new owner-stamping tests.
• python3 -m py_compile clean on every edited backend .py file.
• repo_tests/tests_that_return_instead_of_asserting_test.py -- 5 passed (not run before this
  round; adding it to Verification per review feedback -- these tree-wide guards move on any
  PR, not just ones that touch their own file, and must be checked explicitly).
• repo_tests/ in full -- 1217 passed, 4 skipped, 2 xfailed, 6 failed. All 6 failures verified
  pre-existing and unrelated: a tomllib ModuleNotFoundError (this sandbox runs Python 3.10;
  the check needs 3.11+) affecting 4 tests in pip_ignore_scope_test.py; one failure in
  test_ci_import_smoke_paths_14252.py over an unrelated phase_validation.yml inline-Python
  parse; and with_error_handling_single_definition_test.py's tree-wide scan finding zero
  definitions because its exclusion filter ("/.worktrees/" in path) matches this session's own
  checkout path when run from inside a worktree -- confirmed by running the file's other two
  tests, which parse the canonical file directly and pass. None reproduce from a normal branch
  checkout, and none relate to anything in this diff.
• Frontend: no tsc/node_modules/vitest in this sandbox (nothing installed, per policy);
  the pre-push hook attempted vue-tsc and skipped for the same reason. Every .vue/.ts
  change in this PR, including all four new/updated redaction test files, was therefore
  unverified locally -- CI's Unit & Integration Tests job (self-hosted runner,
  frontend-test.yml) is the only thing that has actually executed them. Confirmed green on
  this PR's current head (02deb71a65476e2ec7bdf00cc1292f9aab6bb15b): Detect changed paths,
  Security Scan, Unit & Integration Tests, Build Test and Test Summary all completed with
  conclusion success (checked via the GitHub Actions API against the actual job run, not the
  separate frontend-required-context.yml shim, which is a distinct workflow that correctly
  skips itself whenever this one applies). Manually traced redactUrlForLogging's logic against
  Node's own URL/URLSearchParams (available in this sandbox) to confirm the
  redact-then-fallback behaviour before writing the composable
  tests against it; that is not a substitute for CI actually running the .ts test files.
• CI's green Unit & Integration Tests run confirms all seven frontend redaction tests (the
  original three plus this round's four -- two new cases in the existing composable/service
  spec files, plus the new redactUrlForLogging.test.ts) execute and pass as authored, which
  this sandbox could not confirm on its own. Mutation itself was not pushed (per instruction),
  so the following is reasoning traced through the code, not a CI-executed mutation run: reverting
  any redactUrlForLogging(wsUrl) call back to bare wsUrl puts the raw token back into the
  console.info/console.error spy's captured args, failing the corresponding
  not.toContain(TOKEN) assertion; reverting a redactErrorForLogging(err) call back to bare
  err does the same for the connection-error tests; removing the fragment/typeof guards from
  redactUrlForLogging fails redactUrlForLogging.test.ts's fragment and non-string cases
  directly.

Known gap in this PR's own history: one commit subject
(fix(terminal): update the e2e smoke script...) cites #14989 -- this PR's own number --
instead of an issue. Left as-is rather than rewrite already-reasoned-about history; flagging
for whoever gates the merge.

Remaining unauthenticated @router.websocket routes (report only, not fixed)

Full sweep of all 24 @router.websocket( declarations in autobot-backend; 3 are the routes
this PR fixes (api/vnc_proxy.py:374, api/terminal.py:937,1071), 7 were already guarded
(api/transcripts.py:154, api/websockets.py:787,858, api/live_events.py:384,
api/task_workspace_ws.py:245, api/advanced_control.py:530,572). The remaining 14, verified
unauthenticated by reading each route (not just grepping a marker string):

Route	File:line
/stream	api/intelligent_agent.py:254
/stream	api/voice_stream.py:316
/ws	api/analytics_quality.py:1668
/{operation_id}/progress	api/long_running_operations.py:494
/tail/{filename}	api/logs.py:848
/ws/sessions/{session_id}/presence	api/presence_ws.py:24 (identity from an unverified ?user_id=)
/ws/knowledge/research	api/knowledge_research_ws.py:119
/processes/{process_id}/stream	api/process_management.py:172
/ws/{session_id}	api/overseer_handlers.py:435 (runs commands)
/ws	api/startup.py:127
/ws/realtime	api/analytics.py:675
/ws/analytics/live	api/analytics.py:1098
/realtime	api/monitoring.py:1148
/workflow_ws/{session_id}	services/workflow_automation/routes.py:596

Model Used

Claude Sonnet 5 (claude-sonnet-5)

Closes #14959, #14960, #14961, #14991

[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.

[codecov]

Codecov Report

❌ Patch coverage is 62.50000% with 15 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
autobot-frontend/src/services/TerminalService.ts	11.76%	15 Missing ⚠️

📢 Thoughts on this report? Let us know!

[mrveiss]

code-quality was failing on the ratchet in the down direction — and the cause is worth recording, because it is a trap this repo will hit again.

autobot-backend/api/terminal.py: 1299 lines, under its recorded ceiling of 1300.
Lower the ceiling to 1299 ... the ratchet only turns down, and an unlowered
ceiling re-licenses the lines just cut.

The file measured 1300 locally and 1299 in CI. The missing line was not drift: CI's own Auto-fix: code formatting bot committed a one-line deletion to terminal.py directly on this branch. So the ratchet was correct and my ceiling was stale by exactly the bot's edit — CI had improved the file and then failed the run for not recording the improvement.

Both copies now say 1299 (scripts/python_file_size_known_large.py and repo_tests/python_file_size_ratchet_baseline.py).

Two things nearly went wrong while fixing it, both caught rather than discovered later.

First, git push --force-with-lease refused. The remote had gained two commits I did not have — that formatting auto-fix and a chore(types) regeneration of autobot-frontend/src/types/generated/api.ts (278 insertions, 278 deletions). A plain force-push would have silently deleted both, including the very line-removal the ratchet was asking me to record. They are preserved by cherry-pick onto the rebased branch, not squashed — the commit-subject gate exempts [bot] authors by design.

Second, rebasing onto the new base conflicted in repo_tests/tests_that_return_instead_of_asserting_test.py, where both sides had lowered a budget: base (via #14518/#15041) took autobot-infrastructure 126 → 121, this branch took autobot-backend 75 → 73. Resolved by keeping the lower of each, never the more permissive one — taking either side wholesale would have re-licensed offenders the other side had actually removed. The merged comment now records both reductions and why the rule is "lower of the two".

Verified after the rebase: check_python_file_size.py --audit-ceilings → 4856 files scanned, 509 grandfathered, all live and at size, rc 0 · python_file_size_ratchet_test.py 31 passed · tests_that_return_instead_of_asserting_test.py 5 passed at (73, 121). Head ae98eb880, 0 behind base.