fix(auth): enforce active-profile session IDs
This commit is contained in:
1 parent
95f1486567
commit
c068dbc39b
3 files changed
+368
-10
No files matched your search
+85
-10
@@ -512,6 +512,50 @@ def _session_visible_to_active_profile(session_profile, handler=None) -> bool:
|
||||
return _profiles_match(session_profile, active_profile)
|
||||
|
||||
|
||||
def _request_session_visibility_exempt(method: str, path: str | None) -> bool:
|
||||
if not path:
|
||||
return False
|
||||
if method != "POST":
|
||||
return False
|
||||
# Import routes create/claim sessions before normal ownership exists, and
|
||||
# chat/start has inline placeholder-retag rules that must run before the
|
||||
# generic request-session guard.
|
||||
return path in {
|
||||
"/api/session/import",
|
||||
"/api/session/import_cli",
|
||||
"/api/chat/start",
|
||||
}
|
||||
|
||||
|
||||
def _session_id_visible_to_request_profile(handler, sid) -> bool:
|
||||
"""Return whether ``sid`` belongs to the active profile."""
|
||||
if not isinstance(sid, str) or not sid:
|
||||
return True
|
||||
if not is_safe_session_id(sid):
|
||||
return True
|
||||
try:
|
||||
session = get_session(sid, metadata_only=True)
|
||||
except KeyError:
|
||||
return True
|
||||
if not _session_visible_to_active_profile(getattr(session, "profile", None), handler):
|
||||
bad(handler, "Session not found", 404)
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def _guard_request_session_visibility(handler, parsed, body=None, method="GET") -> bool:
|
||||
"""Apply request session-profile visibility check to request-supplied IDs."""
|
||||
method = str(method).upper()
|
||||
if _request_session_visibility_exempt(method, getattr(parsed, "path", "")):
|
||||
return True
|
||||
sid = parse_qs(parsed.query).get("session_id", [None])[0]
|
||||
if not _session_id_visible_to_request_profile(handler, sid):
|
||||
return False
|
||||
if isinstance(body, dict) and not _session_id_visible_to_request_profile(handler, body.get("session_id")):
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def _active_skills_dir() -> Path:
|
||||
"""Return the skills directory for the request's active Hermes profile.
|
||||
|
||||
@@ -10012,6 +10056,9 @@ def handle_get(handler, parsed) -> bool:
|
||||
handler.end_headers()
|
||||
return True
|
||||
|
||||
if parsed.path.startswith("/api/") and not _guard_request_session_visibility(handler, parsed, method="GET"):
|
||||
return True
|
||||
|
||||
# ── Insights / knowledge status ──
|
||||
if parsed.path == "/api/insights":
|
||||
return _handle_insights(handler, parsed)
|
||||
@@ -11605,6 +11652,10 @@ def handle_post(handler, parsed) -> bool:
|
||||
if diag:
|
||||
diag.finish()
|
||||
raise
|
||||
if not _guard_request_session_visibility(handler, parsed, body=body, method="POST"):
|
||||
if diag:
|
||||
diag.finish()
|
||||
return True
|
||||
|
||||
if parsed.path == "/api/escape/authorize":
|
||||
return _handle_escape_authorize(handler, parsed, body)
|
||||
@@ -13758,6 +13809,8 @@ def handle_patch(handler, parsed) -> bool:
|
||||
if not _check_csrf(handler):
|
||||
return j(handler, {"error": _csrf_rejection_error(handler)}, status=403)
|
||||
body = read_body(handler)
|
||||
if not _guard_request_session_visibility(handler, parsed, body=body, method="PATCH"):
|
||||
return True
|
||||
if parsed.path.startswith("/api/mcp/servers/"):
|
||||
name = parsed.path[len("/api/mcp/servers/"):]
|
||||
return _handle_mcp_server_toggle(handler, name, body)
|
||||
@@ -13776,6 +13829,8 @@ def handle_delete(handler, parsed) -> bool:
|
||||
if not _check_csrf(handler):
|
||||
return j(handler, {"error": _csrf_rejection_error(handler)}, status=403)
|
||||
body = read_body(handler)
|
||||
if not _guard_request_session_visibility(handler, parsed, body=body, method="DELETE"):
|
||||
return True
|
||||
if parsed.path.startswith("/api/mcp/servers/"):
|
||||
name = parsed.path[len("/api/mcp/servers/"):]
|
||||
return _handle_mcp_server_delete(handler, name)
|
||||
@@ -13802,6 +13857,8 @@ def handle_put(handler, parsed) -> bool:
|
||||
if not _check_csrf(handler):
|
||||
return j(handler, {"error": "Cross-origin request rejected"}, status=403)
|
||||
body = read_body(handler)
|
||||
if not _guard_request_session_visibility(handler, parsed, body=body, method="PUT"):
|
||||
return True
|
||||
if parsed.path.startswith("/api/mcp/servers/"):
|
||||
name = parsed.path[len("/api/mcp/servers/"):]
|
||||
return _handle_mcp_server_update(handler, name, body)
|
||||
@@ -18049,6 +18106,7 @@ def _handle_chat_start(handler, body, diag=None):
|
||||
return bad(handler, "Read-only imported sessions cannot be continued from WebUI", 403)
|
||||
diag.stage("validate_profile") if diag else None
|
||||
requested_profile = str(body.get("profile") or "").strip()
|
||||
active_profile = _get_active_profile_name()
|
||||
if requested_profile:
|
||||
try:
|
||||
from api.profiles import _PROFILE_ID_RE
|
||||
@@ -18057,17 +18115,34 @@ def _handle_chat_start(handler, body, diag=None):
|
||||
return bad(handler, "invalid profile", 400)
|
||||
except ImportError:
|
||||
requested_profile = ""
|
||||
if requested_profile and not _profiles_match(getattr(s, "profile", None), requested_profile):
|
||||
has_persisted_turns = bool(
|
||||
getattr(s, "messages", None)
|
||||
or getattr(s, "context_messages", None)
|
||||
or getattr(s, "pending_user_message", None)
|
||||
)
|
||||
session_profile = getattr(s, "profile", None)
|
||||
has_persisted_turns = bool(
|
||||
getattr(s, "messages", None)
|
||||
or getattr(s, "context_messages", None)
|
||||
or getattr(s, "pending_user_message", None)
|
||||
)
|
||||
if not _session_visible_to_active_profile(session_profile, handler):
|
||||
if (
|
||||
requested_profile
|
||||
and _profiles_match(requested_profile, active_profile)
|
||||
and not has_persisted_turns
|
||||
):
|
||||
# Empty placeholders can still be retagged when the
|
||||
# requested profile matches the active request profile.
|
||||
s.profile = requested_profile
|
||||
else:
|
||||
return bad(handler, "Session not found", 404)
|
||||
elif (
|
||||
requested_profile
|
||||
and _profiles_match(requested_profile, active_profile)
|
||||
and not _profiles_match(session_profile, requested_profile)
|
||||
):
|
||||
# Empty sessions are placeholders. If the user switches profiles
|
||||
# before sending the first turn, run the placeholder under the
|
||||
# active request profile instead of the stale one stamped at
|
||||
# creation time. The request body alone must not retag a session to
|
||||
# a different profile than the server-selected active profile.
|
||||
if not has_persisted_turns:
|
||||
# Empty sessions are placeholders. If the user switches profiles
|
||||
# before sending the first turn, run the placeholder under the
|
||||
# currently-selected profile instead of the stale one stamped at
|
||||
# creation time.
|
||||
s.profile = requested_profile
|
||||
diag.stage("normalize_message") if diag else None
|
||||
msg = str(body.get("message", "")).strip()
|
||||
|
||||
@@ -11,6 +11,7 @@ from pathlib import Path
|
||||
from api.config import MAX_UPLOAD_BYTES, STATE_DIR
|
||||
from api.helpers import j, bad
|
||||
from api.models import get_session
|
||||
from api.profiles import _profiles_match, get_active_profile_name as _get_active_profile_name
|
||||
from api.workspace import (
|
||||
safe_resolve_ws,
|
||||
resolve_trusted_workspace,
|
||||
@@ -150,6 +151,21 @@ def _session_attachment_dir(session_id: str, *, root: Path | None = None) -> Pat
|
||||
return dest_dir
|
||||
|
||||
|
||||
def _session_visible_to_active_profile(session) -> bool:
|
||||
"""Return whether an upload target session belongs to the active profile."""
|
||||
session_profile = getattr(session, 'profile', None)
|
||||
if not isinstance(session_profile, str):
|
||||
session_profile = None
|
||||
return _profiles_match(session_profile, _get_active_profile_name())
|
||||
|
||||
|
||||
def _reject_invisible_session(handler, session) -> bool:
|
||||
if _session_visible_to_active_profile(session):
|
||||
return False
|
||||
j(handler, {'error': 'Session not found'}, status=404)
|
||||
return True
|
||||
|
||||
|
||||
def handle_upload(handler):
|
||||
import traceback as _tb
|
||||
try:
|
||||
@@ -168,6 +184,8 @@ def handle_upload(handler):
|
||||
s = get_session(session_id)
|
||||
except KeyError:
|
||||
return j(handler, {'error': 'Session not found'}, status=404)
|
||||
if _reject_invisible_session(handler, s):
|
||||
return True
|
||||
safe_name = _sanitize_upload_name(filename)
|
||||
dest = _upload_destination(session_id, safe_name)
|
||||
dest.write_bytes(file_bytes)
|
||||
@@ -344,6 +362,8 @@ def handle_upload_extract(handler):
|
||||
s = get_session(session_id)
|
||||
except KeyError:
|
||||
return j(handler, {'error': 'Session not found'}, status=404)
|
||||
if _reject_invisible_session(handler, s):
|
||||
return True
|
||||
session_dir = _session_attachment_dir(session_id)
|
||||
session_dir.mkdir(parents=True, exist_ok=True)
|
||||
result = extract_archive(file_bytes, filename, session_dir)
|
||||
@@ -563,6 +583,8 @@ def handle_workspace_upload(handler):
|
||||
session = get_session(session_id)
|
||||
except KeyError:
|
||||
return j(handler, {'error': 'Session not found'}, status=404)
|
||||
if _reject_invisible_session(handler, session):
|
||||
return True
|
||||
|
||||
# Resolve workspace root from session
|
||||
workspace = resolve_trusted_workspace(session.workspace)
|
||||
|
||||
@@ -0,0 +1,261 @@
|
||||
"""Regression tests for session-profile visibility on request-scoped session_id uses.
|
||||
|
||||
The security contract is now enforced with a generic preflight for request-supplied
|
||||
session IDs. These tests cover the most sensitive paths that previously loaded
|
||||
foreign-profile sessions directly: duplicate, file reads, and chat/start.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import io
|
||||
from urllib.parse import urlparse
|
||||
|
||||
import api.routes as routes
|
||||
import api.upload as upload
|
||||
|
||||
|
||||
class _FakeHandler:
|
||||
def __init__(self):
|
||||
self.status = None
|
||||
self.headers = {"Content-Type": "multipart/form-data; boundary=test", "Content-Length": "1"}
|
||||
self.rfile = io.BytesIO(b"")
|
||||
self.wfile = io.BytesIO()
|
||||
self.command = "GET"
|
||||
self.path = "/"
|
||||
self.client_address = ("127.0.0.1", 12345)
|
||||
|
||||
def send_response(self, status):
|
||||
self.status = status
|
||||
|
||||
def send_header(self, key, value):
|
||||
self.headers[key] = value
|
||||
|
||||
def end_headers(self):
|
||||
pass
|
||||
|
||||
|
||||
def _capture(monkeypatch):
|
||||
cap = {}
|
||||
|
||||
def _j(_h, obj, *_, **__):
|
||||
cap["ok"] = obj
|
||||
return True
|
||||
|
||||
def _bad(_h, msg, code=400):
|
||||
cap["bad"] = (msg, code)
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(routes, "j", _j)
|
||||
monkeypatch.setattr(routes, "bad", _bad)
|
||||
return cap
|
||||
|
||||
|
||||
def _capture_upload(monkeypatch):
|
||||
cap = {}
|
||||
|
||||
def _j(_h, obj, *_, **kwargs):
|
||||
cap["ok"] = obj
|
||||
cap["status"] = kwargs.get("status", 200)
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(upload, "j", _j)
|
||||
return cap
|
||||
|
||||
|
||||
class _SimpleSession:
|
||||
def __init__(self, sid, profile="default", workspace="/workspace", messages=None, context_messages=None, pending_user_message=None):
|
||||
self.session_id = sid
|
||||
self.profile = profile
|
||||
self.workspace = workspace
|
||||
self.model = "test-model"
|
||||
self.model_provider = None
|
||||
self.title = "Test"
|
||||
self.messages = messages or []
|
||||
self.tool_calls = []
|
||||
self.project_id = None
|
||||
self.context_messages = context_messages
|
||||
self.pending_user_message = pending_user_message
|
||||
self.personality = None
|
||||
self.enabled_toolsets = None
|
||||
self.context_length = None
|
||||
self.threshold_tokens = None
|
||||
self.truncation_watermark = None
|
||||
self.truncation_boundary = None
|
||||
self.gateway_routing = None
|
||||
self.gateway_routing_history = []
|
||||
self.llm_title_generated = False
|
||||
self.manual_title = False
|
||||
self.composer_draft = {}
|
||||
self.context_engine = None
|
||||
self.context_engine_state = {}
|
||||
self.input_tokens = 0
|
||||
self.output_tokens = 0
|
||||
self.estimated_cost = 0.0
|
||||
self.cache_read_tokens = 0
|
||||
self.cache_write_tokens = 0
|
||||
|
||||
|
||||
def test_session_duplicate_foreign_profile_session_blocked_by_visibility_guard(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
foreign = _SimpleSession("foreign_duplicate", profile="other")
|
||||
monkeypatch.setattr(routes, "get_session", lambda sid, metadata_only=False: foreign)
|
||||
monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True)
|
||||
monkeypatch.setattr(routes, "read_body", lambda _handler: {"session_id": "foreign_duplicate"})
|
||||
monkeypatch.setattr(routes.Session, "load", staticmethod(lambda sid: (_ for _ in ()).throw(AssertionError("duplicate should not materialize foreign session"))))
|
||||
|
||||
cap = _capture(monkeypatch)
|
||||
routes.handle_post(handler, urlparse("/api/session/duplicate"))
|
||||
|
||||
assert cap["bad"] == ("Session not found", 404)
|
||||
|
||||
|
||||
def test_session_duplicate_same_profile_still_duplicates(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
source = _SimpleSession("session_visible", profile="default")
|
||||
calls = {"load": 0, "save": 0}
|
||||
|
||||
def _load(_sid):
|
||||
calls["load"] += 1
|
||||
return source
|
||||
|
||||
monkeypatch.setattr(routes.Session, "load", staticmethod(_load))
|
||||
monkeypatch.setattr(routes, "get_session", lambda sid, metadata_only=False: source)
|
||||
monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True)
|
||||
monkeypatch.setattr(routes, "read_body", lambda _handler: {"session_id": "session_visible"})
|
||||
|
||||
def _session_save(_self, *_, **__):
|
||||
calls["save"] += 1
|
||||
|
||||
monkeypatch.setattr(routes.Session, "save", _session_save)
|
||||
monkeypatch.setattr(routes, "publish_session_list_changed", lambda *_, **__: None)
|
||||
|
||||
cap = _capture(monkeypatch)
|
||||
routes.handle_post(handler, urlparse("/api/session/duplicate"))
|
||||
|
||||
assert calls["load"] == 1
|
||||
assert calls["save"] == 1
|
||||
assert "bad" not in cap
|
||||
assert cap["ok"]["session"]["session_id"] != "session_visible"
|
||||
|
||||
|
||||
def test_file_read_foreign_profile_session_returns_404_before_file_ops(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
foreign = _SimpleSession("foreign_file", profile="other", workspace="/workspace")
|
||||
monkeypatch.setattr(routes, "get_session", lambda sid, metadata_only=False: foreign)
|
||||
monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(routes, "get_session_for_file_ops", lambda sid: (_ for _ in ()).throw(AssertionError("file ops should not run")))
|
||||
cap = _capture(monkeypatch)
|
||||
|
||||
routes.handle_get(handler, urlparse("/api/file?session_id=foreign_file&path=notes.txt"))
|
||||
|
||||
assert cap["bad"] == ("Session not found", 404)
|
||||
|
||||
|
||||
def test_chat_start_foreign_persisted_session_returns_404_before_start_run(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
persisted_foreign = _SimpleSession(
|
||||
"chat_foreign",
|
||||
profile="other",
|
||||
messages=[{"role": "user", "content": "first"}],
|
||||
)
|
||||
|
||||
monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True)
|
||||
monkeypatch.setattr(routes, "read_body", lambda _handler: {"session_id": "chat_foreign", "message": "hello"})
|
||||
monkeypatch.setattr(routes, "_get_or_materialize_session", lambda sid: persisted_foreign)
|
||||
monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(routes, "_start_run", lambda *_, **__: (_ for _ in ()).throw(AssertionError("_start_run should not run")))
|
||||
|
||||
cap = _capture(monkeypatch)
|
||||
routes.handle_post(handler, urlparse("/api/chat/start"))
|
||||
|
||||
assert cap["bad"] == ("Session not found", 404)
|
||||
|
||||
|
||||
def test_chat_start_body_profile_cannot_retag_visible_empty_session_without_active_profile(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
empty_visible = _SimpleSession("chat_empty", profile="default", messages=[])
|
||||
captured = {}
|
||||
|
||||
monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True)
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"read_body",
|
||||
lambda _handler: {"session_id": "chat_empty", "message": "hello", "profile": "other"},
|
||||
)
|
||||
monkeypatch.setattr(routes, "_get_or_materialize_session", lambda sid: empty_visible)
|
||||
monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(routes, "_resolve_chat_workspace_with_recovery", lambda *_args, **_kwargs: "/workspace")
|
||||
monkeypatch.setattr(routes, "_read_profile_model_config", lambda *_args, **_kwargs: (None, None))
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"_resolve_compatible_session_model_state",
|
||||
lambda *_args, **_kwargs: ("test-model", None, "test-model"),
|
||||
)
|
||||
|
||||
def _start_run(session, **_kwargs):
|
||||
captured["profile"] = session.profile
|
||||
return {"ok": True, "stream_id": "stream-test", "session_id": session.session_id}
|
||||
|
||||
monkeypatch.setattr(routes, "_start_run", _start_run)
|
||||
|
||||
cap = _capture(monkeypatch)
|
||||
routes.handle_post(handler, urlparse("/api/chat/start"))
|
||||
|
||||
assert "bad" not in cap
|
||||
assert captured["profile"] == "default"
|
||||
|
||||
|
||||
def test_attachment_upload_foreign_profile_session_returns_404_before_write(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
foreign = _SimpleSession("upload_foreign", profile="other")
|
||||
monkeypatch.setattr(upload, "parse_multipart", lambda *_args: ({"session_id": "upload_foreign"}, {"file": ("note.txt", b"x")}))
|
||||
monkeypatch.setattr(upload, "get_session", lambda sid: foreign)
|
||||
monkeypatch.setattr(upload, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(
|
||||
upload,
|
||||
"_upload_destination",
|
||||
lambda *_args, **_kwargs: (_ for _ in ()).throw(AssertionError("upload destination should not be resolved")),
|
||||
)
|
||||
|
||||
cap = _capture_upload(monkeypatch)
|
||||
upload.handle_upload(handler)
|
||||
|
||||
assert cap == {"ok": {"error": "Session not found"}, "status": 404}
|
||||
|
||||
|
||||
def test_archive_upload_extract_foreign_profile_session_returns_404_before_extract(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
foreign = _SimpleSession("extract_foreign", profile="other")
|
||||
monkeypatch.setattr(upload, "parse_multipart", lambda *_args: ({"session_id": "extract_foreign"}, {"file": ("archive.zip", b"zip")}))
|
||||
monkeypatch.setattr(upload, "get_session", lambda sid: foreign)
|
||||
monkeypatch.setattr(upload, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(
|
||||
upload,
|
||||
"extract_archive",
|
||||
lambda *_args, **_kwargs: (_ for _ in ()).throw(AssertionError("archive extraction should not run")),
|
||||
)
|
||||
|
||||
cap = _capture_upload(monkeypatch)
|
||||
upload.handle_upload_extract(handler)
|
||||
|
||||
assert cap == {"ok": {"error": "Session not found"}, "status": 404}
|
||||
|
||||
|
||||
def test_workspace_upload_foreign_profile_session_returns_404_before_workspace_resolution(monkeypatch):
|
||||
handler = _FakeHandler()
|
||||
foreign = _SimpleSession("workspace_foreign", profile="other", workspace="/workspace/foreign")
|
||||
monkeypatch.setattr(upload, "parse_multipart", lambda *_args: ({"session_id": "workspace_foreign"}, {"file": ("note.txt", b"x")}))
|
||||
monkeypatch.setattr(upload, "get_session", lambda sid: foreign)
|
||||
monkeypatch.setattr(upload, "_get_active_profile_name", lambda: "default")
|
||||
monkeypatch.setattr(
|
||||
upload,
|
||||
"resolve_trusted_workspace",
|
||||
lambda *_args, **_kwargs: (_ for _ in ()).throw(AssertionError("workspace should not be resolved")),
|
||||
)
|
||||
|
||||
cap = _capture_upload(monkeypatch)
|
||||
upload.handle_workspace_upload(handler)
|
||||
|
||||
assert cap == {"ok": {"error": "Session not found"}, "status": 404}
|
||||
Reference in new issue
Block a user