fix(credentials): unified provider key delete/update across .env, auth.json, config.yaml (#67213)
* fix(env): recognize export-prefixed .env lines in save/remove (#40041) load_env() parses bash-compatible 'export KEY=value' lines (#6659), so a hand-added 'export GITHUB_TOKEN=ghp_...' shows as set (green light) in the desktop Tools & Keys page. But save_env_value/remove_env_value only matched plain 'KEY=' lines: - DELETE /api/env 404'd ('not found in .env') — the token could not be removed through the UI - PUT /api/env appended a SECOND line; a later delete removed the new line while the export line silently resurrected the old value Both writers now match assignments through a shared _env_line_defines_key() helper that understands the export prefix. Commented-out lines are still ignored. Regression tests drive the real dashboard endpoint handlers against a temp HERMES_HOME with runtime-constructed classic-PAT-shaped fixtures, covering save-does-not-500, export-line remove, export-line replace-without-duplicate, and the plain-line path staying intact. Fixes #40041 * fix(credentials): unify provider key delete/update across .env, auth.json, config.yaml (#51071 #59761 #62269) A provider API key can live in three stores at once: ~/.hermes/.env, auth.json credential_pool (env-seeded 'env:<VAR>' entries persisted by the pool loader), and config.yaml mirrors (model.api_key, auxiliary.*.api_key, custom_providers[*].api_key). The desktop/dashboard endpoints and the TUI gateway RPCs only ever mutated .env, so the stores diverged: - #51071/#59761: DELETE /api/env removed the key from .env but left the credential_pool entry (the loader is additive-only and never prunes), so the provider kept appearing in the model picker — surviving restart via the stale pool entry + provider_models_cache.json row. - #62269: PUT /api/env rewrote .env but left the OLD key in config.yaml (model.api_key wins over env at client construction), producing 401s with a key the UI no longer showed. New hermes_cli/credential_lifecycle.py is the single choke point: - remove_provider_env_credential(): clears the .env entry, prunes env:<VAR> pool entries across ALL providers (a shared var like GITHUB_TOKEN can seed several), suppresses the env source so a lingering shell export can't re-seed it (matching 'hermes auth remove' semantics), drops the affected providers' model-cache rows, and scrubs value-matched config.yaml api_key mirrors. Returns 'found' spanning every store so a stale pool-only entry is cleanable through the same delete button. - save_provider_env_credential(): writes .env, rotates any config.yaml mirror that held the PREVIOUS value (value-matched — an unrelated inline key is untouched), and lifts a prior env-source suppression so re-adding behaves like 'hermes auth add'. OAuth preservation: only entries with source == 'env:<VAR>' are pruned. OAuth/device-code/manual/borrowed pool entries and providers.<id> OAuth token blocks are never touched by a key-only delete. (model.disconnect in the TUI gateway still clears OAuth via clear_provider_auth — that surface is a full provider disconnect, which is the documented intent there.) Rerouted call sites: PUT/DELETE /api/env (dashboard + desktop), tui_gateway model.save_key / model.disconnect, save_env_value_secure (TUI/gateway secret capture), and hermes config set/unset for env-shaped keys. E2E tests drive the real endpoint handlers against temp-HERMES_HOME fixtures (.env + auth.json + config.yaml with runtime-constructed fake keys) and assert cross-store consistency after delete/update, pool-reload survival ('restart'), OAuth preservation, models-cache invalidation, and the suppress/unsuppress round-trip. Fixes #51071 Fixes #59761 Fixes #62269
This commit is contained in:
Teknium
+39
-7
@@ -7884,6 +7884,20 @@ def _quote_env_value(value: str) -> str:
|
||||
return f'"{escaped}"'
|
||||
|
||||
|
||||
def _env_line_defines_key(line: str, key: str) -> bool:
|
||||
"""True when a .env line assigns ``key`` — plain or ``export``-prefixed.
|
||||
|
||||
``load_env()`` accepts the bash-compatible ``export KEY=value`` form
|
||||
(#6659), so the writers must recognise the same shape. Otherwise a
|
||||
hand-added ``export`` line is invisible to save (duplicate appended) and
|
||||
remove (line survives → the value resurrects on the next load, #40041).
|
||||
"""
|
||||
stripped = line.strip()
|
||||
if stripped.startswith("export "):
|
||||
stripped = stripped[7:].lstrip()
|
||||
return stripped.startswith(f"{key}=")
|
||||
|
||||
|
||||
def save_env_value(key: str, value: str):
|
||||
"""Save or update a value in ~/.hermes/.env."""
|
||||
if is_managed():
|
||||
@@ -7925,10 +7939,15 @@ def save_env_value(key: str, value: str):
|
||||
|
||||
serialized_value = _quote_env_value(value)
|
||||
|
||||
# Find and update or append
|
||||
# Find and update or append. Match both ``KEY=`` and the bash-compatible
|
||||
# ``export KEY=`` form — load_env() parses export lines (#6659), so a
|
||||
# user-added ``export GITHUB_TOKEN=...`` shows as set in every UI. If the
|
||||
# writer didn't match it, a save would append a SECOND line and a later
|
||||
# delete of that line would silently resurrect the old exported value
|
||||
# (#40041: "token detected but cannot be replaced through the UI").
|
||||
found = False
|
||||
for i, line in enumerate(lines):
|
||||
if line.strip().startswith(f"{key}="):
|
||||
if _env_line_defines_key(line, key):
|
||||
lines[i] = f"{key}={serialized_value}\n"
|
||||
found = True
|
||||
break
|
||||
@@ -8007,7 +8026,7 @@ def remove_env_value(key: str) -> bool:
|
||||
lines = f.readlines()
|
||||
lines = _sanitize_env_lines(lines)
|
||||
|
||||
new_lines = [line for line in lines if not line.strip().startswith(f"{key}=")]
|
||||
new_lines = [line for line in lines if not _env_line_defines_key(line, key)]
|
||||
found = len(new_lines) < len(lines)
|
||||
|
||||
if found:
|
||||
@@ -8068,7 +8087,12 @@ def save_anthropic_api_key(value: str, save_fn=None):
|
||||
|
||||
|
||||
def save_env_value_secure(key: str, value: str) -> Dict[str, Any]:
|
||||
save_env_value(key, value)
|
||||
# Route through the unified credential lifecycle so a rotation via the
|
||||
# secret-capture path also refreshes any config.yaml mirror of the old
|
||||
# value and lifts a prior env-source suppression (#62269 fix family).
|
||||
from hermes_cli.credential_lifecycle import save_provider_env_credential
|
||||
|
||||
save_provider_env_credential(key, value)
|
||||
return {
|
||||
"success": True,
|
||||
"stored_as": key,
|
||||
@@ -8488,7 +8512,11 @@ def set_config_value(key: str, value: str):
|
||||
sys.exit(1)
|
||||
# Check if it's an API key (goes to .env)
|
||||
if _is_env_config_key(key):
|
||||
save_env_value(key.upper(), value)
|
||||
# Unified lifecycle: also rotates any config.yaml mirror of the old
|
||||
# value so a stale higher-precedence copy can't win (#62269).
|
||||
from hermes_cli.credential_lifecycle import save_provider_env_credential
|
||||
|
||||
save_provider_env_credential(key.upper(), value)
|
||||
print(f"✓ Set {key} in {get_env_path()}")
|
||||
return
|
||||
|
||||
@@ -8594,8 +8622,12 @@ def unset_config_value(key: str):
|
||||
sys.exit(1)
|
||||
|
||||
if _is_env_config_key(key):
|
||||
removed = remove_env_value(key.upper())
|
||||
if not removed:
|
||||
# Unified lifecycle: prune env-seeded credential_pool entries and
|
||||
# model-cache rows too, so `hermes config unset <KEY>` fully removes
|
||||
# the provider instead of leaving it resurrectable (#51071 family).
|
||||
from hermes_cli.credential_lifecycle import remove_provider_env_credential
|
||||
|
||||
if not remove_provider_env_credential(key.upper()).get("found"):
|
||||
print(f"Config key not set: {key}", file=sys.stderr)
|
||||
sys.exit(1)
|
||||
print(f"✓ Unset {key} from {get_env_path()}")
|
||||
|
||||
@@ -0,0 +1,272 @@
|
||||
"""Unified provider-credential lifecycle across every store Hermes reads.
|
||||
|
||||
A provider API key can live in up to THREE stores at once:
|
||||
|
||||
1. ``~/.hermes/.env`` — the canonical secret store
|
||||
2. ``~/.hermes/auth.json`` →
|
||||
``credential_pool.<provider>[*]`` — env-seeded pool entries
|
||||
(``source == "env:<VAR>"``) persisted by the pool loader
|
||||
3. ``~/.hermes/config.yaml`` — inline mirrors written by the
|
||||
custom-endpoint flows (``model.api_key``, ``auxiliary.<task>.api_key``,
|
||||
``custom_providers[*].api_key``)
|
||||
|
||||
Historically the desktop/dashboard endpoints (PUT/DELETE ``/api/env``) and the
|
||||
TUI-gateway RPCs only mutated store 1. That divergence is the root cause of a
|
||||
whole bug family:
|
||||
|
||||
* #51071 / #59761 — deleting a key removes it from ``.env`` but the stale
|
||||
``credential_pool`` entry (and ``provider_models_cache.json`` row)
|
||||
survives, so the provider keeps appearing in the model picker, even
|
||||
across restarts (the pool loader is additive-only).
|
||||
* #62269 — updating a key rewrites ``.env`` but leaves the OLD key in a
|
||||
higher-precedence ``config.yaml`` mirror (``model.api_key`` wins over
|
||||
env at client construction), producing persistent 401s with a key the
|
||||
UI no longer shows.
|
||||
|
||||
This module is the single choke point: every surface that saves or removes a
|
||||
provider credential should route through :func:`save_provider_env_credential`
|
||||
/ :func:`remove_provider_env_credential` so all three stores stay consistent.
|
||||
|
||||
OAuth preservation contract: removal only prunes credential-pool entries whose
|
||||
``source`` is exactly ``env:<VAR>``. OAuth/device-code/manual/borrowed entries
|
||||
(``device_code``, ``manual*``, ``gh_cli``, ``claude_code``, ``oauth``, …) and
|
||||
the ``providers.<id>`` OAuth token blocks in auth.json are never touched —
|
||||
deleting an API key must not revoke an OAuth grant for the same provider.
|
||||
|
||||
Secrecy contract: no function in this module logs, prints, or returns a
|
||||
credential value. Results carry key NAMES and config PATHS only.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any, Dict, List
|
||||
|
||||
__all__ = [
|
||||
"save_provider_env_credential",
|
||||
"remove_provider_env_credential",
|
||||
"purge_env_credential_references",
|
||||
]
|
||||
|
||||
|
||||
def _providers_for_env_var(env_var: str) -> List[str]:
|
||||
"""Provider ids whose registered api_key_env_vars include ``env_var``."""
|
||||
try:
|
||||
from hermes_cli.auth import PROVIDER_REGISTRY
|
||||
except Exception:
|
||||
return []
|
||||
hits: List[str] = []
|
||||
for pid, cfg in PROVIDER_REGISTRY.items():
|
||||
try:
|
||||
if env_var in (cfg.api_key_env_vars or ()):
|
||||
hits.append(pid)
|
||||
except Exception:
|
||||
continue
|
||||
return hits
|
||||
|
||||
|
||||
def _prune_env_pool_entries(env_var: str) -> List[str]:
|
||||
"""Drop ``credential_pool`` entries seeded from ``env:<env_var>``.
|
||||
|
||||
Operates across ALL providers in the pool (the source string names the
|
||||
env var unambiguously, and shared vars like GITHUB_TOKEN may seed more
|
||||
than one provider). Entries with any other source — OAuth, device-code,
|
||||
manual, borrowed-CLI — are preserved verbatim, as are the
|
||||
``providers.<id>`` OAuth blocks.
|
||||
|
||||
Returns the list of provider ids that had entries pruned.
|
||||
"""
|
||||
from hermes_cli.auth import _auth_store_lock, _load_auth_store, _save_auth_store
|
||||
|
||||
source = f"env:{env_var}"
|
||||
pruned: List[str] = []
|
||||
with _auth_store_lock():
|
||||
auth_store = _load_auth_store()
|
||||
pool = auth_store.get("credential_pool")
|
||||
if not isinstance(pool, dict):
|
||||
return pruned
|
||||
changed = False
|
||||
for provider in list(pool.keys()):
|
||||
entries = pool[provider]
|
||||
if not isinstance(entries, list):
|
||||
continue
|
||||
kept = [
|
||||
entry
|
||||
for entry in entries
|
||||
if not (isinstance(entry, dict) and entry.get("source") == source)
|
||||
]
|
||||
if len(kept) == len(entries):
|
||||
continue
|
||||
changed = True
|
||||
pruned.append(provider)
|
||||
if kept:
|
||||
pool[provider] = kept
|
||||
else:
|
||||
del pool[provider]
|
||||
if changed:
|
||||
_save_auth_store(auth_store)
|
||||
return pruned
|
||||
|
||||
|
||||
def _scrub_config_yaml_mirrors(old_value: str, new_value: str | None) -> List[str]:
|
||||
"""Reconcile config.yaml api_key mirrors that hold ``old_value``.
|
||||
|
||||
Value-matched on purpose: we only touch a config entry when it provably
|
||||
holds the SAME credential that just changed in ``.env`` — an independent
|
||||
key the user configured for a different endpoint is left alone.
|
||||
|
||||
``new_value=None`` removes the mirror field; a string replaces it.
|
||||
Operates on the RAW user config (never the defaults-merged view) so the
|
||||
write doesn't bake defaults into the user's file. Returns the dotted
|
||||
paths that were updated (names only — never values).
|
||||
"""
|
||||
if not old_value:
|
||||
return []
|
||||
from utils import atomic_yaml_write, fast_safe_load
|
||||
|
||||
from hermes_cli.config import (
|
||||
get_config_path,
|
||||
require_readable_config_before_write,
|
||||
)
|
||||
|
||||
config_path = get_config_path()
|
||||
if not config_path.exists():
|
||||
return []
|
||||
try:
|
||||
with open(config_path, encoding="utf-8") as f:
|
||||
user_config = fast_safe_load(f) or {}
|
||||
except Exception:
|
||||
return []
|
||||
if not isinstance(user_config, dict):
|
||||
return []
|
||||
|
||||
touched: List[str] = []
|
||||
|
||||
def _fix(section: Any, key_path: str) -> None:
|
||||
if not isinstance(section, dict):
|
||||
return
|
||||
# "api" is the legacy alias for model.api_key kept by older configs.
|
||||
for field in ("api_key", "api"):
|
||||
current = section.get(field)
|
||||
if isinstance(current, str) and current == old_value:
|
||||
if new_value:
|
||||
section[field] = new_value
|
||||
else:
|
||||
section.pop(field, None)
|
||||
touched.append(f"{key_path}.{field}")
|
||||
|
||||
_fix(user_config.get("model"), "model")
|
||||
|
||||
aux = user_config.get("auxiliary")
|
||||
if isinstance(aux, dict):
|
||||
for task, slot_cfg in aux.items():
|
||||
_fix(slot_cfg, f"auxiliary.{task}")
|
||||
|
||||
custom = user_config.get("custom_providers")
|
||||
if isinstance(custom, list):
|
||||
for idx, entry in enumerate(custom):
|
||||
_fix(entry, f"custom_providers.{idx}")
|
||||
elif isinstance(custom, dict):
|
||||
for name, entry in custom.items():
|
||||
_fix(entry, f"custom_providers.{name}")
|
||||
|
||||
if touched:
|
||||
require_readable_config_before_write(config_path)
|
||||
atomic_yaml_write(config_path, user_config, sort_keys=False)
|
||||
return touched
|
||||
|
||||
|
||||
def purge_env_credential_references(
|
||||
env_var: str, *, clear_models_cache: bool = True
|
||||
) -> Dict[str, Any]:
|
||||
"""Remove non-.env references to an env-var credential.
|
||||
|
||||
Prunes ``credential_pool`` env-seeded entries and (optionally) the
|
||||
affected providers' rows in ``provider_models_cache.json`` so the model
|
||||
picker stops advertising a provider whose key is gone (#59761).
|
||||
"""
|
||||
pruned = _prune_env_pool_entries(env_var)
|
||||
providers = sorted(set(pruned) | set(_providers_for_env_var(env_var)))
|
||||
# Make the removal sticky the same way `hermes auth remove` does: a
|
||||
# lingering shell export (or another live process's os.environ) would
|
||||
# otherwise re-seed the pool entry on the next load_pool(). The matching
|
||||
# save path lifts the suppression on an explicit re-add.
|
||||
try:
|
||||
from hermes_cli.auth import suppress_credential_source
|
||||
|
||||
for provider in providers:
|
||||
suppress_credential_source(provider, f"env:{env_var}")
|
||||
except Exception:
|
||||
pass
|
||||
if clear_models_cache and providers:
|
||||
try:
|
||||
from hermes_cli.models import clear_provider_models_cache
|
||||
|
||||
for provider in providers:
|
||||
clear_provider_models_cache(provider)
|
||||
except Exception:
|
||||
# Cache cleanup is best-effort — a failure here must not block
|
||||
# the credential removal itself.
|
||||
pass
|
||||
return {"pool_pruned": pruned, "providers": providers}
|
||||
|
||||
|
||||
def save_provider_env_credential(env_var: str, value: str) -> Dict[str, Any]:
|
||||
"""Save/update a credential in ``.env`` and reconcile every mirror.
|
||||
|
||||
After the ``.env`` write, any config.yaml mirror that held the PREVIOUS
|
||||
value of this var (``model.api_key`` etc.) is updated to the new value so
|
||||
a stale higher-precedence copy cannot shadow the rotation (#62269).
|
||||
Suppressed ``env:<VAR>`` pool sources are re-enabled so a deliberate
|
||||
re-add through the UI behaves like ``hermes auth add``.
|
||||
"""
|
||||
from hermes_cli.config import load_env, save_env_value
|
||||
|
||||
old_value = load_env().get(env_var)
|
||||
save_env_value(env_var, value)
|
||||
|
||||
config_updates: List[str] = []
|
||||
if value and old_value and old_value != value:
|
||||
config_updates = _scrub_config_yaml_mirrors(old_value, value)
|
||||
|
||||
# A prior UI/CLI removal may have suppressed this env source; a fresh
|
||||
# save is an explicit re-add, so lift the suppression for every provider
|
||||
# that reads this var.
|
||||
try:
|
||||
from hermes_cli.auth import unsuppress_credential_source
|
||||
|
||||
for provider in _providers_for_env_var(env_var):
|
||||
unsuppress_credential_source(provider, f"env:{env_var}")
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
return {"ok": True, "key": env_var, "config_updates": config_updates}
|
||||
|
||||
|
||||
def remove_provider_env_credential(env_var: str) -> Dict[str, Any]:
|
||||
"""Remove a credential from EVERY store it lives in.
|
||||
|
||||
Clears the ``.env`` entry (and process env), prunes env-seeded
|
||||
``credential_pool`` entries, drops the affected providers' model-cache
|
||||
rows, and removes any config.yaml mirror holding the same value.
|
||||
OAuth/device-code/manual credentials are preserved (see module docstring).
|
||||
|
||||
``found`` is True when ANY store held the credential — callers that
|
||||
previously 404'd on ".env miss" should key off this instead so a stale
|
||||
pool-only entry can still be cleaned up through the same button.
|
||||
"""
|
||||
from hermes_cli.config import load_env, remove_env_value
|
||||
|
||||
old_value = load_env().get(env_var)
|
||||
removed_from_env = remove_env_value(env_var)
|
||||
refs = purge_env_credential_references(env_var)
|
||||
config_scrubbed = _scrub_config_yaml_mirrors(old_value, None) if old_value else []
|
||||
|
||||
return {
|
||||
"ok": True,
|
||||
"key": env_var,
|
||||
"removed": removed_from_env,
|
||||
"pool_pruned": refs["pool_pruned"],
|
||||
"providers": refs["providers"],
|
||||
"config_scrubbed": config_scrubbed,
|
||||
"found": bool(removed_from_env or refs["pool_pruned"] or config_scrubbed),
|
||||
}
|
||||
@@ -6715,8 +6715,15 @@ async def get_env_vars(profile: Optional[str] = None):
|
||||
async def set_env_var(body: EnvVarUpdate, profile: Optional[str] = None):
|
||||
try:
|
||||
with _profile_scope(body.profile or profile):
|
||||
save_env_value(body.key, body.value)
|
||||
return {"ok": True, "key": body.key}
|
||||
# Unified credential lifecycle: writes .env AND reconciles any
|
||||
# config.yaml mirror still holding the previous value of this var
|
||||
# (model.api_key / auxiliary.*.api_key / custom_providers[*]),
|
||||
# so a rotation can't leave a stale higher-precedence copy that
|
||||
# keeps authenticating with the old key (#62269).
|
||||
from hermes_cli.credential_lifecycle import save_provider_env_credential
|
||||
|
||||
result = save_provider_env_credential(body.key, body.value)
|
||||
return result
|
||||
except ValueError as exc:
|
||||
# save_env_value raises ValueError for invalid names and for keys
|
||||
# on the denylist (LD_PRELOAD, PATH, PYTHONPATH, …). Surface the
|
||||
@@ -6834,10 +6841,18 @@ async def validate_provider_credential(body: EnvVarUpdate, request: Request):
|
||||
async def remove_env_var(body: EnvVarDelete, profile: Optional[str] = None):
|
||||
try:
|
||||
with _profile_scope(body.profile or profile):
|
||||
removed = remove_env_value(body.key)
|
||||
if not removed:
|
||||
# Unified credential lifecycle: clears the .env entry AND every
|
||||
# mirror of the credential — env-seeded credential_pool entries in
|
||||
# auth.json (stale ones kept providers alive in the model picker,
|
||||
# #51071/#59761), the affected providers' model-cache rows, and
|
||||
# value-matched config.yaml api_key mirrors. OAuth/device-code/
|
||||
# manual pool entries for the same provider are preserved.
|
||||
from hermes_cli.credential_lifecycle import remove_provider_env_credential
|
||||
|
||||
result = remove_provider_env_credential(body.key)
|
||||
if not result.get("found"):
|
||||
raise HTTPException(status_code=404, detail=f"{body.key} not found in .env")
|
||||
return {"ok": True, "key": body.key}
|
||||
return result
|
||||
except HTTPException:
|
||||
raise
|
||||
except ValueError as exc:
|
||||
|
||||
@@ -0,0 +1,335 @@
|
||||
"""E2E tests for the unified provider-credential lifecycle (#51071 #59761 #62269).
|
||||
|
||||
A provider API key can live in .env, auth.json's credential_pool, and
|
||||
config.yaml mirrors at once. These tests drive the REAL dashboard endpoint
|
||||
handlers (PUT/DELETE /api/env) against real on-disk fixtures in a temp
|
||||
HERMES_HOME (tests/conftest.py isolation) and assert every store agrees
|
||||
afterwards.
|
||||
|
||||
All fake secrets are constructed at runtime so no key-shaped literal ever
|
||||
lands in the repo.
|
||||
"""
|
||||
|
||||
import json
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
from hermes_cli.web_server import _SESSION_TOKEN, app
|
||||
|
||||
client = TestClient(app)
|
||||
HEADERS = {"X-Hermes-Session-Token": _SESSION_TOKEN}
|
||||
|
||||
# Runtime-constructed fake credentials (never literal key-shaped strings).
|
||||
FAKE_ZAI_KEY = "zk-" + "a" * 24
|
||||
FAKE_OAUTH_TOKEN = "oa-" + "b" * 24
|
||||
NEW_KEY = "zk-" + "c" * 24
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def hermes_home(monkeypatch, tmp_path):
|
||||
"""Fresh HERMES_HOME with .env + auth.json + config.yaml fixtures."""
|
||||
home = tmp_path / "cred_home"
|
||||
home.mkdir()
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
from hermes_cli.config import invalidate_env_cache
|
||||
|
||||
invalidate_env_cache()
|
||||
return home
|
||||
|
||||
|
||||
def _write_env(home, **pairs):
|
||||
home.joinpath(".env").write_text(
|
||||
"".join(f"{k}={v}\n" for k, v in pairs.items()), encoding="utf-8"
|
||||
)
|
||||
from hermes_cli.config import invalidate_env_cache
|
||||
|
||||
invalidate_env_cache()
|
||||
|
||||
|
||||
def _write_auth(home, pool):
|
||||
home.joinpath("auth.json").write_text(
|
||||
json.dumps({"credential_pool": pool}), encoding="utf-8"
|
||||
)
|
||||
|
||||
|
||||
def _read_auth(home):
|
||||
return json.loads(home.joinpath("auth.json").read_text(encoding="utf-8"))
|
||||
|
||||
|
||||
def _zai_pool_fixture():
|
||||
"""One env-seeded API-key entry plus one OAuth entry for the same provider."""
|
||||
return {
|
||||
"zai": [
|
||||
{
|
||||
"id": "e1",
|
||||
"label": "env",
|
||||
"auth_type": "api_key",
|
||||
"priority": 0,
|
||||
"source": "env:ZAI_API_KEY",
|
||||
"access_token": FAKE_ZAI_KEY,
|
||||
},
|
||||
{
|
||||
"id": "o1",
|
||||
"label": "oauth",
|
||||
"auth_type": "oauth",
|
||||
"priority": 0,
|
||||
"source": "device_code",
|
||||
"access_token": FAKE_OAUTH_TOKEN,
|
||||
"refresh_token": "rt-" + "d" * 16,
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# DELETE — #51071 / #59761: stale credential_pool entries must be pruned
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_delete_env_key_prunes_env_seeded_pool_entry(hermes_home):
|
||||
_write_env(hermes_home, ZAI_API_KEY=FAKE_ZAI_KEY)
|
||||
_write_auth(hermes_home, _zai_pool_fixture())
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
body = resp.json()
|
||||
assert body["ok"] is True
|
||||
assert "zai" in body["pool_pruned"]
|
||||
|
||||
# .env cleared
|
||||
from hermes_cli.config import load_env
|
||||
|
||||
assert "ZAI_API_KEY" not in load_env()
|
||||
|
||||
# auth.json: env-seeded entry gone, OAuth entry preserved
|
||||
store = _read_auth(hermes_home)
|
||||
sources = [e["source"] for e in store["credential_pool"]["zai"]]
|
||||
assert "env:ZAI_API_KEY" not in sources
|
||||
assert "device_code" in sources, "OAuth grant must survive an API-key delete"
|
||||
|
||||
|
||||
def test_delete_env_key_removes_provider_pool_key_when_emptied(hermes_home):
|
||||
"""A provider whose ONLY pool entry was env-seeded disappears entirely."""
|
||||
_write_env(hermes_home, ZAI_API_KEY=FAKE_ZAI_KEY)
|
||||
_write_auth(
|
||||
hermes_home,
|
||||
{"zai": [_zai_pool_fixture()["zai"][0]]}, # env entry only
|
||||
)
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
store = _read_auth(hermes_home)
|
||||
assert "zai" not in store.get("credential_pool", {}), (
|
||||
"provider must vanish from credential_pool so the model picker "
|
||||
"stops listing it (#51071)"
|
||||
)
|
||||
|
||||
|
||||
def test_delete_survives_pool_reload(hermes_home):
|
||||
"""#59761: the pool loader must not resurrect the entry after 'restart'."""
|
||||
_write_env(hermes_home, ZAI_API_KEY=FAKE_ZAI_KEY)
|
||||
_write_auth(hermes_home, {"zai": [_zai_pool_fixture()["zai"][0]]})
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
|
||||
# Simulate restart: reload the pool from disk the way startup does.
|
||||
from agent.credential_pool import load_pool
|
||||
|
||||
entries = load_pool("zai").entries()
|
||||
assert entries == [], f"stale entries resurrected: {[e.source for e in entries]}"
|
||||
|
||||
|
||||
def test_delete_clears_provider_models_cache(hermes_home):
|
||||
_write_env(hermes_home, ZAI_API_KEY=FAKE_ZAI_KEY)
|
||||
_write_auth(hermes_home, {"zai": [_zai_pool_fixture()["zai"][0]]})
|
||||
cache_path = hermes_home / "provider_models_cache.json"
|
||||
cache_path.write_text(
|
||||
json.dumps({"zai": {"models": ["glm-5"], "ts": 0}}), encoding="utf-8"
|
||||
)
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
if cache_path.exists():
|
||||
cache = json.loads(cache_path.read_text(encoding="utf-8"))
|
||||
assert "zai" not in cache
|
||||
|
||||
|
||||
def test_delete_pool_only_credential_still_cleans_up(hermes_home):
|
||||
"""Stale pool entry with NO .env line (the #59761 restart state) is
|
||||
removable through the same delete button instead of 404ing."""
|
||||
_write_env(hermes_home) # empty .env
|
||||
_write_auth(hermes_home, {"zai": [_zai_pool_fixture()["zai"][0]]})
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
store = _read_auth(hermes_home)
|
||||
assert "zai" not in store.get("credential_pool", {})
|
||||
|
||||
|
||||
def test_delete_unknown_key_404s(hermes_home):
|
||||
_write_env(hermes_home)
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "NEVER_SET_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
|
||||
|
||||
def test_delete_does_not_touch_other_providers(hermes_home):
|
||||
_write_env(hermes_home, ZAI_API_KEY=FAKE_ZAI_KEY)
|
||||
other_key = "dk-" + "e" * 24
|
||||
pool = _zai_pool_fixture()
|
||||
pool["deepseek"] = [
|
||||
{
|
||||
"id": "d1",
|
||||
"label": "env",
|
||||
"auth_type": "api_key",
|
||||
"priority": 0,
|
||||
"source": "env:DEEPSEEK_API_KEY",
|
||||
"access_token": other_key,
|
||||
}
|
||||
]
|
||||
_write_auth(hermes_home, pool)
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
store = _read_auth(hermes_home)
|
||||
assert [e["source"] for e in store["credential_pool"]["deepseek"]] == [
|
||||
"env:DEEPSEEK_API_KEY"
|
||||
]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# UPDATE — #62269: config.yaml mirrors of the old key must rotate with .env
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _write_config(home, text):
|
||||
home.joinpath("config.yaml").write_text(text, encoding="utf-8")
|
||||
|
||||
|
||||
def test_update_rotates_config_yaml_model_mirror(hermes_home):
|
||||
old = "sk-oe-" + "f" * 24
|
||||
new = "sk-oe-" + "g" * 24
|
||||
_write_env(hermes_home, OPENAI_API_KEY=old)
|
||||
_write_config(
|
||||
hermes_home,
|
||||
"model:\n"
|
||||
" provider: custom\n"
|
||||
" default: my-model\n"
|
||||
" base_url: https://llm.example.test/v1\n"
|
||||
f" api_key: {old}\n",
|
||||
)
|
||||
|
||||
resp = client.put(
|
||||
"/api/env", json={"key": "OPENAI_API_KEY", "value": new}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert "model.api_key" in resp.json().get("config_updates", [])
|
||||
|
||||
cfg_text = hermes_home.joinpath("config.yaml").read_text(encoding="utf-8")
|
||||
assert old not in cfg_text, "stale old key left in config.yaml (#62269)"
|
||||
assert new in cfg_text, "config.yaml mirror not rotated to the new key"
|
||||
|
||||
from hermes_cli.config import load_env
|
||||
|
||||
assert load_env()["OPENAI_API_KEY"] == new
|
||||
|
||||
|
||||
def test_update_rotates_custom_provider_mirror(hermes_home):
|
||||
old = "sk-cp-" + "h" * 24
|
||||
new = "sk-cp-" + "i" * 24
|
||||
_write_env(hermes_home, OPENAI_API_KEY=old)
|
||||
_write_config(
|
||||
hermes_home,
|
||||
"custom_providers:\n"
|
||||
" - name: myendpoint\n"
|
||||
" base_url: https://llm.example.test/v1\n"
|
||||
f" api_key: {old}\n",
|
||||
)
|
||||
|
||||
resp = client.put(
|
||||
"/api/env", json={"key": "OPENAI_API_KEY", "value": new}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
cfg_text = hermes_home.joinpath("config.yaml").read_text(encoding="utf-8")
|
||||
assert old not in cfg_text
|
||||
assert new in cfg_text
|
||||
|
||||
|
||||
def test_update_leaves_unrelated_config_keys_alone(hermes_home):
|
||||
"""A DIFFERENT key configured inline must not be rewritten by value-match."""
|
||||
old = "sk-un-" + "j" * 24
|
||||
unrelated = "sk-un-" + "k" * 24
|
||||
_write_env(hermes_home, OPENAI_API_KEY=old)
|
||||
_write_config(hermes_home, f"model:\n provider: custom\n api_key: {unrelated}\n")
|
||||
|
||||
resp = client.put(
|
||||
"/api/env",
|
||||
json={"key": "OPENAI_API_KEY", "value": "sk-un-" + "l" * 24},
|
||||
headers=HEADERS,
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
cfg_text = hermes_home.joinpath("config.yaml").read_text(encoding="utf-8")
|
||||
assert unrelated in cfg_text, "unrelated inline key must be preserved"
|
||||
|
||||
|
||||
def test_delete_scrubs_config_yaml_mirror(hermes_home):
|
||||
old = "sk-dl-" + "m" * 24
|
||||
_write_env(hermes_home, OPENAI_API_KEY=old)
|
||||
_write_config(hermes_home, f"model:\n provider: custom\n api_key: {old}\n")
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "OPENAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert "model.api_key" in resp.json()["config_scrubbed"]
|
||||
cfg_text = hermes_home.joinpath("config.yaml").read_text(encoding="utf-8")
|
||||
assert old not in cfg_text
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Suppression round-trip: delete sticks, re-add lifts it
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_delete_then_resave_round_trip(hermes_home):
|
||||
_write_env(hermes_home, ZAI_API_KEY=FAKE_ZAI_KEY)
|
||||
_write_auth(hermes_home, {"zai": [_zai_pool_fixture()["zai"][0]]})
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "ZAI_API_KEY"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
|
||||
from hermes_cli.auth import is_source_suppressed
|
||||
|
||||
assert is_source_suppressed("zai", "env:ZAI_API_KEY"), (
|
||||
"delete must suppress the env source so a lingering shell export "
|
||||
"can't re-seed the pool"
|
||||
)
|
||||
|
||||
resp = client.put(
|
||||
"/api/env", json={"key": "ZAI_API_KEY", "value": NEW_KEY}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert not is_source_suppressed("zai", "env:ZAI_API_KEY"), (
|
||||
"an explicit re-save must lift the suppression (like `hermes auth add`)"
|
||||
)
|
||||
|
||||
from hermes_cli.config import load_env
|
||||
|
||||
assert load_env()["ZAI_API_KEY"] == NEW_KEY
|
||||
@@ -0,0 +1,125 @@
|
||||
"""Regression tests for the Tools & Keys GitHub PAT save/remove path (#40041).
|
||||
|
||||
Users following generic docs add ``export GITHUB_TOKEN=ghp_...`` to
|
||||
``~/.hermes/.env``. ``load_env()`` parses the export prefix (#6659), so every
|
||||
UI shows the token as set (green light) — but ``save_env_value`` /
|
||||
``remove_env_value`` only matched plain ``KEY=`` lines. Result: the UI could
|
||||
neither replace nor remove the token (delete 404s as "not found in .env";
|
||||
save appends a duplicate line that a later delete removes while the export
|
||||
line silently resurrects the old value).
|
||||
|
||||
Fake tokens are constructed at runtime — no key-shaped literals on disk.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
from hermes_cli.web_server import _SESSION_TOKEN, app
|
||||
|
||||
client = TestClient(app)
|
||||
HEADERS = {"X-Hermes-Session-Token": _SESSION_TOKEN}
|
||||
|
||||
# Classic-PAT-shaped token, constructed at runtime (36 chars after prefix).
|
||||
OLD_PAT = "ghp_" + "A" * 36
|
||||
NEW_PAT = "ghp_" + "B" * 36
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def hermes_home(monkeypatch, tmp_path):
|
||||
home = tmp_path / "pat_home"
|
||||
home.mkdir()
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
from hermes_cli.config import invalidate_env_cache
|
||||
|
||||
invalidate_env_cache()
|
||||
return home
|
||||
|
||||
|
||||
def _write_env_raw(home, text):
|
||||
home.joinpath(".env").write_text(text, encoding="utf-8")
|
||||
from hermes_cli.config import invalidate_env_cache
|
||||
|
||||
invalidate_env_cache()
|
||||
|
||||
|
||||
def test_classic_pat_save_via_endpoint_succeeds(hermes_home):
|
||||
"""Saving a ghp_* classic PAT through the env endpoint must not 500 —
|
||||
GITHUB_TOKEN is a REST/Skills-Hub credential, not a Copilot one."""
|
||||
resp = client.put(
|
||||
"/api/env", json={"key": "GITHUB_TOKEN", "value": NEW_PAT}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200, resp.text
|
||||
|
||||
from hermes_cli.config import load_env
|
||||
|
||||
assert load_env()["GITHUB_TOKEN"] == NEW_PAT
|
||||
|
||||
|
||||
def test_remove_export_prefixed_token(hermes_home):
|
||||
"""DELETE must clear an ``export KEY=...`` line, not 404 on it."""
|
||||
_write_env_raw(hermes_home, f"export GITHUB_TOKEN={OLD_PAT}\n")
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "GITHUB_TOKEN"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200, (
|
||||
"export-prefixed lines are parsed by load_env (UI shows the token as "
|
||||
"set) so the delete path must recognise them too (#40041)"
|
||||
)
|
||||
|
||||
env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8")
|
||||
assert OLD_PAT not in env_text
|
||||
|
||||
from hermes_cli.config import load_env
|
||||
|
||||
assert "GITHUB_TOKEN" not in load_env()
|
||||
|
||||
|
||||
def test_update_export_prefixed_token_does_not_duplicate(hermes_home):
|
||||
"""Saving over an ``export KEY=`` line must replace it in place."""
|
||||
_write_env_raw(hermes_home, f"export GITHUB_TOKEN={OLD_PAT}\n")
|
||||
|
||||
resp = client.put(
|
||||
"/api/env", json={"key": "GITHUB_TOKEN", "value": NEW_PAT}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
|
||||
env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8")
|
||||
assert OLD_PAT not in env_text, "old exported token line must be replaced"
|
||||
assert env_text.count("GITHUB_TOKEN") == 1, (
|
||||
"save must not append a duplicate GITHUB_TOKEN line alongside the "
|
||||
"export-prefixed one"
|
||||
)
|
||||
|
||||
from hermes_cli.config import load_env
|
||||
|
||||
assert load_env()["GITHUB_TOKEN"] == NEW_PAT
|
||||
|
||||
|
||||
def test_plain_line_save_and_remove_still_work(hermes_home):
|
||||
"""Sanity: the ordinary KEY= path is unchanged."""
|
||||
from hermes_cli.config import load_env, remove_env_value, save_env_value
|
||||
|
||||
save_env_value("GITHUB_TOKEN", OLD_PAT)
|
||||
assert load_env()["GITHUB_TOKEN"] == OLD_PAT
|
||||
save_env_value("GITHUB_TOKEN", NEW_PAT)
|
||||
env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8")
|
||||
assert env_text.count("GITHUB_TOKEN") == 1
|
||||
assert remove_env_value("GITHUB_TOKEN") is True
|
||||
assert "GITHUB_TOKEN" not in load_env()
|
||||
|
||||
|
||||
def test_export_line_with_comment_untouched(hermes_home):
|
||||
"""Commented-out export lines are not live assignments — leave them."""
|
||||
_write_env_raw(
|
||||
hermes_home,
|
||||
f"# export GITHUB_TOKEN={OLD_PAT}\nOTHER_KEY=value\n",
|
||||
)
|
||||
|
||||
resp = client.request(
|
||||
"DELETE", "/api/env", json={"key": "GITHUB_TOKEN"}, headers=HEADERS
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8")
|
||||
assert "# export GITHUB_TOKEN=" in env_text
|
||||
assert "OTHER_KEY=value" in env_text
|
||||
+17
-7
@@ -14025,7 +14025,7 @@ def _(rid, params: dict) -> dict:
|
||||
"""
|
||||
try:
|
||||
from hermes_cli.auth import PROVIDER_REGISTRY
|
||||
from hermes_cli.config import is_managed, save_env_value
|
||||
from hermes_cli.config import is_managed
|
||||
from hermes_cli.inventory import build_models_payload, load_picker_context
|
||||
|
||||
slug = (params.get("slug") or "").strip()
|
||||
@@ -14049,9 +14049,13 @@ def _(rid, params: dict) -> dict:
|
||||
if not pconfig.api_key_env_vars:
|
||||
return _err(rid, 4004, f"no env var defined for {pconfig.name}")
|
||||
|
||||
# Save the key to ~/.hermes/.env
|
||||
# Save the key to ~/.hermes/.env via the unified credential lifecycle
|
||||
# so any stale config.yaml mirror of the previous key (model.api_key,
|
||||
# custom_providers[*].api_key) is rotated in the same action (#62269).
|
||||
env_var = pconfig.api_key_env_vars[0]
|
||||
save_env_value(env_var, api_key)
|
||||
from hermes_cli.credential_lifecycle import save_provider_env_credential
|
||||
|
||||
save_provider_env_credential(env_var, api_key)
|
||||
# Also set in current process so the refreshed inventory sees it.
|
||||
import os
|
||||
|
||||
@@ -14105,7 +14109,7 @@ def _(rid, params: dict) -> dict:
|
||||
"""
|
||||
try:
|
||||
from hermes_cli.auth import PROVIDER_REGISTRY, clear_provider_auth
|
||||
from hermes_cli.config import remove_env_value
|
||||
from hermes_cli.credential_lifecycle import remove_provider_env_credential
|
||||
|
||||
slug = (params.get("slug") or "").strip()
|
||||
if not slug:
|
||||
@@ -14115,13 +14119,19 @@ def _(rid, params: dict) -> dict:
|
||||
cleared_env = False
|
||||
cleared_auth = False
|
||||
|
||||
# Remove API key env vars from .env and process
|
||||
# Remove API key env vars from .env and process, plus every mirror
|
||||
# (env-seeded credential_pool entries, provider model cache rows,
|
||||
# value-matched config.yaml api_key copies) via the unified helper —
|
||||
# otherwise the provider resurrects in the picker after restart
|
||||
# (#51071 / #59761).
|
||||
if pconfig and pconfig.api_key_env_vars:
|
||||
for ev in pconfig.api_key_env_vars:
|
||||
if remove_env_value(ev):
|
||||
if remove_provider_env_credential(ev).get("found"):
|
||||
cleared_env = True
|
||||
|
||||
# Clear OAuth / credential pool state
|
||||
# Clear OAuth / credential pool state. This is a full provider
|
||||
# disconnect (TUI "disconnect" action), so removing OAuth grants
|
||||
# here is the documented intent — unlike the key-only delete paths.
|
||||
cleared_auth = clear_provider_auth(slug)
|
||||
|
||||
if not cleared_env and not cleared_auth:
|
||||
|
||||
Reference in New Issue
Block a user