fix(vault): independent-review findings — vendor contracts, profile scope, transport, target binding
Bitwarden unlock now uses the CLI's documented non-interactive channel: `bw unlock --raw --nointeraction --passwordenv VAR`, VAR set on the child environment only (bw 2026.x rejects a piped password with "Master password is required"). Verified against the real published binary. Manager session tokens are keyed by (profile home, backend): a Desktop gateway hosting several profiles can no longer reuse or lock another profile's session. Status probes (`vault.sources`, is_unlocked) no longer refresh the idle TTL; only real manager calls do. Gateway session teardown locks the profile's managers (a per-session unlock ends with the session). 1Password service-account token comes from the profile-scoped secret store (get_secret), not ambient os.environ. `vault.source.set` no longer references a module constant (bind_module rebinding dropped it → NameError on every Settings toggle). Fill target binding: inspection stamps each input with a per-inspection slot attribute; the fill resolves by stamp and requires type=password, then strips every stamp. A DOM reflow between inspect and fill can no longer redirect the password into a text field (reproduced in real Chrome before, 0 filled after). Redaction boundary: no 4-char floor, CR/LF-normalized form registered (what a text input actually stores), JSON object KEYS scrubbed in both browser redactors; longest value first. Docs now state the real trust model: accidental-disclosure protection, not an execution sandbox. Desktop: the mid-turn card sends the master password through the owning session's socket (requestForOwnedSession), never the ambient foreground gateway; `vault.unlock.expire` clears a stale card; Settings keeps the master password out of react-query mutation variables (ref consumed by the mutationFn). One renderer invariant test for the routing.
This commit is contained in:
+7
-6
@@ -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»")
|
||||
|
||||
@@ -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 {}
|
||||
|
||||
@@ -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 <id>``.
|
||||
"""
|
||||
@@ -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]
|
||||
|
||||
@@ -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 ────────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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"
|
||||
"})()"
|
||||
)
|
||||
|
||||
+19
-1
@@ -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
|
||||
|
||||
@@ -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 })
|
||||
}
|
||||
}}
|
||||
>
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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<Record<string, unknown>>()),
|
||||
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(<PromptOverlays sessionId="session-a" />)
|
||||
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())
|
||||
})
|
||||
@@ -42,6 +42,7 @@ export const UNSCOPED_STREAM_EVENT_TYPES = new Set([
|
||||
'tool.generating',
|
||||
'tool.progress',
|
||||
'tool.start',
|
||||
'vault.unlock.expire',
|
||||
'vault.unlock.request'
|
||||
])
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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 <VAR>).
|
||||
# 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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.<name>.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()
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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(<submit>)
|
||||
- **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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user