fix(vault): ownership and race findings from the second independent review
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 `<nonce>:<index>`; 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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user