From 3246b263d9600d885d12ecacf0d661e0cfcd5fad Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 21 Apr 2026 10:04:11 -0700 Subject: [PATCH] fix(profiles): complete profile isolation via cookie + thread-local (#805) Closes the gap left by #800. Full isolation via hermes_profile cookie + TLS. Co-authored-by: bergeouss --- CHANGELOG.md | 5 ++ api/helpers.py | 56 ++++++++++++- api/profiles.py | 101 +++++++++++++++++----- api/routes.py | 10 ++- server.py | 15 +++- tests/test_issue803.py | 184 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 345 insertions(+), 26 deletions(-) create mode 100644 tests/test_issue803.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 247e5de..63608c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Hermes Web UI -- Changelog +## [v0.50.129] — 2026-04-21 + +### Fixed +- **Profile isolation: complete fix via cookie + thread-local context** — PR #800 (v0.50.127) only fixed `POST /api/session/new`. `GET /api/profile/active` still read the process-level `_active_profile` global, so a page refresh while another client had a different profile active would corrupt `S.activeProfile` in JS, defeating the session-creation fix on the next new chat. This release completes the isolation: profile switches now set a `hermes_profile` cookie (HttpOnly, SameSite=Lax) and never mutate the process global. Every request handler reads the cookie into a thread-local; all server functions (`get_active_profile_name()`, `get_active_hermes_home()`, `list_profiles_api()`, memory endpoints, model loading) automatically see the per-client profile. `switch_profile()` gains a `process_wide` kwarg — the HTTP route passes `False`, keeping the global clean; CLI callers default to `True` (unchanged behaviour). Absorbed from PR #803 by @bergeouss with correctness fixes reviewed by Opus. (#804-not-yet — see PR #803 absorption) + ## [v0.50.128] — 2026-04-21 ### Fixed diff --git a/api/helpers.py b/api/helpers.py index 95c7a0e..5c984f0 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -54,14 +54,21 @@ def _security_headers(handler): ) -def j(handler, payload, status: int=200) -> None: - """Send a JSON response.""" +def j(handler, payload, status: int=200, extra_headers: dict=None) -> None: + """Send a JSON response. + + *extra_headers*: optional dict of additional headers to include + (e.g., {'Set-Cookie': '...'}). Headers are sent before end_headers(). + """ body = _json.dumps(payload, ensure_ascii=False, indent=2).encode('utf-8') handler.send_response(status) handler.send_header('Content-Type', 'application/json; charset=utf-8') handler.send_header('Content-Length', str(len(body))) handler.send_header('Cache-Control', 'no-store') _security_headers(handler) + if extra_headers: + for k, v in extra_headers.items(): + handler.send_header(k, v) handler.end_headers() handler.wfile.write(body) @@ -173,3 +180,48 @@ def read_body(handler) -> dict: return _json.loads(raw) except Exception: return {} + + +# ── Profile cookie helpers (issue #798) ───────────────────────────────────── + +PROFILE_COOKIE_NAME = 'hermes_profile' + + +def get_profile_cookie(handler) -> str | None: + """Extract the hermes_profile cookie value from the request, or None.""" + cookie_header = handler.headers.get('Cookie', '') + if not cookie_header: + return None + import http.cookies as _hc + cookie = _hc.SimpleCookie() + try: + cookie.load(cookie_header) + except _hc.CookieError: + return None + morsel = cookie.get(PROFILE_COOKIE_NAME) + if morsel and morsel.value: + # Validate against profile-name pattern before trusting + from api.profiles import _PROFILE_ID_RE + val = morsel.value + if val == 'default' or _PROFILE_ID_RE.fullmatch(val): + return val + return None + + +def build_profile_cookie(name: str) -> str: + """Build a Set-Cookie header value for the hermes_profile cookie. + + name='default' clears the cookie (max-age=0). + Any other valid profile name sets it for the browser session. + httponly=True: the JS reads profile from /api/profile/active JSON, never + from document.cookie, so httponly exposure is unnecessary. + """ + import http.cookies as _hc + cookie = _hc.SimpleCookie() + cookie[PROFILE_COOKIE_NAME] = '' if name == 'default' else name + cookie[PROFILE_COOKIE_NAME]['path'] = '/' + cookie[PROFILE_COOKIE_NAME]['httponly'] = True + cookie[PROFILE_COOKIE_NAME]['samesite'] = 'Lax' + if name == 'default': + cookie[PROFILE_COOKIE_NAME]['max-age'] = '0' + return cookie[PROFILE_COOKIE_NAME].OutputString() diff --git a/api/profiles.py b/api/profiles.py index ae8e363..fbeb74b 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -31,6 +31,12 @@ _active_profile = 'default' _profile_lock = threading.Lock() _loaded_profile_env_keys: set[str] = set() +# Thread-local profile context: set per-request by server.py, cleared after. +# Enables per-client profile isolation (issue #798) — each HTTP request thread +# reads its own profile from the hermes_profile cookie instead of the +# process-global _active_profile. +_tls = threading.local() + def _resolve_base_hermes_home() -> Path: """Return the BASE ~/.hermes directory — the root that contains profiles/. @@ -86,15 +92,47 @@ def _read_active_profile_file() -> str: # ── Public API ────────────────────────────────────────────────────────────── def get_active_profile_name() -> str: - """Return the currently active profile name.""" + """Return the currently active profile name. + + Priority: + 1. Thread-local (set per-request from hermes_profile cookie) — issue #798 + 2. Process-level default (_active_profile) + """ + tls_name = getattr(_tls, 'profile', None) + if tls_name is not None: + return tls_name return _active_profile +def set_request_profile(name: str) -> None: + """Set the per-request profile context for this thread. + + Called by server.py at the start of each request when a hermes_profile + cookie is present. Always paired with clear_request_profile() in a + finally block so the thread-local is released after the request. + """ + _tls.profile = name + + +def clear_request_profile() -> None: + """Clear the per-request profile context for this thread. + + Called by server.py in the finally block of do_GET / do_POST. + Safe to call even if set_request_profile() was never called. + """ + _tls.profile = None + + def get_active_hermes_home() -> Path: - """Return the HERMES_HOME path for the currently active profile.""" - if _active_profile == 'default': + """Return the HERMES_HOME path for the currently active profile. + + Uses get_active_profile_name() so per-request TLS context (issue #798) + is respected, not just the process-level global. + """ + name = get_active_profile_name() + if name == 'default': return _DEFAULT_HERMES_HOME - profile_dir = _DEFAULT_HERMES_HOME / 'profiles' / _active_profile + profile_dir = _DEFAULT_HERMES_HOME / 'profiles' / name if profile_dir.is_dir(): return profile_dir return _DEFAULT_HERMES_HOME @@ -190,12 +228,18 @@ def init_profile_state() -> None: _reload_dotenv(home) -def switch_profile(name: str) -> dict: +def switch_profile(name: str, *, process_wide: bool = True) -> dict: """Switch the active profile. Validates the profile exists, updates process state, patches module caches, reloads .env, and reloads config.yaml. + Args: + name: Profile name to switch to. + process_wide: If True (default), updates the process-global + _active_profile. Set to False for per-client switches from the + WebUI where the profile is managed via cookie + thread-local (#798). + Returns: {'profiles': [...], 'active': name} Raises ValueError if profile doesn't exist or agent is busy. """ @@ -221,24 +265,41 @@ def switch_profile(name: str) -> dict: raise ValueError(f"Profile '{name}' does not exist.") with _profile_lock: - _active_profile = name - _set_hermes_home(home) - _reload_dotenv(home) + if process_wide: + global _active_profile + _active_profile = name + _set_hermes_home(home) + _reload_dotenv(home) - # Write sticky default for CLI consistency - try: - ap_file = _DEFAULT_HERMES_HOME / 'active_profile' - ap_file.write_text(name if name != 'default' else '', encoding='utf-8') - except Exception: - logger.debug("Failed to write active profile file") + if process_wide: + # Write sticky default for CLI consistency + try: + ap_file = _DEFAULT_HERMES_HOME / 'active_profile' + ap_file.write_text(name if name != 'default' else '', encoding='utf-8') + except Exception: + logger.debug("Failed to write active profile file") - # Reload config.yaml from the new profile - reload_config() + # Reload config.yaml from the new profile + reload_config() - # Return profile-specific defaults so frontend can apply them + # Return profile-specific defaults so frontend can apply them. + # For process_wide=False (per-client switch), read the target profile's + # config.yaml directly from disk rather than from _cfg_cache (process-global), + # since reload_config() was intentionally skipped. from api.workspace import get_last_workspace - from api.config import get_config - cfg = get_config() + if process_wide: + from api.config import get_config + cfg = get_config() + else: + # Direct disk read — does not touch _cfg_cache + try: + import yaml as _yaml + cfg_path = home / 'config.yaml' + cfg = _yaml.safe_load(cfg_path.read_text(encoding='utf-8')) if cfg_path.exists() else {} + if not isinstance(cfg, dict): + cfg = {} + except Exception: + cfg = {} model_cfg = cfg.get('model', {}) default_model = None if isinstance(model_cfg, str): @@ -263,7 +324,7 @@ def list_profiles_api() -> list: # hermes_cli not available -- return just the default return [_default_profile_dict()] - active = _active_profile + active = get_active_profile_name() result = [] for p in infos: result.append({ diff --git a/api/routes.py b/api/routes.py index a244046..83082b6 100644 --- a/api/routes.py +++ b/api/routes.py @@ -1153,11 +1153,15 @@ def handle_post(handler, parsed) -> bool: return bad(handler, "name is required") try: from api.profiles import switch_profile, _validate_profile_name - + from api.helpers import build_profile_cookie if name != 'default': _validate_profile_name(name) - result = switch_profile(name) - return j(handler, result) + # process_wide=False: don't mutate the process-global _active_profile. + # Per-client profile is managed via cookie + thread-local (#798). + result = switch_profile(name, process_wide=False) + return j(handler, result, extra_headers={ + 'Set-Cookie': build_profile_cookie(name), + }) except (ValueError, FileNotFoundError) as e: return bad(handler, _sanitize_error(e), 404) except RuntimeError as e: diff --git a/server.py b/server.py index 5d8c4b1..39f8b6f 100644 --- a/server.py +++ b/server.py @@ -15,7 +15,8 @@ logger = logging.getLogger(__name__) from api.auth import check_auth from api.config import HOST, PORT, STATE_DIR, SESSION_DIR, DEFAULT_WORKSPACE -from api.helpers import j +from api.helpers import j, get_profile_cookie +from api.profiles import set_request_profile, clear_request_profile from api.routes import handle_get, handle_post from api.startup import auto_install_agent_deps, fix_credential_permissions from api.updates import WEBUI_VERSION @@ -64,6 +65,10 @@ class Handler(BaseHTTPRequestHandler): def do_GET(self) -> None: self._req_t0 = time.time() + # Per-request profile context from cookie (issue #798) + cookie_profile = get_profile_cookie(self) + if cookie_profile: + set_request_profile(cookie_profile) try: parsed = urlparse(self.path) if not check_auth(self, parsed): return @@ -73,9 +78,15 @@ class Handler(BaseHTTPRequestHandler): except Exception as e: print(f'[webui] ERROR {self.command} {self.path}\n' + traceback.format_exc(), flush=True) return j(self, {'error': 'Internal server error'}, status=500) + finally: + clear_request_profile() def do_POST(self) -> None: self._req_t0 = time.time() + # Per-request profile context from cookie (issue #798) + cookie_profile = get_profile_cookie(self) + if cookie_profile: + set_request_profile(cookie_profile) try: parsed = urlparse(self.path) if not check_auth(self, parsed): return @@ -85,6 +96,8 @@ class Handler(BaseHTTPRequestHandler): except Exception as e: print(f'[webui] ERROR {self.command} {self.path}\n' + traceback.format_exc(), flush=True) return j(self, {'error': 'Internal server error'}, status=500) + finally: + clear_request_profile() def main() -> None: diff --git a/tests/test_issue803.py b/tests/test_issue803.py new file mode 100644 index 0000000..9190362 --- /dev/null +++ b/tests/test_issue803.py @@ -0,0 +1,184 @@ +""" +Issue #803 (completes #798) — per-client profile isolation via cookie + thread-local. + +PR #800 fixed POST /api/session/new (client sends profile in body). +PR #805 extends the fix to ALL endpoints: profile switches set a hermes_profile +cookie, server.py reads it per-request into a thread-local, and the existing +api/profiles.py helpers consult the thread-local before the process global. + +Covers: + 1. build_profile_cookie() / get_profile_cookie() roundtrip + validation + 2. set_request_profile() / get_active_profile_name() / clear_request_profile() + 3. get_active_hermes_home() routes via thread-local + 4. switch_profile(process_wide=False) does NOT mutate process globals + 5. Concurrent requests on different threads see independent profiles +""" +import os +import threading +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + + +# ── 1. Cookie build/parse roundtrip ────────────────────────────────────────── + +class TestProfileCookieHelpers: + + def test_build_profile_cookie_sets_value(self): + from api.helpers import build_profile_cookie + s = build_profile_cookie('alice') + assert 'hermes_profile=alice' in s + assert 'HttpOnly' in s + assert 'SameSite=Lax' in s + assert 'Path=/' in s + + def test_build_profile_cookie_default_clears(self): + from api.helpers import build_profile_cookie + s = build_profile_cookie('default') + assert 'Max-Age=0' in s + # Empty value indicates clear + assert 'hermes_profile=""' in s or 'hermes_profile=;' in s + + def test_get_profile_cookie_returns_none_when_absent(self): + from api.helpers import get_profile_cookie + handler = MagicMock() + handler.headers.get = lambda k, d='': '' + assert get_profile_cookie(handler) is None + + def test_get_profile_cookie_extracts_valid_name(self): + from api.helpers import get_profile_cookie + handler = MagicMock() + handler.headers.get = lambda k, d='': 'hermes_profile=alice' if k == 'Cookie' else d + assert get_profile_cookie(handler) == 'alice' + + def test_get_profile_cookie_accepts_default(self): + from api.helpers import get_profile_cookie + handler = MagicMock() + handler.headers.get = lambda k, d='': 'hermes_profile=default' if k == 'Cookie' else d + assert get_profile_cookie(handler) == 'default' + + def test_get_profile_cookie_rejects_injection(self): + """Cookie value must pass _PROFILE_ID_RE fullmatch — rejects traversal/injection.""" + from api.helpers import get_profile_cookie + for bad in ('../etc', 'a/b', 'name;DROP', 'WithCaps', 'has space', '.hidden'): + handler = MagicMock() + handler.headers.get = lambda k, d='', v=bad: f'hermes_profile={v}' if k == 'Cookie' else d + assert get_profile_cookie(handler) is None, f"{bad!r} should be rejected" + + def test_get_profile_cookie_ignores_malformed_header(self): + from api.helpers import get_profile_cookie + handler = MagicMock() + handler.headers.get = lambda k, d='': '\x00\x01not-a-cookie' if k == 'Cookie' else d + # Must not raise; returns None + result = get_profile_cookie(handler) + assert result is None + + +# ── 2. Thread-local request context ────────────────────────────────────────── + +class TestThreadLocalProfileContext: + + def test_tls_takes_priority_over_global(self): + import api.profiles as p + original = p._active_profile + try: + p._active_profile = 'global-default' + p.set_request_profile('alice') + assert p.get_active_profile_name() == 'alice' + finally: + p.clear_request_profile() + p._active_profile = original + + def test_global_used_when_tls_cleared(self): + import api.profiles as p + original = p._active_profile + try: + p._active_profile = 'global-default' + p.set_request_profile('alice') + p.clear_request_profile() + assert p.get_active_profile_name() == 'global-default' + finally: + p._active_profile = original + + def test_clear_is_idempotent(self): + import api.profiles as p + # Calling clear on a thread that never set anything must not raise + p.clear_request_profile() + p.clear_request_profile() + + +# ── 3. get_active_hermes_home routes through TLS ───────────────────────────── + +def test_get_active_hermes_home_respects_tls(tmp_path, monkeypatch): + import api.profiles as p + monkeypatch.setattr(p, '_DEFAULT_HERMES_HOME', tmp_path) + profile_dir = tmp_path / 'profiles' / 'alice' + profile_dir.mkdir(parents=True) + try: + p.set_request_profile('alice') + assert p.get_active_hermes_home() == profile_dir + p.set_request_profile('default') + assert p.get_active_hermes_home() == tmp_path + finally: + p.clear_request_profile() + + +# ── 4. switch_profile(process_wide=False) does not mutate globals ───────────── + +def test_switch_profile_process_wide_false_does_not_mutate_global(): + """Per-client switches from the WebUI must leave _active_profile untouched.""" + import api.profiles as p + + # Monkey in a fake profile listing so switch_profile finds 'alice' + original_global = p._active_profile + original_env_home = os.environ.get('HERMES_HOME') + + # We need a profile that exists to get past the validation path. + # Use 'default' — switch_profile accepts it without requiring hermes_cli. + try: + result = p.switch_profile('default', process_wide=False) + # Global must not change + assert p._active_profile == original_global, ( + f"process_wide=False must not mutate _active_profile " + f"(was {original_global!r}, now {p._active_profile!r})" + ) + # HERMES_HOME env must not change + assert os.environ.get('HERMES_HOME') == original_env_home, ( + "process_wide=False must not mutate os.environ['HERMES_HOME']" + ) + # Response still shape-compatible + assert isinstance(result, dict) + finally: + p._active_profile = original_global + + +# ── 5. Concurrent threads see independent profile context ──────────────────── + +def test_concurrent_threads_see_independent_profiles(): + """The whole point of thread-local isolation: two threads, two cookies, + two different get_active_profile_name() results, simultaneously.""" + import api.profiles as p + + results = {} + errors = [] + barrier = threading.Barrier(2, timeout=5) + + def worker(name, key): + try: + p.set_request_profile(name) + barrier.wait() # both threads have set their TLS + # Now each thread reads — must see its own value + results[key] = p.get_active_profile_name() + p.clear_request_profile() + except Exception as exc: + errors.append(exc) + + t1 = threading.Thread(target=worker, args=('alice', 'alice')) + t2 = threading.Thread(target=worker, args=('bob', 'bob')) + t1.start(); t2.start() + t1.join(timeout=10); t2.join(timeout=10) + + assert not errors, f"Workers raised: {errors}" + assert results.get('alice') == 'alice', f"alice thread saw {results.get('alice')!r}" + assert results.get('bob') == 'bob', f"bob thread saw {results.get('bob')!r}"