fix(profiles): profile isolation — new_session uses per-request profile, not process global (#800)
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 <nesquena@users.noreply.github.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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":
|
||||
|
||||
@@ -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', '')
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
180
tests/test_issue798.py
Normal file
180
tests/test_issue798.py
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user