security(desktop): authenticate to VNC server-side, close the VITE_* password leak (#16299)

[mrveiss]

Thinking Path

#16299 (guessable default credentials) names VITE_DESKTOP_VNC_PASSWORD/VITE_TERMINAL_VNC_PASSWORD/VITE_PLAYWRIGHT_VNC_PASSWORD as compiled into every browser's bundle — Vite inlines every import.meta.env.VITE_* reference into the built output regardless of whether the surrounding code path is reachable at runtime, so this is a real leak even though the consuming code (AppConfig.js, useChatStore.ts) is marked DORMANT (#1130, preserved for #5136).

Traced the actual architecture before designing a fix: api.vnc_proxy's websocket_proxy is a pure byte-level relay with zero RFB-protocol awareness — the VNC password challenge-response currently happens client-side, in the browser's noVNC JS, using the leaked value. Owner ruling (ratified across several messages on #16299): the backend answers the real server's challenge itself and offers the browser security-type "None", so noVNC never sees or needs a password.

Two real bugs found by exploring the actual deployment topology before assuming the naive design would work, not by trusting the initial framing:

1. Initially assumed the backend could read the VNC password off a local file (/etc/autobot/vnc-secrets.env / the x11vnc passwd file the ansible vnc role already writes). Traced the actual topology (NetworkConstants.MAIN_MACHINE_IP/BACKEND_API show the backend and the "desktop" VNC type are co-located, but "browser" is a separate VM) and confirmed with the owner: a plaintext env-file read would be a parallel credential path, forbidden by the canonical-secrets-manager rule. Redesigned to register the password through the existing POST /api/v2/secrets/system route (already AUTOBOT_INTERNAL_API_KEY-authenticated) instead.
2. Traced whether "browser" VNC is actually wired through the vnc ansible role at all — tasks/register-credentials.yml's own vnc_type: "desktop" is hardcoded, and no playbook applies this role for the browser node_role today. Rather than fork the mechanism by host, parameterized the new registration task by a vnc_type role default (mirrors api.vnc_proxy.VNC_ENDPOINTS's own keys exactly) so the same task works for a future browser invocation with zero changes — one mechanism, no desktop/browser split, per the owner's "one PR, one agent, per file-set touched" rule. Also found and filed security(vnc): docker-compose deployments have no VNC password provisioning at all #17059: docker-compose deployments have no VNC password provisioning at all (a different, separate gap).

The DES/RFB algorithm itself was verified against three independent sources before writing a line of it, not derived from memory:

• RFC 6143 §7.2.2 (password truncate/pad to 8 bytes).
• LibVNCServer's src/common/d3des.c — its own comment documents the bytebit[] table "has been reversed" from standard DES, confirming the classic VNC per-byte bit-reversal quirk is real, not folklore.
• vncdotool's modern Python client (vncdotool/rfb.py) — uses the identical cryptography.hazmat.decrepit.ciphers.algorithms.TripleDES primitive already available in this repo's pinned cryptography>=50.0.1. Its reverse_bits was cross-checked byte-for-byte against this implementation's across all 256 values, and the full password→key→response pipeline cross-checked end-to-end across 6 password shapes × 5 random challenges.
• cryptography's TripleDES(key*3) in ECB mode was independently verified against the canonical FIPS DES test vector (key 133457799BBCDFF1 → ciphertext 85E813540F0AB405, exact match) before relying on it.

What Changed

• autobot-backend/security/vnc_rfb_auth.py (new): the RFB handshake state machine — authenticate_as_client (answers the real server's VNC-Auth challenge, refuses to proceed if the server doesn't offer VNC Auth at all) and offer_no_auth_as_server (offers the browser security-type "None" only). Fails closed on any protocol mismatch; never includes the password in an exception message.
• autobot-backend/api/vnc_handshake_bridge.py (new): adapts websocket_proxy's two real connections (an aiohttp WebSocket to the real VNC server, a FastAPI WebSocket to the browser) to the handshake module's byte-stream interface, buffering across WS message boundaries (RFB byte boundaries don't align with WS frame boundaries) and surfacing any leftover bytes so the plain relay doesn't drop the start of the framebuffer protocol. get_vnc_password reads the secret via SecretsCoordinator at connection time — fails closed (raises, never falls back to unauthenticated) when nothing is registered.
• autobot-backend/api/vnc_proxy.py: websocket_proxy now calls the handshake before starting the relay; a VncAuthError closes the socket (1011) before either forwarding task starts.
• autobot-slm-backend/ansible/roles/vnc/tasks/main.yml: after generating the VNC password (already existing, openssl rand-based, gated first-run-only), registers it via POST /api/v2/secrets/system using the internal API key already read from slm-secrets.env. Flagged for reviewer confirmation: reaches the backend via {{ main_host }}:{{ backend_port }}, mirroring NetworkConstants.BACKEND_API's Python-side equivalent — backend_port's own comment elsewhere says "loopback only", true for the co-located desktop host, unconfirmed for the browser VM's cross-host case.
• autobot-slm-backend/ansible/roles/vnc/defaults/main.yml: new vnc_type: "desktop" default, naming the secret this role registers.
• Frontend (AppConfig.js, ssot-config.ts, defaults.js, useChatStore.ts, ssot-config.spec.ts): every password field/env-var reference removed from the VNC config paths (all in code explicitly marked DORMANT — preserved for feat(browser): region-marking + AI-assisted scraping templates on InteractiveScreenshot #5136, not deleted). VNCConfig interface updated to match.
• repo_tests/vnc_password_not_in_frontend_source_16299_test.py (new): a _reach-floored sweep of all 1,411 frontend .vue/.ts/.js source files asserting none reference a VITE_*_VNC_PASSWORD env var — a source-level guard (this environment has no node_modules to run an actual build) that is a strictly necessary precondition for the string never reaching a built bundle. Verified it actually fails on a reintroduced reference before relying on it.

Out of scope, filed separately (per the owner's explicit direction this PR must not silently absorb them):

• security(desktop): the desktop WebSocket admits any signed-in user with full view and input — no role or owner check #17054 — the desktop WebSocket admits any signed-in user, not scoped to admin/owner. This PR does not change who can open the socket.
• security(vnc): both VNC launch paths bind the RFB port on every interface although only loopback websockify uses it #17055 — both VNC launch paths bind the RFB port on every interface; needs a decision on whether direct-client access is relied on before hardening.
• security(vnc): docker-compose deployments have no VNC password provisioning at all #17059 — docker-compose deployments have no VNC password provisioning at all (a missing feature, separate from this credential-leak fix).

Verification

$ python3 -m pytest autobot-backend/security/vnc_rfb_auth_test.py autobot-backend/api/vnc_handshake_bridge_test.py autobot-backend/api/vnc_proxy_handshake_wiring_test.py autobot-backend/api/vnc_proxy_control_lock_test.py autobot-backend/api/vnc_proxy_websocket_auth_test.py repo_tests/vnc_password_not_in_frontend_source_16299_test.py repo_tests/python_file_size_ratchet_test.py repo_tests/guard_reach_meta_test.py -q
83 passed
$ ansible-playbook --syntax-check autobot-slm-backend/ansible/setup-user-backend.yml
no errors (full task-graph parse, not just YAML)

No live VNC server or ansible target host is available in this environment. The DES/RFB crypto and protocol logic is proven correct against fake in-memory peers implementing exactly RFC 6143's wire format (not just self-consistent — cross-checked against an independent reference implementation, see Thinking Path), including a negative control (an upstream server offering only "None" is refused) and a fail-closed test (missing secret refuses the connection). The WS-message-boundary buffering is proven with fake WebSocket doubles that deliberately split reads and leave one extra byte past the handshake boundary on each leg. websocket_proxy's wiring (handshake failure → close 1011 before any relay; success → relay starts with the right arguments) is proven against the real route object with the aiohttp/FastAPI session chain mocked. No frontend build tooling (node_modules) is available locally; the source-level guard is the strongest check available in this environment and would need a CI build-output grep to additionally verify (flagged, not added — no workflow config change made).

Model Used

Claude Sonnet 5

Acceptance Criteria

• No VITE_* variable carries a VNC credential; auth moves server-side (api/vnc_handshake_bridge.py) — the browser never holds or sends a password
• Remaining security: guessable default credentials ship in templates, ansible defaults and the compose file #16299 ACs (the .env* template edits, pending the owner's protect-files.sh patch; the secrets-baseline guard and generate-on-first-run pieces) are tracked in separate PRs on security: guessable default credentials ship in templates, ansible defaults and the compose file #16299 — not this PR

Single-issue rationale

This PR closes no issue -- #16299 has several independent AC slices already split across separate PRs (this one, #17037's guessable-defaults slice, and the pending .env*/protect-files piece), and #17054/#17055/#17059 are explicitly out of scope, filed separately per the owner's direction not to silently absorb them. A security-critical, from-scratch protocol implementation (new DES/RFB module, four review rounds) is also large and risky enough on its own that batching it with unrelated issues would make review harder, not easier.

Issue Link

Refs #16299 (one of several slices; #16299 stays open until every AC is met)
Refs #17054, #17055, #17059 (scoped out, see What Changed)

Changelog Fragment

changelog/unreleased/16299-vnc-server-side-auth.md (type: security, scope: desktop)

Checklist

• Tests added/updated and passing locally
• Changelog fragment added
• No hardcoded values introduced
• Commit message follows <type>(scope): <description> (#issue)

[coderabbitai]

Warning

Review limit reached

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: cf90c5fd-e509-4215-a678-afcfb3024480

📥 Commits

Reviewing files that changed from the base of the PR and between 88ee31c and 36862bc.

📒 Files selected for processing (20)

• .secrets.baseline
• autobot-backend/api/vnc_handshake_bridge.py
• autobot-backend/api/vnc_handshake_bridge_test.py
• autobot-backend/api/vnc_proxy.py
• autobot-backend/api/vnc_proxy_handshake_wiring_test.py
• autobot-backend/security/vnc_rfb_auth.py
• autobot-backend/security/vnc_rfb_auth_test.py
• autobot-frontend/src/config/AppConfig.js
• autobot-frontend/src/config/__tests__/ssot-config.spec.ts
• autobot-frontend/src/config/defaults.js
• autobot-frontend/src/config/ssot-config.ts
• autobot-frontend/src/stores/useChatStore.ts
• autobot-slm-backend/ansible/roles/vnc/defaults/main.yml
• autobot-slm-backend/ansible/roles/vnc/tasks/main.yml
• autobot-slm-backend/ansible/roles/vnc/tasks/register-vnc-password.yml
• autobot-slm-backend/ansible/setup-user-backend.yml
• changelog/unreleased/16299-vnc-server-side-auth.md
• repo_tests/glob_declared_reads_15900_test.py
• repo_tests/vnc_ansible_secrets_registration_16299_test.py
• repo_tests/vnc_password_not_in_frontend_source_16299_test.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

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

[github-actions]

Notice: 29 open PRs — past the runaway threshold (25)

There is no PR queue limit, and this is not a request to defer this PR. Work proceeds one issue at a time without a cap on open PRs; review capacity is the constraint.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

• fix(security): audit the orphan-secret-repair test fixture in .secrets.baseline #17067 fix(security): audit the orphan-secret-repair test fixture in .secrets.baseline
• chore(deps): bump the uv group across 1 directory with 2 updates #17066 chore(deps): bump the uv group across 1 directory with 2 updates
• chore(vehicle): land ChatInterface fixes — 2 approved PRs (#16472, #16911) #17064 chore(vehicle): land ChatInterface fixes — 2 approved PRs (feat(collaboration): surface pending invitations in the collaboration panel (#16470) #16472, fix(chat): the init stops at its first await once the component unmounts (#16274) #16911)
• chore(vehicle): land document-parser stack — 3 approved PRs (#16783, #16788, #16790) #17063 chore(vehicle): land document-parser stack — 3 approved PRs (feat(documents): verify ZIP-based office and ODF formats by content (#16773) #16783, feat(kb): accept spreadsheet, presentation and OpenDocument uploads (#16775) #16788, feat(knowledge): DocumentExtractor handles .csv/.json/.html like the GUI upload path (#16785) #16790)
• chore(vehicle): land ansible provisioning fix — 1 approved PR (#17037) #17062 chore(vehicle): land ansible provisioning fix — 1 approved PR (security: guessable-default fixes for #16299 (baseline-reasons guard, generate-on-first-run secrets) #17037)
• feat(backend): orphan-storage detector framework + code-source clone detector (#17038, #17039) #17050 feat(backend): orphan-storage detector framework + code-source clone detector (Umbrella: SLM Data hygiene page — detect and safely clean up orphaned data across services #17038, feat(backend): orphan-storage detector framework + code-source clone detector, delete through the owning service (#17038) #17039)
• chore(vehicle): land WebSocket auth stack — 4 approved PRs (#16891, #16939, #17018, #17027) #17048 chore(vehicle): land WebSocket auth stack — 4 approved PRs (security(websocket): auth token travels via Sec-WebSocket-Protocol, not the URL (#16457) #16891, security(websocket): every authenticated WebSocket echoes the bearer subprotocol through one helper (#16457) #16939, fix(security): authenticate nine WebSocket endpoints before accept, owner-scope the overseer and workflow sockets, admin-only /api/long-running (#17009, #17010) #17018, fix(ops): long-running operations — create works and records its creator, creator-or-admin access, the panel reads real data, unimplemented starts answer 501 (#17017) #17027)
• chore(vehicle): land CI & hooks infra — 6 approved PRs (#16938, #17032, #16952, #16958, #16882, #16959) #17047 chore(vehicle): land CI & hooks infra — 6 approved PRs (fix(ci): chain formatter dispatch after the branch guard in pre-commit (#16923) #16938, chore(hooks): track the commit-msg hook, install it, and enforce the subject convention (#17029) #17032, fix(tests): prune SKIP dirs during the walk, not after rglob returns (#16601) #16952, test(security): a delegated subagent runs a write its parent is held from (#16950) #16958, security(infra): stop binding ChromaDB to every interface by default (#15317) #16882, fix(infra): gate doc-sync to main-at-origin-tip, move it off post-commit (#16934) #16959)
• chore(vehicle): land security & KB hardening — 5 approved PRs (#16930, #16895, #16937, #16954, #16989) #17046 chore(vehicle): land security & KB hardening — 5 approved PRs (security(rag): route the chat's RAG path through the content firewall (#16771) #16930, security(secrets): content-scanning credential redactor -- catches a credential in free text (#13708) #16895, fix(security): match injection patterns against what a model reads, not the raw text (#16354) #16937, fix(multimodal): the response says whether a result was stored, and if not, why (#16926) #16954, feat(auth): SLM API keys only on declared routes, 403 elsewhere, every key request audited; main backend accepts none (#16040, #16294, #16999) #16989)
• fix(hooks): use stdin JSON instead of $TOOL_ARGS in settings.json hooks (#17024) #17044 fix(hooks): use stdin JSON instead of $TOOL_ARGS in settings.json hooks (Suspected dead hooks in .claude/settings.json: $TOOL_ARGS may not be a real env var #17024)
• security: guessable-default fixes for #16299 (baseline-reasons guard, generate-on-first-run secrets) #17037 security: guessable-default fixes for security: guessable default credentials ship in templates, ansible defaults and the compose file #16299 (baseline-reasons guard, generate-on-first-run secrets)
• chore(hooks): track the commit-msg hook, install it, and enforce the subject convention (#17029) #17032 chore(hooks): track the commit-msg hook, install it, and enforce the subject convention (commit-msg hook should reject a subject with no issue reference #17029)
• fix(ops): long-running operations — create works and records its creator, creator-or-admin access, the panel reads real data, unimplemented starts answer 501 (#17017) #17027 fix(ops): long-running operations — create works and records its creator, creator-or-admin access, the panel reads real data, unimplemented starts answer 501 (long-running: the four start routes cannot create an operation — create_operation gets estimated_items/context it does not accept (TypeError → 500) #17017)
• fix(security): authenticate nine WebSocket endpoints before accept, owner-scope the overseer and workflow sockets, admin-only /api/long-running (#17009, #17010) #17018 fix(security): authenticate nine WebSocket endpoints before accept, owner-scope the overseer and workflow sockets, admin-only /api/long-running (security(ws): nine WebSocket endpoints accept without authentication — PTY execution, workflow control and KB writes among them #17009, security: /api/long-running has no authentication — anyone can start test runs, scans and KB population, or cancel any operation #17010)
• security(kb): re-scope #16985 to a guard -- every connector already reaches store_fact #17016 security(kb): re-scope security(secrets): wire credential redaction into the remaining 11 KB connectors (#13708 follow-up) #16985 to a guard -- every connector already reaches store_fact
• feat(agents): one-shot idle notice (#16949) #17015 feat(agents): one-shot idle notice (feat(agents): subscribe once to be told when a peer is next idle #16949)
• feat(ci): add an open-PR cap to the pre-push gate (#17006) #17008 feat(ci): add an open-PR cap to the pre-push gate (Open-PR cap: refuse new-branch pushes at/above AUTOBOT_OPEN_PR_CAP (default 40) #17006)
• security(websocket): authenticate /api/intelligent_agent/stream before processing (#17000) #17003 security(websocket): authenticate /api/intelligent_agent/stream before processing (security(websocket): /api/intelligent_agent/stream runs agent goals with no authentication — an unauthenticated bypass of POST /process #17000)
• fix(agents): the peer channel delivers to the recipient, not the sender; broadcast reaches every other agent (#16986) #17001 fix(agents): the peer channel delivers to the recipient, not the sender; broadcast reaches every other agent (agents: the peer channel never delivers to the recipient — a request lands in the sender's own inbox and the sender answers itself #16986)
• feat(agents): peer-to-peer messaging and idle notice (#16948, #16949) #16996 feat(agents): peer-to-peer messaging and idle notice (feat(agents): deliver peer messages at the recipient's next turn, not mid-task #16948, feat(agents): subscribe once to be told when a peer is next idle #16949)
• fix(test): give result.metadata a real dict in 3 multimodal perf/scalability mocks (#16990) #16995 fix(test): give result.metadata a real dict in 3 multimodal perf/scalability mocks (bug(test): 3 multimodal performance/scalability tests fail — metadata Mock lacks __setitem__ since #15234 #16990)
• feat(auth): SLM API keys only on declared routes, 403 elsewhere, every key request audited; main backend accepts none (#16040, #16294, #16999) #16989 feat(auth): SLM API keys only on declared routes, 403 elsewhere, every key request audited; main backend accepts none (API key scopes are stored and displayed but never enforced — keys inherit full user authority #16040, feat(auth): decide which routes accept API keys, and wire X-API-Key through the #16040 mechanism #16294, docs(api): the public API docs tell integrators to send a user API key to the main backend, which accepts none #16999)
• fix(ci): bump the mcp CI floor mirror to match the backend's declared floor (#16449) #16983 fix(ci): bump the mcp CI floor mirror to match the backend's declared floor (chore(mcp): autobot-backend/mcp/ shadows the real mcp PyPI SDK, making it unreachable by name #16449)
• fix(deploy): enable rendered nginx sites on self-update, not just full provision #16980 fix(deploy): enable rendered nginx sites on self-update, not just full provision
• feat(agents): capture a terminal session's tenant at creation (#16975) #16978 feat(agents): capture a terminal session's tenant at creation (feat(agents): capture a terminal session's tenant at creation — it cannot be recovered afterwards #16975)
• feat(security): bound every Company OS run by its org role, per adapter (#16950) #16974 feat(security): bound every Company OS run by its org role, per adapter (security(agents): an agent refused an action can get another agent to do it — no per-peer identity #16950)
• fix(deploy): the user-frontend build sites publish through a staged swap (#15603) #16970 fix(deploy): the user-frontend build sites publish through a staged swap (bug(deploy): the user-frontend build sites still publish their own failures, and three SLM build sites sit outside the shared staged publish #15603)
• fix(security): A2A trust keyed on the verified pair, capabilities enforced where they apply (#16957, #16950) #16969 fix(security): A2A trust keyed on the verified pair, capabilities enforced where they apply (security(a2a): three of four trust-matrix capabilities are enforced nowhere — a level that denies them denies nothing #16957, security(agents): an agent refused an action can get another agent to do it — no per-peer identity #16950)
• fix(security): a delegated agent inherits its parent's authority; the originator survives relays (#16950) #16966 fix(security): a delegated agent inherits its parent's authority; the originator survives relays (security(agents): an agent refused an action can get another agent to do it — no per-peer identity #16950)

If the queue is draining normally, ignore this. Otherwise:

1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.