From cbb4ba3f2803bc98bc8e88c1ceacb9588515cd68 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 21 Apr 2026 09:16:51 -0700 Subject: [PATCH] =?UTF-8?q?fix(profiles):=20profile=20isolation=20?= =?UTF-8?q?=E2=80=94=20new=5Fsession=20uses=20per-request=20profile,=20not?= =?UTF-8?q?=20process=20global=20(#800)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the multi-client profile isolation bug (#798). - get_hermes_home_for_profile(): pure path resolver, validates name against _PROFILE_ID_RE (rejects path traversal), never mutates os.environ or globals - new_session() accepts explicit profile= param from POST body (S.activeProfile), short-circuits the process-level _active_profile global - streaming handler resolves HERMES_HOME from s.profile instead of the global - sessions.js sends profile: S.activeProfile in every new-session POST 10 tests in tests/test_issue798.py including concurrency and traversal coverage. Co-authored-by: nesquena --- CHANGELOG.md | 5 ++ api/models.py | 25 ++++-- api/profiles.py | 20 +++++ api/routes.py | 4 +- api/streaming.py | 9 ++- static/sessions.js | 2 +- tests/test_issue798.py | 180 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 232 insertions(+), 13 deletions(-) create mode 100644 tests/test_issue798.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e1bec55..588d4b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Hermes Web UI -- Changelog +## [v0.50.127] — 2026-04-21 + +### Fixed +- **Profile isolation: switching profiles in one browser client no longer affects concurrent clients** — `api/profiles.py` stored `_active_profile` as a process-level global; `switch_profile()` mutated it for the whole server, so a second user switching profiles would clobber new-session creation for all other active tabs. The fix: (1) `get_hermes_home_for_profile(name)` — a pure path resolver that reads only the filesystem, validates the profile name against the existing `_PROFILE_ID_RE` pattern (rejects path traversal), and never mutates `os.environ` or module state; (2) `new_session()` now accepts an explicit `profile` param passed from the client's `S.activeProfile` in the POST body, short-circuiting the process global; (3) the streaming handler resolves `HERMES_HOME` from the per-session `s.profile` instead of the shared global. Reported in #798. (#800) + ## [v0.50.126] — 2026-04-21 ### Fixed diff --git a/api/models.py b/api/models.py index ec1634f..aa95af0 100644 --- a/api/models.py +++ b/api/models.py @@ -176,18 +176,27 @@ def get_session(sid): return s raise KeyError(sid) -def new_session(workspace=None, model=None): - # Use the live config-derived default so Hermes config changes apply without restart. - try: - from api.profiles import get_active_profile_name - _profile = get_active_profile_name() - except ImportError: - _profile = None +def new_session(workspace=None, model=None, profile=None): + """Create a new in-memory session and persist it. + + *profile* — when supplied by the caller (e.g. from the request body sent + by the active browser tab), it is used directly so that concurrent clients + on different profiles don't fight over a shared process-global. If not + supplied, we fall back to the process-level active profile (the pre-#798 + behaviour, preserved for calls that originate outside a request context). + """ + if profile is None: + # Fallback: read process-level global (single-client or startup path) + try: + from api.profiles import get_active_profile_name + profile = get_active_profile_name() + except ImportError: + profile = None effective_model = model or get_effective_default_model() s = Session( workspace=workspace or get_last_workspace(), model=effective_model, - profile=_profile, + profile=profile, ) with LOCK: SESSIONS[s.session_id] = s diff --git a/api/profiles.py b/api/profiles.py index 26b0864..ae8e363 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -100,6 +100,26 @@ def get_active_hermes_home() -> Path: return _DEFAULT_HERMES_HOME + +def get_hermes_home_for_profile(name: str) -> Path: + """Return the HERMES_HOME Path for *name* without mutating any process state. + + Safe to call from per-request context (streaming, session creation) because + it reads only the filesystem — it never touches os.environ, module-level + cached paths, or the process-level _active_profile global. + + Falls back to _DEFAULT_HERMES_HOME (same as 'default') when *name* is None, + empty, 'default', or does not match the profile-name format (rejects path + traversal such as '../../etc'). + """ + if not name or name == 'default' or not _PROFILE_ID_RE.match(name): + return _DEFAULT_HERMES_HOME + profile_dir = _DEFAULT_HERMES_HOME / 'profiles' / name + if profile_dir.is_dir(): + return profile_dir + return _DEFAULT_HERMES_HOME + + def _set_hermes_home(home: Path): """Set HERMES_HOME env var and monkey-patch cached module-level paths.""" os.environ['HERMES_HOME'] = str(home) diff --git a/api/routes.py b/api/routes.py index 5be4784..a244046 100644 --- a/api/routes.py +++ b/api/routes.py @@ -886,7 +886,9 @@ def handle_post(handler, parsed) -> bool: workspace = str(resolve_trusted_workspace(body.get("workspace"))) if body.get("workspace") else None except ValueError as e: return bad(handler, str(e)) - s = new_session(workspace=workspace, model=body.get("model")) + # Use the profile sent by the client tab (if any) so that two tabs on + # different profiles never clobber each other via the process-level global. + s = new_session(workspace=workspace, model=body.get("model"), profile=body.get("profile") or None) return j(handler, {"session": s.compact() | {"messages": s.messages}}) if parsed.path == "/api/default-model": diff --git a/api/streaming.py b/api/streaming.py index 6c5d784..2f6fb3e 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -834,10 +834,13 @@ def _run_agent_streaming(session_id, msg_text, model, workspace, stream_id, atta put('cancel', {'message': 'Cancelled before start'}) return - # Resolve profile home for this agent run (snapshot at start) + # Resolve profile home for this agent run — use the session's own profile + # (stamped at new_session() time from the client's S.activeProfile) so that + # two concurrent tabs on different profiles don't clobber each other via the + # process-level active-profile global. Falls back gracefully. try: - from api.profiles import get_active_hermes_home - _profile_home = str(get_active_hermes_home()) + from api.profiles import get_hermes_home_for_profile + _profile_home = str(get_hermes_home_for_profile(getattr(s, 'profile', None))) except ImportError: _profile_home = os.environ.get('HERMES_HOME', '') diff --git a/static/sessions.js b/static/sessions.js index 9a04380..b7c8024 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -18,7 +18,7 @@ async function newSession(flash){ // otherwise inherit from the current session (or let server pick the default) const inheritWs=S._profileDefaultWorkspace||(S.session?S.session.workspace:null); S._profileDefaultWorkspace=null; // consume — only applies to the first new session after switch - const data=await api('/api/session/new',{method:'POST',body:JSON.stringify({model:$('modelSelect').value,workspace:inheritWs})}); + const data=await api('/api/session/new',{method:'POST',body:JSON.stringify({model:$('modelSelect').value,workspace:inheritWs,profile:S.activeProfile||'default'})}); S.session=data.session;S.messages=data.session.messages||[]; S.lastUsage={...(data.session.last_usage||{})}; if(flash)S.session._flash=true; diff --git a/tests/test_issue798.py b/tests/test_issue798.py new file mode 100644 index 0000000..cfe5144 --- /dev/null +++ b/tests/test_issue798.py @@ -0,0 +1,180 @@ +""" +Issue #798 — Profile isolation: switching profile in one browser client must not +affect sessions created by other concurrent clients. + +Root cause: _active_profile was a process-level global in api/profiles.py. +Fix: new_session() now accepts an explicit `profile` param passed from the client +request body (S.activeProfile), which bypasses the shared global entirely. +get_hermes_home_for_profile() resolves a HERMES_HOME path from a name without +touching os.environ or module-level state. +""" + +import os +import sys +import threading +from pathlib import Path +from unittest.mock import patch + +import pytest + + +# ── R19: get_hermes_home_for_profile ───────────────────────────────────────── + +def test_get_hermes_home_for_profile_returns_default_for_none(): + """R19a: None / empty string / 'default' all return the base home.""" + import api.profiles as p + base = p._DEFAULT_HERMES_HOME + assert p.get_hermes_home_for_profile(None) == base + assert p.get_hermes_home_for_profile('') == base + assert p.get_hermes_home_for_profile('default') == base + + +def test_get_hermes_home_for_profile_returns_profile_subdir(tmp_path, monkeypatch): + """R19b: Named profile that exists returns its subdirectory.""" + import api.profiles as p + + profile_dir = tmp_path / 'profiles' / 'alice' + profile_dir.mkdir(parents=True) + monkeypatch.setattr(p, '_DEFAULT_HERMES_HOME', tmp_path) + result = p.get_hermes_home_for_profile('alice') + assert result == profile_dir + + +def test_get_hermes_home_for_profile_falls_back_for_missing_profile(tmp_path, monkeypatch): + """R19c: Named profile that does not exist falls back to base home.""" + import api.profiles as p + + monkeypatch.setattr(p, '_DEFAULT_HERMES_HOME', tmp_path) + result = p.get_hermes_home_for_profile('ghost') + assert result == tmp_path + + +def test_get_hermes_home_for_profile_does_not_mutate_globals(): + """R19d: get_hermes_home_for_profile() must never change _active_profile or os.environ.""" + import api.profiles as p + + before_active = p._active_profile + before_hermes_home = os.environ.get('HERMES_HOME') + + p.get_hermes_home_for_profile('some-other-profile') + + assert p._active_profile == before_active, ( + "get_hermes_home_for_profile() must not mutate _active_profile" + ) + assert os.environ.get('HERMES_HOME') == before_hermes_home, ( + "get_hermes_home_for_profile() must not mutate os.environ['HERMES_HOME']" + ) + + +# ── R19e-h: new_session() profile isolation ─────────────────────────────────── +# These tests call new_session() directly in-process. Session.save() would write +# to SESSION_DIR which is set from HERMES_WEBUI_STATE_DIR at import time and may +# point to a test-scoped tmp dir that has already been torn down. We patch save() +# to a no-op — the tests only care about s.profile, not persistence. + +def test_new_session_uses_explicit_profile_not_global(): + """R19e: new_session(profile='alice') stamps session.profile='alice' even when + the process-level _active_profile is 'default'. + Core fix for #798: client B's session is tagged to B's profile, not the global. + """ + import api.profiles as p + import api.models as m + + original = p._active_profile + try: + p._active_profile = 'default' + with patch.object(m.Session, 'save', return_value=None): + s = m.new_session(profile='alice') + assert s.profile == 'alice', ( + f"Expected s.profile='alice', got {s.profile!r}. " + "new_session() should use the explicit profile param, not the global." + ) + finally: + p._active_profile = original + + +def test_new_session_falls_back_to_global_when_profile_not_supplied(): + """R19f: new_session() without explicit profile still reads _active_profile (backward compat).""" + import api.profiles as p + import api.models as m + + original = p._active_profile + try: + p._active_profile = 'default' + with patch.object(m.Session, 'save', return_value=None): + s = m.new_session() + assert s.profile == 'default' + finally: + p._active_profile = original + + +def test_new_session_none_profile_falls_back_to_global(): + """R19g: profile=None explicitly also falls back to the global (same as omitting it).""" + import api.profiles as p + import api.models as m + + original = p._active_profile + try: + p._active_profile = 'default' + with patch.object(m.Session, 'save', return_value=None): + s = m.new_session(profile=None) + assert s.profile == 'default' + finally: + p._active_profile = original + + +def test_concurrent_new_sessions_get_correct_profiles(): + """R19h: Two threads call new_session() with different explicit profiles simultaneously. + Each session must be stamped with its own profile, never the other's. + Direct reproduction of the #798 race (minus the actual switch_profile() call). + """ + import api.models as m + + results = {} + errors = [] + + def make_session(profile_name, key): + try: + with patch.object(m.Session, 'save', return_value=None): + s = m.new_session(profile=profile_name) + results[key] = s.profile + except Exception as exc: + errors.append(exc) + + t1 = threading.Thread(target=make_session, args=('alice', 'alice')) + t2 = threading.Thread(target=make_session, args=('bob', 'bob')) + t1.start(); t2.start() + t1.join(timeout=5); t2.join(timeout=5) + + assert not errors, f"Threads raised: {errors}" + assert results.get('alice') == 'alice', f"alice session had profile {results.get('alice')!r}" + assert results.get('bob') == 'bob', f"bob session had profile {results.get('bob')!r}" + + +# ── R19i: sessions.js sends profile in the POST body ───────────────────────── + +def test_sessions_js_sends_profile_in_new_session_post(): + """R19i: sessions.js newSession() must include profile:S.activeProfile in the + JSON body sent to /api/session/new — the client-side half of the #798 fix.""" + js = (Path(__file__).parent.parent / 'static' / 'sessions.js').read_text() + assert 'profile:S.activeProfile' in js or 'profile: S.activeProfile' in js, ( + "sessions.js newSession() must send profile: S.activeProfile in the POST body " + "so the server uses the tab's active profile, not the process global." + ) + + +def test_get_hermes_home_for_profile_rejects_path_traversal(): + """R19j: get_hermes_home_for_profile() must reject names that don't match + _PROFILE_ID_RE (e.g. path traversal like '../../etc') and return the base home. + The regex guard is defence-in-depth on top of the is_dir() fallback.""" + import api.profiles as p + base = p._DEFAULT_HERMES_HOME + assert p.get_hermes_home_for_profile('../../etc') == base + assert p.get_hermes_home_for_profile('../escape') == base + assert p.get_hermes_home_for_profile('/absolute/path') == base + assert p.get_hermes_home_for_profile('has spaces') == base + assert p.get_hermes_home_for_profile('UPPERCASE') == base + # Valid names still work + assert p.get_hermes_home_for_profile('alice') == base # nonexistent → fallback + assert p.get_hermes_home_for_profile('my-profile') == base + assert p.get_hermes_home_for_profile('profile_1') == base