diff --git a/agent/redact.py b/agent/redact.py index 9c38cd571d..f23b6e03c1 100644 --- a/agent/redact.py +++ b/agent/redact.py @@ -29,7 +29,6 @@ logger = logging.getLogger(__name__) # never logged, cleared with the process. _VAULT_REDACTION_VALUES: set = set() _VAULT_REDACTION_LOCK = threading.Lock() -_VAULT_REDACTION_MIN_LEN = 4 # avoid pathological scrubs on 1-3 char values def register_vault_redaction_value(value) -> None: @@ -37,14 +36,16 @@ def register_vault_redaction_value(value) -> None: Called by the vault fill path for every secret value it injects into a page, BEFORE the injection happens, so no later browser tool result can - echo the value back into model context. + echo the value back into model context. Also registers the form a text + input normalizes it to (CR/LF stripped), since that is what the page holds. """ - if not isinstance(value, str): - return - if len(value) < _VAULT_REDACTION_MIN_LEN: + if not isinstance(value, str) or not value: return with _VAULT_REDACTION_LOCK: _VAULT_REDACTION_VALUES.add(value) + normalized = value.replace("\r", "").replace("\n", "") + if normalized: + _VAULT_REDACTION_VALUES.add(normalized) def redact_registered_vault_values(text: str) -> str: @@ -52,7 +53,7 @@ def redact_registered_vault_values(text: str) -> str: if not isinstance(text, str) or not text: return text with _VAULT_REDACTION_LOCK: - values = list(_VAULT_REDACTION_VALUES) + values = sorted(_VAULT_REDACTION_VALUES, key=len, reverse=True) # longest first: a substring never shadows its superstring for value in values: if value in text: text = text.replace(value, "«redacted-vault-secret»") diff --git a/agent/vault_backends/base.py b/agent/vault_backends/base.py index 26f84bdbcb..5886635796 100644 --- a/agent/vault_backends/base.py +++ b/agent/vault_backends/base.py @@ -62,6 +62,22 @@ def run_with_stdin_secret(argv: Sequence[str], *, env: Dict[str, str], secret: s raise RuntimeError(f"failed to invoke {label}: {exc}") from exc +def run_with_secret_env(argv: Sequence[str], *, env: Dict[str, str], secret_env: str, secret: str, timeout: float, + label: str) -> subprocess.CompletedProcess: + """Run a manager CLI whose non-interactive contract reads the secret from a named env var. + The variable is set on the child's environment only (never argv, never our process).""" + child_env = dict(env) + child_env[secret_env] = secret + try: + return subprocess.run( # noqa: S603 — argv list, no shell + list(argv), env=child_env, stdin=subprocess.DEVNULL, capture_output=True, text=True, + encoding="utf-8", errors="replace", timeout=timeout) + except subprocess.TimeoutExpired as exc: + raise RuntimeError(f"{label} unlock timed out after {timeout:.0f}s") from exc + except OSError as exc: + raise RuntimeError(f"failed to invoke {label}: {exc}") from exc + + def _cfg() -> Dict: from hermes_cli.config import load_config_readonly cfg = load_config_readonly().get("vault") or {} diff --git a/agent/vault_backends/bitwarden.py b/agent/vault_backends/bitwarden.py index 66f1095caa..697c490a7d 100644 --- a/agent/vault_backends/bitwarden.py +++ b/agent/vault_backends/bitwarden.py @@ -2,7 +2,7 @@ This is the personal/org *password* vault (``bw``), distinct from the Bitwarden Secrets Manager (``bws``) source that hydrates API keys at startup. -Unlock: ``bw unlock --raw`` with the master password on stdin mints a +Unlock: ``bw unlock --raw --passwordenv VAR`` (the CLI rejects a piped password) mints a ``BW_SESSION`` token. List: ``bw list items`` filtered to type=1 (login) with a URI. Resolve: ``bw get password ``. """ @@ -19,7 +19,7 @@ from typing import Dict, List, Optional from agent.secret_sources.base import run_cli, scrub_ansi from agent.vault_backends import unlock as _unlock -from agent.vault_backends.base import LoginBackend, UnlockRequired, run_with_stdin_secret +from agent.vault_backends.base import LoginBackend, UnlockRequired, run_with_secret_env from agent.vault_store import VaultItemMeta, normalize_origin logger = logging.getLogger(__name__) @@ -56,8 +56,11 @@ class BitwardenLoginBackend(LoginBackend): return _unlock.is_unlocked(self.name) def unlock(self, master_password: str) -> None: - proc = run_with_stdin_secret([str(self._bw()), "unlock", "--raw", "--nointeraction"], - env=self._env(None), secret=master_password, timeout=_TIMEOUT, label="bw") + # bw refuses a piped password ("Master password is required"); its non-interactive contract is + # --passwordenv: the variable exists only in the child's environment, never in argv or ours. + proc = run_with_secret_env([str(self._bw()), "unlock", "--raw", "--nointeraction", "--passwordenv", "HERMES_BW_MASTER"], + env=self._env(None), secret_env="HERMES_BW_MASTER", secret=master_password, + timeout=_TIMEOUT, label="bw") token = (proc.stdout or "").strip() if proc.returncode != 0 or not token: err = scrub_ansi(proc.stderr or "").strip()[:200] diff --git a/agent/vault_backends/onepassword.py b/agent/vault_backends/onepassword.py index a700781904..bb1799c4c8 100644 --- a/agent/vault_backends/onepassword.py +++ b/agent/vault_backends/onepassword.py @@ -35,8 +35,9 @@ class OnePasswordLoginBackend(LoginBackend): def __init__(self, cfg: Optional[Dict] = None): self.cfg = cfg or {} + from agent.secret_scope import get_secret env_name = str(self.cfg.get("service_account_token_env") or "OP_SERVICE_ACCOUNT_TOKEN") - self._service_token = os.environ.get(env_name, "") or "" + self._service_token = get_secret(env_name, "") or "" # ── auth ──────────────────────────────────────────────────────────────── diff --git a/agent/vault_backends/unlock.py b/agent/vault_backends/unlock.py index 5f297ef6f0..49048c3505 100644 --- a/agent/vault_backends/unlock.py +++ b/agent/vault_backends/unlock.py @@ -22,7 +22,7 @@ from typing import Callable, Dict, Optional _IDLE_TTL_S = 30 * 60 _lock = threading.Lock() -_sessions: Dict[str, tuple[str, float]] = {} # backend name → (token, last_used) +_sessions: Dict[tuple[str, str], tuple[str, float]] = {} # (profile home, backend) → (token, last_used) _callback_tls = threading.local() UnlockPrompt = Callable[[str, str], str] # (backend_name, display_name) -> master password ("" = cancelled) @@ -37,35 +37,55 @@ def get_unlock_prompt_callback() -> Optional[UnlockPrompt]: return getattr(_callback_tls, "prompt", None) -def get_session_token(backend: str) -> Optional[str]: +def _key(backend: str) -> tuple[str, str]: + # Tokens are profile-scoped: a Desktop gateway hosts several profiles in one process and + # profile B must never reuse (or lock) profile A's manager session. + from hermes_constants import get_hermes_home + return (str(get_hermes_home()), backend) + + +def _live(backend: str, *, touch: bool) -> Optional[str]: + key = _key(backend) with _lock: - entry = _sessions.get(backend) + entry = _sessions.get(key) if entry is None: return None token, last = entry if time.monotonic() - last > _IDLE_TTL_S: - del _sessions[backend] + del _sessions[key] return None - _sessions[backend] = (token, time.monotonic()) + if touch: + _sessions[key] = (token, time.monotonic()) return token +def get_session_token(backend: str) -> Optional[str]: + """Token for a real manager call; refreshes the idle timer.""" + return _live(backend, touch=True) + + def store_session_token(backend: str, token: str) -> None: with _lock: - _sessions[backend] = (token, time.monotonic()) + _sessions[_key(backend)] = (token, time.monotonic()) def lock(backend: Optional[str] = None) -> None: - """Forget one backend's session (or every one when *backend* is None).""" + """Forget the current profile's session for one backend (or all of them when None).""" + home = _key("")[0] with _lock: - if backend is None: - _sessions.clear() - else: - _sessions.pop(backend, None) + for key in [k for k in _sessions if k[0] == home and (backend is None or k[1] == backend)]: + del _sessions[key] + + +def lock_all_profiles() -> None: + """Process shutdown: drop every token.""" + with _lock: + _sessions.clear() def is_unlocked(backend: str) -> bool: - return get_session_token(backend) is not None + """Status probe: does NOT extend the idle TTL (only real manager calls do).""" + return _live(backend, touch=False) is not None def can_prompt_here() -> bool: diff --git a/agent/vault_login_classifier.py b/agent/vault_login_classifier.py index 25952d3d02..21438e7d87 100644 --- a/agent/vault_login_classifier.py +++ b/agent/vault_login_classifier.py @@ -133,9 +133,16 @@ def select_password_fill( # JS expression evaluated in the page to inspect candidate input controls. # Ported from OpenInstinct's nativeLoginControlInspectionExpression. +# Inspection stamps every input with its index under a per-inspection attribute; the fill script +# resolves targets by that stamp instead of re-querying by position, so a DOM that reflows between +# inspect and fill (late-mounted inputs, cookie banners) cannot redirect the password into the wrong field. +INSPECTION_STAMP_ATTR = "data-hermes-vault-slot" + LOGIN_CONTROL_INSPECTION_JS = """(() => { const elements = Array.from(document.querySelectorAll("input")); const forms = Array.from(document.forms); + document.querySelectorAll("[data-hermes-vault-slot]").forEach((n) => n.removeAttribute("data-hermes-vault-slot")); + elements.forEach((element, index) => element.setAttribute("data-hermes-vault-slot", String(index))); const out = elements.flatMap((element, index) => { if (element.disabled || element.readOnly) return []; if (["hidden", "submit", "button", "reset", "file", "image", "checkbox", "radio"].includes(element.type)) return []; @@ -190,11 +197,10 @@ def build_fill_js(fills: List[Dict[str, Any]], expected_origin: str) -> str: " return JSON.stringify({ refused: \"origin_changed\", found: window.location.origin });\n" " }\n" f" const fills = {payload};\n" - " const elements = Array.from(document.querySelectorAll(\"input\"));\n" " let filled = 0;\n" " for (const f of fills) {\n" - " const el = elements[f.index];\n" - " if (!el) continue;\n" + " const el = document.querySelector('input[data-hermes-vault-slot=\"' + f.index + '\"]');\n" + " if (!el || el.type !== \"password\") continue;\n" " try {\n" " el.focus();\n" " const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, \"value\");\n" @@ -204,6 +210,7 @@ def build_fill_js(fills: List[Dict[str, Any]], expected_origin: str) -> str: " if (el.value.length > 0) filled += 1;\n" " } catch (e) { /* skip */ }\n" " }\n" + " document.querySelectorAll(\"[data-hermes-vault-slot]\").forEach((n) => n.removeAttribute(\"data-hermes-vault-slot\"));\n" " return JSON.stringify({ filled });\n" "})()" ) diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/input-requests.ts b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/input-requests.ts index 1e023118bb..38e3155a6d 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/input-requests.ts +++ b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/input-requests.ts @@ -12,7 +12,14 @@ import { import { $gateway } from '@/store/gateway' import { setMcpSetupRequest } from '@/store/mcp-setup' import { dispatchNativeNotification } from '@/store/native-notifications' -import { receiveApprovalRequest, setSecretRequest, setSudoRequest, setVaultUnlockRequest } from '@/store/prompts' +import { + $vaultUnlockRequests, + clearVaultUnlockRequest, + receiveApprovalRequest, + setSecretRequest, + setSudoRequest, + setVaultUnlockRequest +} from '@/store/prompts' import { requestScrollToBottom } from '@/store/thread-scroll' import type { GatewayEventContext } from './types' @@ -157,6 +164,17 @@ export function handleInputRequestEvent(ctx: GatewayEventContext): boolean { return true } + if (event.type === 'vault.unlock.expire') { + const requestId = typeof payload?.request_id === 'string' ? payload.request_id : '' + const request = sessionId ? $vaultUnlockRequests.get()[sessionId] : undefined + + if (requestId && request && request.requestId === requestId) { + clearVaultUnlockRequest(sessionId, requestId) + } + + return true + } + if (event.type === 'clarify.expire') { if (!sessionId) { return true diff --git a/apps/desktop/src/app/settings/vault-settings.tsx b/apps/desktop/src/app/settings/vault-settings.tsx index 30eec37dad..a9d5e1c639 100644 --- a/apps/desktop/src/app/settings/vault-settings.tsx +++ b/apps/desktop/src/app/settings/vault-settings.tsx @@ -1,6 +1,6 @@ import { useStore } from '@nanostores/react' import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query' -import { useCallback, useEffect, useMemo, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { useSearchParams } from 'react-router' import { useGatewayRequest } from '@/app/gateway/hooks/use-gateway-request' @@ -195,9 +195,17 @@ export function VaultSettings() { setUnlockError(null) }, []) + // The master password never becomes mutation *variables* (react-query retains those in its + // cache after the dialog closes); it lives in a ref that the mutationFn consumes and wipes. + const pendingMasterPassword = useRef('') + const unlockSource = useMutation({ - mutationFn: ({ name, password }: { name: VaultSourceName; password: string }) => - requestGateway<{ unlocked: boolean }>('vault.unlock', { name, password }), + mutationFn: ({ name }: { name: VaultSourceName }) => { + const password = pendingMasterPassword.current + pendingMasterPassword.current = '' + + return requestGateway<{ unlocked: boolean }>('vault.unlock', { name, password }) + }, onSuccess: (_result, { name }) => { triggerHaptic('submit') const source = externalSources.find(s => s.name === name) @@ -479,7 +487,9 @@ export function VaultSettings() { e.preventDefault() if (unlockTarget && masterPassword) { - unlockSource.mutate({ name: unlockTarget.name, password: masterPassword }) + pendingMasterPassword.current = masterPassword + setMasterPassword('') + unlockSource.mutate({ name: unlockTarget.name }) } }} > diff --git a/apps/desktop/src/components/prompt-overlays.tsx b/apps/desktop/src/components/prompt-overlays.tsx index bd773423dd..875edfb9e3 100644 --- a/apps/desktop/src/components/prompt-overlays.tsx +++ b/apps/desktop/src/components/prompt-overlays.tsx @@ -28,6 +28,8 @@ import { sessionSudoRequest, sessionVaultUnlockRequest } from '@/store/prompts' +import { ambientRequestFor } from '@/store/session-gone-latch' +import { requestForOwnedSession } from '@/store/session-states' // Renders the modal mid-turn prompts the gateway raises and waits on: sudo // password and skill secret capture. Dangerous-command / execute_code approval @@ -278,10 +280,14 @@ function VaultUnlockDialog({ sessionId }: { sessionId: string | null }) { setSubmitting(true) try { - await gateway.request<{ status?: string }>('vault.unlock.respond', { - request_id: request.requestId, - password - }) + // A master password must reach the backend that raised the prompt, not whatever + // gateway is foreground right now (background profile tiles have their own socket). + await requestForOwnedSession<{ status?: string }>( + request.sessionId, + ambientRequestFor(gateway), + 'vault.unlock.respond', + { request_id: request.requestId, password } + ) triggerHaptic('submit') clearVaultUnlockRequest(request.sessionId, request.requestId) } catch (error) { diff --git a/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx b/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx new file mode 100644 index 0000000000..d85c98dd55 --- /dev/null +++ b/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx @@ -0,0 +1,53 @@ +import { cleanup, fireEvent, render, waitFor } from '@testing-library/react' +import { afterEach, expect, it, vi } from 'vitest' + +import { stubResizeObserver } from '@/test/jsdom' + +const gatewayMocks = vi.hoisted(() => ({ + requestGatewayForAgent: vi.fn(async () => ({ status: 'ok' })) +})) + +vi.mock('@/store/gateway', async importActual => ({ + ...(await importActual>()), + requestGatewayForAgent: gatewayMocks.requestGatewayForAgent +})) +vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) +vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) + +import { PromptOverlays } from '@/components/prompt-overlays' +import { $gateway } from '@/store/gateway' +import { $profiles } from '@/store/profile' +import { clearAllPrompts, sessionVaultUnlockRequest, setVaultUnlockRequest } from '@/store/prompts' +import { $activeSessionId, _resetSessionOwnerHintsForTests, setSessionOwnerHint } from '@/store/session' + +stubResizeObserver() + +afterEach(() => { + cleanup() + clearAllPrompts() + _resetSessionOwnerHintsForTests() + $gateway.set(null) + vi.clearAllMocks() +}) + +// A master password typed into a background profile's unlock card must reach the +// backend that raised the prompt. The window's ambient `$gateway` may be another +// profile's socket entirely; sending the password there is a cross-backend leak. +it('routes the master password to the owning profile socket, never the ambient gateway', async () => { + $profiles.set([{ name: 'owner' }, { name: 'profile-b' }] as never) + setSessionOwnerHint('session-a', { connectionId: 'conn-1', profile: 'owner' }) + const ambient = vi.fn().mockResolvedValue({ status: 'ok' }) + $activeSessionId.set('session-b') + $gateway.set({ request: ambient } as never) + setVaultUnlockRequest({ backend: 'bitwarden', displayName: 'Bitwarden', requestId: 'req-a', sessionId: 'session-a' }) + + render() + const input = document.querySelector('input[type=password]')! + fireEvent.change(input, { target: { value: 'fixture-master' } }) + fireEvent.submit(input.closest('form')!) + + await waitFor(() => expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1)) + expect(gatewayMocks.requestGatewayForAgent.mock.calls[0].slice(0, 3)).toEqual(['conn-1', 'owner', 'vault.unlock.respond']) + expect(ambient).not.toHaveBeenCalled() + await waitFor(() => expect(sessionVaultUnlockRequest('session-a').get()).toBeNull()) +}) diff --git a/apps/desktop/src/lib/gateway-events.ts b/apps/desktop/src/lib/gateway-events.ts index a5905c5e6b..2ec1a64259 100644 --- a/apps/desktop/src/lib/gateway-events.ts +++ b/apps/desktop/src/lib/gateway-events.ts @@ -42,6 +42,7 @@ export const UNSCOPED_STREAM_EVENT_TYPES = new Set([ 'tool.generating', 'tool.progress', 'tool.start', + 'vault.unlock.expire', 'vault.unlock.request' ]) diff --git a/apps/desktop/src/store/prompts.ts b/apps/desktop/src/store/prompts.ts index 13eed8df39..0589245f7d 100644 --- a/apps/desktop/src/store/prompts.ts +++ b/apps/desktop/src/store/prompts.ts @@ -228,6 +228,7 @@ export const clearSecretRequest = secret.clear export const $vaultUnlockRequest = vaultUnlock.$active export const setVaultUnlockRequest = vaultUnlock.set export const clearVaultUnlockRequest = vaultUnlock.clear +export const $vaultUnlockRequests = vaultUnlock.$all export const sessionVaultUnlockRequest = (sessionId: string | null) => computed(vaultUnlock.$all, all => all[keyFor(sessionId)] ?? null) diff --git a/tests/agent/test_vault_backends.py b/tests/agent/test_vault_backends.py index 305615c2ba..c092127fa7 100644 --- a/tests/agent/test_vault_backends.py +++ b/tests/agent/test_vault_backends.py @@ -3,9 +3,10 @@ Two contracts that must never regress: 1. A locked manager never prompts where nobody can answer (cron/headless) and never leaks a value: browser_vault_list reports it under ``locked``, browser_vault_fill refuses. -2. The unlock path hands the master password to the manager CLI on stdin only (never argv), - keeps just the session token in memory, and a fill then routes by handle prefix through - the real subprocess path. Locking forgets the token. +2. The unlock path hands the master password to the manager CLI through its documented + non-interactive channel (bw: ``--passwordenv`` on the CHILD env only — never argv, never our + process env), keeps just the session token in memory scoped to the profile, and a fill then + routes by handle prefix through the real subprocess path. Locking forgets the token. """ from __future__ import annotations @@ -20,17 +21,22 @@ import pytest from agent.vault_backends import unlock as unlock_mod from agent.vault_backends.bitwarden import BitwardenLoginBackend -# A stand-in `bw` that mimics the three commands the backend uses. It records argv + stdin so the -# test can prove the master password travelled on stdin only, and a fake session key gates `list`. +# A stand-in `bw` that mimics the three commands the backend uses and the real CLI's password contract +# (bw 2026.x rejects a piped password: "Master password is required"; it reads --passwordenv ). +# It records argv + stdin + the named env var so the test can prove where the master password travelled. # (No env passthrough: the backend's allowlisted child env is part of what is under test.) _FAKE_BW = r'''#!/usr/bin/env python3 import json, os, sys log = open(os.path.join(os.path.dirname(os.path.abspath(__file__)), "bw.log"), "a") argv = sys.argv[1:] stdin = sys.stdin.read() if not sys.stdin.isatty() else "" -log.write(json.dumps({"argv": argv, "stdin": stdin, "BW_SESSION": os.environ.get("BW_SESSION")}) + "\n") +pw_env = argv[argv.index("--passwordenv") + 1] if "--passwordenv" in argv else None +log.write(json.dumps({"argv": argv, "stdin": stdin, "BW_SESSION": os.environ.get("BW_SESSION"), + "pw": os.environ.get(pw_env) if pw_env else None}) + "\n") if argv[:2] == ["unlock", "--raw"]: - if stdin.strip() != "correct horse": + if pw_env is None: + sys.stderr.write("Master password is required. Try again in interactive mode or provide a password file or environment variable.\n"); sys.exit(1) + if os.environ.get(pw_env) != "correct horse": sys.stderr.write("Invalid master password.\n"); sys.exit(1) print("SESSION-TOKEN-123"); sys.exit(0) if os.environ.get("BW_SESSION") != "SESSION-TOKEN-123": @@ -88,7 +94,7 @@ def test_locked_manager_is_reported_not_prompted_when_headless(fake_bw, monkeypa assert not unlock_mod.is_unlocked("bitwarden") -def test_unlock_feeds_stdin_only_then_fill_routes_by_prefix(fake_bw): +def test_unlock_uses_vendor_passwordenv_contract_then_fill_routes_by_prefix(fake_bw): exe, log = fake_bw from tools.browser_vault_tool import browser_vault_fill, browser_vault_list @@ -124,9 +130,17 @@ def test_unlock_feeds_stdin_only_then_fill_routes_by_prefix(fake_bw): calls = [json.loads(line) for line in log.read_text(encoding="utf-8").splitlines()] unlock_calls = [c for c in calls if c["argv"][:2] == ["unlock", "--raw"]] - assert len(unlock_calls) == 1 and unlock_calls[0]["stdin"].strip() == "correct horse" + assert len(unlock_calls) == 1 and unlock_calls[0]["pw"] == "correct horse" and unlock_calls[0]["stdin"] == "" assert all("correct horse" not in " ".join(c["argv"]) for c in calls), "master password must never be argv" assert all(c["BW_SESSION"] == "SESSION-TOKEN-123" for c in calls if c["argv"][0] != "unlock") + assert "HERMES_BW_MASTER" not in os.environ, "master password env var is child-only" + + # Tokens are profile-scoped: another HERMES_HOME sees the manager locked and cannot lock ours. + other = str(exe.parent / "other-profile") + with patch.dict(os.environ, {"HERMES_HOME": other}): + assert not backend.is_unlocked() + unlock_mod.lock("bitwarden") + assert backend.is_unlocked() unlock_mod.lock("bitwarden") assert not backend.is_unlocked() diff --git a/tests/test_browser_vault.py b/tests/test_browser_vault.py index d2ebdbb4d6..70fc3cb268 100644 --- a/tests/test_browser_vault.py +++ b/tests/test_browser_vault.py @@ -213,14 +213,20 @@ class TestClassifier: ) assert "InputEvent" in js and '"change"' in js and "filled" in js - def test_build_fill_js_has_no_dom_marker(self): - # P1-1: no deterministic selector for filled controls. + def test_build_fill_js_leaves_no_dom_marker_and_binds_target_to_inspection(self): + # P1-1: no persistent selector for filled controls. The fill targets the input by the + # slot stamp the inspection script wrote (a bare index is re-resolved by position and a + # DOM reflow between inspect and fill would redirect the password into another field); + # the stamps carry no secret and the fill script strips every one before returning. js = build_fill_js( [{"index": 0, "token": "current-password", "value": "x"}], expected_origin="https://example.com", ) assert "vaultSecret" not in js assert "data-vault-secret" not in js + assert "elements[f.index]" not in js + assert "input[data-hermes-vault-slot=" in js and 'el.type !== "password"' in js + assert js.index('removeAttribute("data-hermes-vault-slot")') > js.index("setter.set.call") def test_build_fill_js_asserts_origin_before_any_write(self): # P1-2: the origin assert must run inside the SAME script, before diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index 7dd5bdc8b9..982d1fdb32 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -70,7 +70,8 @@ def _redact_cdp_output(value: Any, *, always_paths: tuple = (), flagged_paths: t redacted: Dict[str, Any] = {} for key, item in value.items(): opaque = leaf(always_paths, key) or (leaf(flagged_paths, key) and base64_flagged) - redacted[key] = item if isinstance(item, str) and opaque else _redact_cdp_output( + out_key = redact_sensitive_text(key, force=True) if isinstance(key, str) else key # by-value objects can carry a secret as a KEY + redacted[out_key] = item if isinstance(item, str) and opaque else _redact_cdp_output( item, always_paths=descend(always_paths, key), flagged_paths=descend(flagged_paths, key)) return redacted diff --git a/tools/browser_tool_snapshot.py b/tools/browser_tool_snapshot.py index c35a512382..5a1329a7d3 100644 --- a/tools/browser_tool_snapshot.py +++ b/tools/browser_tool_snapshot.py @@ -121,5 +121,5 @@ def _redact_browser_output(value: Any) -> Any: if isinstance(value, tuple): return tuple(_redact_browser_output(item) for item in value) if isinstance(value, dict): - return {key: _redact_browser_output(item) for key, item in value.items()} + return {_redact_browser_output(key): _redact_browser_output(item) for key, item in value.items()} return value diff --git a/tui_gateway/methods_vault.py b/tui_gateway/methods_vault.py index 2cf96f5002..010a251f98 100644 --- a/tui_gateway/methods_vault.py +++ b/tui_gateway/methods_vault.py @@ -47,9 +47,6 @@ def _(rid, params: dict) -> dict: return _err(rid, 5095, str(e)) -_EXTERNAL_SOURCES = ("onepassword", "bitwarden") - - @method("vault.sources") def _(rid, params: dict) -> dict: """Status of every login source: {name, display_name, enabled, needs_unlock, unlocked, installed}.""" @@ -70,11 +67,12 @@ def _(rid, params: dict) -> dict: @method("vault.source.set") def _(rid, params: dict) -> dict: """Enable/disable an external manager: writes ``vault..enabled`` and locks it when disabling.""" + from agent.vault_backends.base import external_backend_classes from agent.vault_backends.unlock import lock from hermes_cli.config import load_config, save_config name = str(params.get("name") or "") - if name not in _EXTERNAL_SOURCES: + if name not in {cls.name for cls in external_backend_classes()}: return _err(rid, 5095, f"unknown vault source: {name}") enabled = bool(params.get("enabled")) cfg = load_config() diff --git a/tui_gateway/session_lifecycle.py b/tui_gateway/session_lifecycle.py index 251636b3c2..56136e5e50 100644 --- a/tui_gateway/session_lifecycle.py +++ b/tui_gateway/session_lifecycle.py @@ -5,6 +5,8 @@ globals at install time (method_ctx.bind_module), so they reference server.py gl from __future__ import annotations +import logging + import contextlib from .method_ctx import bind_module @@ -193,12 +195,30 @@ def _lifecycle_own_sid(session: dict, sid_hint: str = "") -> str: return own_sid +def _lock_vault_managers(session: dict) -> None: + """A per-session unlock ends with the session: forget the profile's manager tokens.""" + try: + from agent.vault_backends import unlock + from hermes_constants import reset_hermes_home_override, set_hermes_home_override + + home = session.get("profile_home") + token = set_hermes_home_override(home) if home else None + try: + unlock.lock() + finally: + if token is not None: + reset_hermes_home_override(token) + except Exception: + logging.getLogger(__name__).debug("vault manager lock on session end failed", exc_info=True) + + def _finalize_session(session: dict | None, end_reason: str = "tui_close") -> None: """Best-effort finalize hook + memory commit; mirrors the CLI exit path so a force-quit mid-turn (double Ctrl-C, terminal close, SIGHUP) loses nothing.""" if not session or session.get("_finalized"): return session["_finalized"] = True + _lock_vault_managers(session) if (history_ready := session.get("resume_history_ready")) is not None and not history_ready.is_set(): session["resume_history_error"] = "session resume cancelled" history_ready.set() diff --git a/website/docs/user-guide/features/credential-vault.md b/website/docs/user-guide/features/credential-vault.md index 0c63325e65..a89f626fe3 100644 --- a/website/docs/user-guide/features/credential-vault.md +++ b/website/docs/user-guide/features/credential-vault.md @@ -33,10 +33,10 @@ injected directly into the page. supervised browser session's direct CDP WebSocket, and returns only `{filled_fields, kind, origin, success}`. -The password never appears in tool results, logs, or the session database. -Its exact bytes are additionally registered with the browser-result -redaction boundary, so even a later `browser_cdp` read that manages to echo -the page's DOM cannot return them to the model. +The password does not appear in the fill's tool result, logs, or the +session database, and its exact bytes are registered with the browser-result +redaction boundary so a later `browser_*` read that echoes the page's DOM is +scrubbed. See *What this does and does not guarantee* below for the limits. ## CLI @@ -77,11 +77,13 @@ Unlock, Lock). A manager starts **locked**. The first time the agent needs one of its logins it asks you to unlock: a masked master-password prompt appears in the CLI, TUI, or Desktop chat (or you can unlock ahead of time from -Settings). Hermes hands the master password to `op signin` / `bw unlock` on -stdin, never as a command-line argument, and keeps only the resulting -session token in memory. The token expires after 30 minutes idle, when you -press **Lock**, or when the session ends. The agent never sees the master -password, the token, or any password. +Settings). Hermes hands the master password to the manager CLI through its +non-interactive channel (`op signin` reads stdin; `bw unlock --passwordenv` +reads a variable set only in the child process) — never as a command-line +argument, never in Hermes' own environment — and keeps only the resulting +session token in memory, scoped to the current profile. The token expires +after 30 minutes idle, when you press **Lock**, or when the session ends. +The agent never sees the master password, the token, or any password. `browser_vault_list` reports a locked manager under `locked`, and `browser_vault_unlock(backend)` triggers the prompt explicitly. @@ -168,9 +170,24 @@ Agent: browser_click() - **Encrypted at rest:** vault file and key are created `0600` in your Hermes home; nothing is sent to any server. - **Master password never stored:** for 1Password/Bitwarden the master - password goes to the manager CLI on stdin and is dropped; only the - session token is held, in memory, with an idle timeout. Headless sessions - can't prompt and see the manager as locked. + password is consumed by the manager CLI and dropped; only the session + token is held, in memory, per profile, with an idle timeout. Headless + sessions can't prompt and see the manager as locked. + +### What this does and does not guarantee + +The vault keeps passwords out of the model's *normal* path: list/fill +results carry metadata only, the fill runs over the supervised CDP socket, +and every browser tool result is scrubbed for the exact filled bytes +(including the CR/LF-normalized form a text input stores, and JSON object +keys). This is accidental-disclosure protection, not an execution sandbox: +a session that also has arbitrary page JavaScript (`browser_cdp +Runtime.evaluate`) or host code execution could in principle transform a +filled value (for example base64-encode it) into a string the redactor does +not recognize. If that matters for a credential, restrict the session's +toolset (drop `browser_cdp`/`terminal`/`execute_code`) or use a dedicated +low-privilege account for agent logins. Treat the filled credential as +exposed to the same trust boundary as the browser session itself. ## Notes