From dedc99ec6cb259acec614d2dea1ae0ee09ab291e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 9 Sep 2026 05:22:16 -0700 Subject: [PATCH] fix(vault): ownership and race findings from the second independent review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manager tokens: lock generation fence (a Lock acknowledged while `bw unlock` / `op signin` is still running discards the late token); tokens record the unlocking gateway session and are released when THAT session ends, not when any sibling session in the profile is torn down. 1Password: OP_CONNECT_HOST/TOKEN come from the profile's scoped secret store like the service token (Connect outranks a service token inside op), never from the launch environment. Vault RPCs bind params.profile (home + secret scope) so a shared remote backend serving several profiles locks/lists/unlocks the requested one; unknown profile → RPC error, not a crash. Fill target: inspection stamps are `:`; a fill resolves only its own inspection's stamps, so an interleaved second inspection can no longer redirect A's password into a newly mounted field (real Chrome: 0 filled, both fields empty). Desktop Settings: every RPC goes through the owner profile's socket (requestGatewayForProfile), query keys carry (connection, profile), an owner change closes dialogs and wipes drafts (a master password typed for A is never submitted to B; a late list from A never paints under B), and vault.add secrets travel in a ref consumed by the mutationFn instead of mutation variables. Three owner-routing invariant tests on the real component. Docs/PR body: session-scoped release, lock-race semantics, bw --passwordenv. --- agent/vault_backends/bitwarden.py | 4 +- agent/vault_backends/onepassword.py | 12 +- agent/vault_backends/unlock.py | 56 ++++++++- agent/vault_login_classifier.py | 22 ++-- .../settings/vault-settings.owner.test.tsx | 106 ++++++++++++++++++ .../src/app/settings/vault-settings.test.tsx | 8 +- .../src/app/settings/vault-settings.tsx | 57 ++++++++-- tests/agent/test_vault_backends.py | 22 ++++ tests/test_browser_vault.py | 8 +- tools/browser_vault_tool.py | 8 +- tui_gateway/agent_callbacks.py | 3 +- tui_gateway/methods_vault.py | 24 +++- tui_gateway/session_lifecycle.py | 12 +- .../user-guide/features/credential-vault.md | 6 +- 14 files changed, 297 insertions(+), 51 deletions(-) create mode 100644 apps/desktop/src/app/settings/vault-settings.owner.test.tsx diff --git a/agent/vault_backends/bitwarden.py b/agent/vault_backends/bitwarden.py index 697c490a7d..9857563282 100644 --- a/agent/vault_backends/bitwarden.py +++ b/agent/vault_backends/bitwarden.py @@ -58,6 +58,7 @@ class BitwardenLoginBackend(LoginBackend): def unlock(self, master_password: str) -> None: # 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. + generation = _unlock.begin_unlock(self.name) 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") @@ -67,7 +68,8 @@ class BitwardenLoginBackend(LoginBackend): if "not logged in" in err.lower(): err = "not logged in — run `bw login` once in a terminal first" raise RuntimeError(f"Bitwarden unlock failed: {err or 'no session key'}") - _unlock.store_session_token(self.name, token) + if not _unlock.store_session_token(self.name, token, generation): + raise RuntimeError("Bitwarden was locked while unlocking; try again") def _run(self, *args: str) -> str: token = _unlock.get_session_token(self.name) diff --git a/agent/vault_backends/onepassword.py b/agent/vault_backends/onepassword.py index bb1799c4c8..75e64ec14e 100644 --- a/agent/vault_backends/onepassword.py +++ b/agent/vault_backends/onepassword.py @@ -48,7 +48,13 @@ class OnePasswordLoginBackend(LoginBackend): return op def _env(self, session_token: Optional[str]) -> Dict[str, str]: - env = {k: os.environ[k] for k in _OP_ENV_ALLOWLIST if k in os.environ} + from agent.secret_scope import get_secret + env = {k: os.environ[k] for k in _OP_ENV_ALLOWLIST if k in os.environ and not k.startswith("OP_CONNECT_")} + # Connect credentials outrank OP_SERVICE_ACCOUNT_TOKEN inside op, so they must come from the + # profile's own secret scope like the service token does — never from the launch environment. + for k in ("OP_CONNECT_HOST", "OP_CONNECT_TOKEN"): + if v := get_secret(k, ""): + env[k] = v env["NO_COLOR"] = "1" account = str(self.cfg.get("account") or "") if account: @@ -66,6 +72,7 @@ class OnePasswordLoginBackend(LoginBackend): def unlock(self, master_password: str) -> None: """Mint a session token from the master password (consumed on stdin, never argv).""" + generation = _unlock.begin_unlock(self.name) cmd = [str(self._op()), "signin", "--raw"] if account := str(self.cfg.get("account") or ""): cmd += ["--account", account] @@ -73,7 +80,8 @@ class OnePasswordLoginBackend(LoginBackend): token = (proc.stdout or "").strip() if proc.returncode != 0 or not token: raise RuntimeError(f"1Password unlock failed: {_scrub(proc.stderr or '')[:200] or 'no session token'}") - _unlock.store_session_token(self.name, token) + if not _unlock.store_session_token(self.name, token, generation): + raise RuntimeError("1Password was locked while unlocking; try again") def _run(self, *args: str) -> str: token = None if self._service_token else _unlock.get_session_token(self.name) diff --git a/agent/vault_backends/unlock.py b/agent/vault_backends/unlock.py index 49048c3505..52009ceed8 100644 --- a/agent/vault_backends/unlock.py +++ b/agent/vault_backends/unlock.py @@ -44,6 +44,21 @@ def _key(backend: str) -> tuple[str, str]: return (str(get_hermes_home()), backend) +# Lock generation per key: ``lock()`` bumps it, and an unlock that started before the bump must +# not commit its token afterwards (a slow `bw unlock` child would otherwise silently undo an +# acknowledged Lock). +_generation: Dict[tuple[str, str], int] = {} +# Which gateway session performed the unlock; the token is released when THAT session ends, +# not when any sibling session in the profile is torn down. +_owner_session: Dict[tuple[str, str], Optional[str]] = {} +_current_session_tls = threading.local() + + +def set_current_session_id(session_id: Optional[str]) -> None: + """Gateway surfaces bind the session running on this thread so an unlock records its owner.""" + _current_session_tls.sid = session_id + + def _live(backend: str, *, touch: bool) -> Optional[str]: key = _key(backend) with _lock: @@ -64,23 +79,54 @@ def get_session_token(backend: str) -> Optional[str]: return _live(backend, touch=True) -def store_session_token(backend: str, token: str) -> None: +def begin_unlock(backend: str) -> int: + """Snapshot the lock generation before spawning the manager CLI; pass it to ``store_session_token``.""" with _lock: - _sessions[_key(backend)] = (token, time.monotonic()) + return _generation.get(_key(backend), 0) + + +def store_session_token(backend: str, token: str, generation: Optional[int] = None) -> bool: + """Commit an unlock. Returns False (and drops the token) when a Lock happened since ``begin_unlock``.""" + key = _key(backend) + with _lock: + if generation is not None and generation != _generation.get(key, 0): + return False + _sessions[key] = (token, time.monotonic()) + _owner_session[key] = getattr(_current_session_tls, "sid", None) + return True def lock(backend: Optional[str] = None) -> None: """Forget the current profile's session for one backend (or all of them when None).""" home = _key("")[0] with _lock: - for key in [k for k in _sessions if k[0] == home and (backend is None or k[1] == backend)]: - del _sessions[key] + # Bump the generation for every key the lock names (not only the ones holding a token): + # an unlock that is still running for this backend must see the lock when it returns. + keys = {k for k in list(_sessions) + list(_generation) if k[0] == home and (backend is None or k[1] == backend)} + if backend is not None: + keys.add((home, backend)) + for key in keys: + _forget(key) + + +def release_session(session_id: str) -> None: + """A gateway session ended: drop only the tokens that session unlocked.""" + with _lock: + for key in [k for k, sid in _owner_session.items() if sid == session_id]: + _forget(key) + + +def _forget(key: tuple[str, str]) -> None: + _sessions.pop(key, None) + _owner_session.pop(key, None) + _generation[key] = _generation.get(key, 0) + 1 def lock_all_profiles() -> None: """Process shutdown: drop every token.""" with _lock: - _sessions.clear() + for key in list(_sessions): + _forget(key) def is_unlocked(backend: str) -> bool: diff --git a/agent/vault_login_classifier.py b/agent/vault_login_classifier.py index 21438e7d87..3468d2c481 100644 --- a/agent/vault_login_classifier.py +++ b/agent/vault_login_classifier.py @@ -133,16 +133,21 @@ 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 stamps every input with ``:`` under a per-inspection attribute; the fill +# script resolves targets by the stamp of ITS OWN inspection instead of re-querying by position, so +# neither a DOM reflow nor a second inspection in between can redirect the password into another field. INSPECTION_STAMP_ATTR = "data-hermes-vault-slot" -LOGIN_CONTROL_INSPECTION_JS = """(() => { + +def build_inspection_js(nonce: str) -> str: + return _LOGIN_CONTROL_INSPECTION_JS_TEMPLATE.replace("__NONCE__", json.dumps(nonce)) + + +_LOGIN_CONTROL_INSPECTION_JS_TEMPLATE = """(() => { + const nonce = __NONCE__; 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))); + elements.forEach((element, index) => element.setAttribute("data-hermes-vault-slot", nonce + ":" + 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 []; @@ -173,7 +178,7 @@ LOGIN_CONTROL_INSPECTION_JS = """(() => { })()""" -def build_fill_js(fills: List[Dict[str, Any]], expected_origin: str) -> str: +def build_fill_js(fills: List[Dict[str, Any]], expected_origin: str, nonce: str = "") -> str: """Build a JS expression that fills the selected inputs and reports only a count. The returned expression never echoes the values back. @@ -197,9 +202,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" + f" const nonce = {json.dumps(nonce)};\n" " let filled = 0;\n" " for (const f of fills) {\n" - " const el = document.querySelector('input[data-hermes-vault-slot=\"' + f.index + '\"]');\n" + " const el = document.querySelector('input[data-hermes-vault-slot=\"' + nonce + ':' + f.index + '\"]');\n" " if (!el || el.type !== \"password\") continue;\n" " try {\n" " el.focus();\n" diff --git a/apps/desktop/src/app/settings/vault-settings.owner.test.tsx b/apps/desktop/src/app/settings/vault-settings.owner.test.tsx new file mode 100644 index 0000000000..38418572d1 --- /dev/null +++ b/apps/desktop/src/app/settings/vault-settings.owner.test.tsx @@ -0,0 +1,106 @@ +import { QueryClientProvider } from '@tanstack/react-query' +import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { MemoryRouter } from 'react-router' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +import { stubResizeObserver } from '@/test/jsdom' + +// Every vault RPC is routed to the OWNER profile's socket; the mock records which profile each +// call targeted so the tests can prove a draft never crosses owners. +const { calls } = vi.hoisted(() => ({ calls: [] as { method: string; params: Record; profile: string }[] })) +let respond: (profile: string, method: string) => Promise = async () => ({}) + +vi.mock('@/store/gateway', async importActual => ({ + ...(await importActual>()), + requestGatewayForProfile: (profile: string, method: string, params?: Record) => { + calls.push({ method, params: params ?? {}, profile }) + + return respond(profile, method) + } +})) +vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) +vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) + +import { queryClient } from '@/lib/query-client' +import { $activeGatewayProfile } from '@/store/profile' +import { $gatewayState } from '@/store/session' + +import { VaultSettings } from './vault-settings' + +stubResizeObserver() + +const sources = [ + { name: 'bitwarden', display_name: 'Bitwarden', enabled: true, needs_unlock: true, unlocked: false, installed: true } +] + +function mount() { + return render( + + + + + + ) +} + +beforeEach(() => { + calls.length = 0 + queryClient.clear() + $activeGatewayProfile.set('default') + $gatewayState.set('open') + respond = async (_profile, method) => (method === 'vault.sources' ? { sources } : method === 'vault.list' ? { items: [] } : { ok: true }) +}) + +afterEach(() => { + cleanup() + queryClient.clear() +}) + +it('a master-password draft is wiped on a profile switch and never submitted to the new owner', async () => { + mount() + fireEvent.click(await screen.findByRole('button', { name: 'Unlock' })) + fireEvent.change(screen.getByPlaceholderText('Master password'), { target: { value: 'password-for-A' } }) + + act(() => $activeGatewayProfile.set('other-profile')) + + await waitFor(() => expect(screen.queryByPlaceholderText('Master password')).toBeNull()) + expect(calls.filter(c => c.method === 'vault.unlock')).toHaveLength(0) + // Reads for the new owner target the new profile, not the old one. + await waitFor(() => expect(calls.some(c => c.profile === 'other-profile' && c.method === 'vault.list')).toBe(true)) +}) + +it("a late list response from profile A never paints under profile B", async () => { + let resolveA!: (value: unknown) => void + const held = new Promise(r => (resolveA = r)) + respond = async (profile, method) => { + if (method === 'vault.sources') return { sources } + if (profile === 'default' && method === 'vault.list') return held + return { items: [] } + } + mount() + await waitFor(() => expect(calls.some(c => c.profile === 'default' && c.method === 'vault.list')).toBe(true)) + + act(() => $activeGatewayProfile.set('other-profile')) + await waitFor(() => expect(calls.some(c => c.profile === 'other-profile' && c.method === 'vault.list')).toBe(true)) + + await act(async () => { + resolveA({ items: [{ id: 'a', kind: 'login', label: 'A-only private account', origin: 'https://a.example', identifier: 'a@example.com', created_at: '' }] }) + await held + }) + expect(screen.queryByText('A-only private account')).toBeNull() +}) + +it('vault.add secrets never enter the mutation cache', async () => { + respond = async (_profile, method) => (method === 'vault.sources' ? { sources } : method === 'vault.list' ? { items: [] } : { id: 'created' }) + const view = mount() + fireEvent.click(await screen.findByRole('button', { name: 'Add credential' })) + for (const [label, value] of [['Label', 'fixture'], ['Site origin', 'https://example.com'], ['Identifier', 'fixture@example.com'], ['Password', 'fixture-retained-password']] as const) { + fireEvent.change(screen.getByLabelText(label), { target: { value } }) + } + fireEvent.click(screen.getByRole('button', { name: 'Save to vault' })) + await waitFor(() => expect(calls.some(c => c.method === 'vault.add')).toBe(true)) + expect((calls.find(c => c.method === 'vault.add')!.params.secret as Record).password).toBe('fixture-retained-password') + await waitFor(() => expect(screen.queryByLabelText('Password')).toBeNull()) + view.unmount() + expect(JSON.stringify(queryClient.getMutationCache().getAll().map(m => m.state.variables))).not.toContain('fixture-retained-password') +}) diff --git a/apps/desktop/src/app/settings/vault-settings.test.tsx b/apps/desktop/src/app/settings/vault-settings.test.tsx index e6586fc5a2..9b29c58f11 100644 --- a/apps/desktop/src/app/settings/vault-settings.test.tsx +++ b/apps/desktop/src/app/settings/vault-settings.test.tsx @@ -9,8 +9,12 @@ const { requestGateway } = vi.hoisted(() => ({ requestGateway: vi.fn() })) -vi.mock('@/app/gateway/hooks/use-gateway-request', () => ({ - useGatewayRequest: () => ({ requestGateway }) +// The panel routes every RPC through the owner profile's socket (never the ambient gateway); +// the mock receives (method, params) after the profile argument. +vi.mock('@/store/gateway', async importActual => ({ + ...(await importActual>()), + requestGatewayForProfile: (_profile: string, method: string, params?: Record) => + requestGateway(method, params ?? {}) })) import { queryClient } from '@/lib/query-client' diff --git a/apps/desktop/src/app/settings/vault-settings.tsx b/apps/desktop/src/app/settings/vault-settings.tsx index a9d5e1c639..d2571dc00d 100644 --- a/apps/desktop/src/app/settings/vault-settings.tsx +++ b/apps/desktop/src/app/settings/vault-settings.tsx @@ -3,7 +3,6 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query' import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { useSearchParams } from 'react-router' -import { useGatewayRequest } from '@/app/gateway/hooks/use-gateway-request' import { Button } from '@/components/ui/button' import { ConfirmDialog } from '@/components/ui/confirm-dialog' import { @@ -22,14 +21,19 @@ import { Switch } from '@/components/ui/switch' import { useI18n } from '@/i18n' import { triggerHaptic } from '@/lib/haptics' import { KeyRound, Lock, Plus, ShieldLock, Trash2 } from '@/lib/icons' +import { $activeConnectionId } from '@/store/connections' +import { requestGatewayForProfile } from '@/store/gateway' import { notify, notifyError } from '@/store/notifications' import { $gatewayState } from '@/store/session' +import { $settingsScopeProfile } from '@/store/settings-scope' import { CONTROL_TEXT } from './constants' import { ListRow, Pill, SectionHeading, SettingsContent } from './primitives' -const VAULT_QUERY_KEY = ['vault-items'] as const -const VAULT_SOURCES_QUERY_KEY = ['vault-sources'] as const +// Vault data is private to one (connection, profile); the cache key carries that owner so a +// late response from profile A can never paint under profile B. +const vaultQueryKey = (owner: string) => ['vault-items', owner] as const +const vaultSourcesQueryKey = (owner: string) => ['vault-sources', owner] as const export type VaultSourceName = 'bitwarden' | 'local' | 'onepassword' @@ -145,9 +149,21 @@ function buildSecret(form: VaultForm): Record { export function VaultSettings() { const { t } = useI18n() const v = t.settings.vault - const { requestGateway } = useGatewayRequest() const gatewayState = useStore($gatewayState) const queryClient = useQueryClient() + // The owner this panel edits: pinned per render, and every RPC below goes through the owner's + // socket with an explicit profile — never the ambient foreground gateway. Changing owner + // (profile switch, connection swap) closes every dialog and wipes drafts (see the effect below). + const scopeProfile = useStore($settingsScopeProfile) + const connectionId = useStore($activeConnectionId) + const owner = `${connectionId ?? ''}::${scopeProfile}` + const requestGateway = useCallback( + (method: string, params: Record = {}) => + requestGatewayForProfile(scopeProfile, method, params), + [scopeProfile] + ) + const VAULT_QUERY_KEY = useMemo(() => vaultQueryKey(owner), [owner]) + const VAULT_SOURCES_QUERY_KEY = useMemo(() => vaultSourcesQueryKey(owner), [owner]) const [searchParams, setSearchParams] = useSearchParams() const [addOpen, setAddOpen] = useState(false) @@ -157,6 +173,23 @@ export function VaultSettings() { const [unlockTarget, setUnlockTarget] = useState(null) const [masterPassword, setMasterPassword] = useState('') const [unlockError, setUnlockError] = useState(null) + // Secrets never become mutation variables (react-query retains those after settle); they live + // in refs the mutationFn consumes and wipes. + const pendingMasterPassword = useRef('') + const pendingSecret = useRef>(null) + + useEffect(() => { + // A draft typed for one owner must not be submitted to another. + setAddOpen(false) + setForm(EMPTY_FORM) + setFormError(null) + setPendingDelete(null) + setUnlockTarget(null) + setMasterPassword('') + setUnlockError(null) + pendingMasterPassword.current = '' + pendingSecret.current = null + }, [owner]) const { data: sourcesData } = useQuery({ enabled: gatewayState === 'open', @@ -195,10 +228,6 @@ 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 }: { name: VaultSourceName }) => { const password = pendingMasterPassword.current @@ -283,8 +312,12 @@ export function VaultSettings() { ) const addMutation = useMutation({ - mutationFn: async (payload: { kind: VaultKind; label: string; origin?: string; secret: Record }) => - requestGateway<{ id: string }>('vault.add', payload), + mutationFn: async (payload: { kind: VaultKind; label: string; origin?: string }) => { + const secret = pendingSecret.current + pendingSecret.current = null + + return requestGateway<{ id: string }>('vault.add', { ...payload, secret: secret ?? {} }) + }, onSuccess: () => { triggerHaptic('success') notify({ kind: 'info', message: v.added }) @@ -326,11 +359,11 @@ export function VaultSettings() { return } + pendingSecret.current = buildSecret(form) addMutation.mutate({ kind: form.kind, label: form.label.trim(), - ...(origin ? { origin } : {}), - secret: buildSecret(form) + ...(origin ? { origin } : {}) }) }, [addMutation, form, v.labelRequired, v.loginFieldsRequired, v.originInvalid]) diff --git a/tests/agent/test_vault_backends.py b/tests/agent/test_vault_backends.py index c092127fa7..f76b98a559 100644 --- a/tests/agent/test_vault_backends.py +++ b/tests/agent/test_vault_backends.py @@ -145,3 +145,25 @@ def test_unlock_uses_vendor_passwordenv_contract_then_fill_routes_by_prefix(fake unlock_mod.lock("bitwarden") assert not backend.is_unlocked() assert os.environ.get("BW_SESSION") is None, "session token must never touch the process env" + + +def test_lock_during_unlock_wins_and_only_the_owning_session_release_drops_a_token(fake_bw, monkeypatch): + """A Lock acknowledged while `bw unlock` is still running must not be undone when the child returns; + a session teardown releases only the tokens that session unlocked.""" + exe, _log = fake_bw + patcher, backend = _enabled(exe) + with patcher: + # Lock races the in-flight unlock: the generation moved, so the late token is discarded. + gen = unlock_mod.begin_unlock("bitwarden") + unlock_mod.lock("bitwarden") + assert unlock_mod.store_session_token("bitwarden", "LATE-TOKEN", gen) is False + assert not backend.is_unlocked() + + unlock_mod.set_current_session_id("sess-A") + backend.unlock("correct horse") + assert backend.is_unlocked() + unlock_mod.release_session("sess-B") # an unrelated sibling session ends + assert backend.is_unlocked() + unlock_mod.release_session("sess-A") + assert not backend.is_unlocked() + unlock_mod.set_current_session_id(None) diff --git a/tests/test_browser_vault.py b/tests/test_browser_vault.py index 70fc3cb268..a654eddac4 100644 --- a/tests/test_browser_vault.py +++ b/tests/test_browser_vault.py @@ -215,9 +215,9 @@ class TestClassifier: 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. + # : stamp ITS OWN inspection wrote (a bare index is re-resolved by position, + # and a second inspection in between would re-stamp — either way the password could land in + # another field); 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", @@ -225,7 +225,7 @@ class TestClassifier: 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 "input[data-hermes-vault-slot=" in js and "nonce + ':' + f.index" 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): diff --git a/tools/browser_vault_tool.py b/tools/browser_vault_tool.py index b0966db07a..37d075752a 100644 --- a/tools/browser_vault_tool.py +++ b/tools/browser_vault_tool.py @@ -26,6 +26,7 @@ autofill (kernel-login-autofill.ts / fill_from_vault.ts). from __future__ import annotations import json +import secrets import logging from typing import Any, Dict, Optional @@ -230,10 +231,10 @@ def browser_vault_fill(handle: str, task_id: Optional[str] = None) -> str: """ from agent.redact import register_vault_redaction_value from agent.vault_login_classifier import ( - LOGIN_CONTROL_INSPECTION_JS, ClassifiedLoginControl, LoginControl, build_fill_js, + build_inspection_js, classify_login_control, select_password_fill, ) @@ -289,7 +290,8 @@ def browser_vault_fill(handle: str, task_id: Optional[str] = None) -> str: ) # ── Inspect + classify page controls ──────────────────────────────────── - inspect = _eval_js(effective_task_id, LOGIN_CONTROL_INSPECTION_JS) + nonce = secrets.token_hex(8) # binds this fill to THIS inspection's stamps + inspect = _eval_js(effective_task_id, build_inspection_js(nonce)) if not inspect.get("success"): return json.dumps( {"success": False, "error": f"Could not inspect page inputs: {inspect.get('error', 'eval failed')}"} @@ -330,7 +332,7 @@ def browser_vault_fill(handle: str, task_id: Optional[str] = None) -> str: try: fill_result = _eval_js_secret( - effective_task_id, build_fill_js(fills, expected_origin=str(meta.origin)) + effective_task_id, build_fill_js(fills, expected_origin=str(meta.origin), nonce=nonce) ) except Exception as exc: # Strip any secret material from exception text before surfacing. diff --git a/tui_gateway/agent_callbacks.py b/tui_gateway/agent_callbacks.py index db15890d16..66e8b50753 100644 --- a/tui_gateway/agent_callbacks.py +++ b/tui_gateway/agent_callbacks.py @@ -171,7 +171,8 @@ def _wire_callbacks(sid: str): set_secret_capture_callback(secret_cb) # External password-manager unlock: the renderer shows a masked master-password card; the # answer is consumed by the manager CLI on stdin and only a session token stays in memory. - from agent.vault_backends.unlock import set_unlock_prompt_callback + from agent.vault_backends.unlock import set_current_session_id, set_unlock_prompt_callback + set_current_session_id(sid) # an unlock made on this turn belongs to this session (released with it) set_unlock_prompt_callback(lambda backend, display_name: _block( "vault.unlock.request", sid, {"backend": backend, "display_name": display_name}, timeout=120)) diff --git a/tui_gateway/methods_vault.py b/tui_gateway/methods_vault.py index 010a251f98..67918b77bb 100644 --- a/tui_gateway/methods_vault.py +++ b/tui_gateway/methods_vault.py @@ -14,7 +14,12 @@ JSON-RPC channel every other Settings surface uses. Contracts: - ``vault.remove`` → {removed: bool}. - ``vault.sources`` / ``vault.source.set`` → external password-manager status and enable toggle. - ``vault.unlock`` / ``vault.lock`` → per-session unlock of a manager from Settings; the master - password is consumed by the manager CLI on stdin and never stored or logged. + password is consumed by the manager CLI through its non-interactive channel and never stored + or logged. + +Every handler honours ``params.profile`` (app-global remote mode serves several profiles from one +backend): the requested profile's HERMES_HOME and secret scope are bound around the body, so the +vault file, manager config and manager tokens all resolve to that profile. Handlers are rebound onto server.py's globals at install time (see method_ctx.py) and may reference server module globals (``_ok``, ``_err``). @@ -23,7 +28,22 @@ method_ctx.py) and may reference server module globals (``_ok``, ``_err``). from .method_ctx import HandlerRegistry _registry = HandlerRegistry() -method = _registry.method + + +def method(name: str): + """``@method(name)`` with ``params.profile`` bound (home + secret scope) around the handler.""" + def deco(fn): + def scoped(rid, params: dict) -> dict: + try: + home = _profile_home(params.get("profile") if isinstance(params, dict) else None) + except FileNotFoundError as e: + return _err(rid, 5095, str(e)) + if home is None: + return fn(rid, params) + with _session_profile_runtime_scope({"profile_home": str(home)}): + return fn(rid, params) + return _registry.method(name)(scoped) + return deco # JSON-RPC error code 5095 = vault failure (validation + store errors). # Kept as a literal inside handler bodies: handlers are rebound onto diff --git a/tui_gateway/session_lifecycle.py b/tui_gateway/session_lifecycle.py index 56136e5e50..0b763aa9ac 100644 --- a/tui_gateway/session_lifecycle.py +++ b/tui_gateway/session_lifecycle.py @@ -196,18 +196,12 @@ def _lifecycle_own_sid(session: dict, sid_hint: str = "") -> str: def _lock_vault_managers(session: dict) -> None: - """A per-session unlock ends with the session: forget the profile's manager tokens.""" + """A per-session unlock ends with the session that made it; siblings in the same profile keep theirs.""" 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) + if sid := session.get("_sid"): + unlock.release_session(sid) except Exception: logging.getLogger(__name__).debug("vault manager lock on session end failed", exc_info=True) diff --git a/website/docs/user-guide/features/credential-vault.md b/website/docs/user-guide/features/credential-vault.md index a89f626fe3..d2360934fa 100644 --- a/website/docs/user-guide/features/credential-vault.md +++ b/website/docs/user-guide/features/credential-vault.md @@ -82,8 +82,10 @@ 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. +after 30 minutes idle, when you press **Lock**, or when the chat session that +unlocked it ends (other sessions in the same profile keep their own unlocks). +A **Lock** pressed while an unlock is still in flight wins. 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.