fix(security): fail closed on null-owner session in sync-chat endpoint (#870)
POST /api/v1/chat (the n8n/Make/Activepieces sync-chat endpoint) verified session ownership with `_tok_user and _sess_owner and _sess_owner != _tok_user`. The `_sess_owner and` clause skipped the check entirely whenever the session's owner was null — so any chat-scoped API token (e.g. a token minted for a paired mobile device) could pass a legacy/migrated null-owner session id, inject a message into that session, and read back its conversation history plus reuse the owner's endpoint credentials. This is the same `if owner and owner != user` null-owner-bypass pattern that was already hardened in the gallery, calendar, and notes routes (see test_null_owner_gates.py) and in session_routes._verify_session_owner. Make this gate strict and fail closed too: require a resolvable caller and an exact owner match, mirroring _verify_session_owner. Extract the decision into _caller_owns_session() and pin it with regression tests.
This commit is contained in:
@@ -26,6 +26,25 @@ MAX_MESSAGE_LEN = 32_000
|
||||
from core.middleware import require_admin as _require_admin
|
||||
|
||||
|
||||
def _caller_owns_session(sess_owner, caller) -> bool:
|
||||
"""Strict session-ownership gate for the token-authenticated sync-chat
|
||||
endpoint (`POST /api/v1/chat`).
|
||||
|
||||
Mirrors ``_verify_session_owner`` in session_routes.py and the null-owner
|
||||
gates in notes/calendar/gallery: a caller may resume a session ONLY when
|
||||
its owner matches them exactly. A null/empty session owner (legacy or
|
||||
migrated rows) is deliberately NOT resumable by an arbitrary token — the
|
||||
old ``sess_owner and sess_owner != caller`` form skipped the check whenever
|
||||
``sess_owner`` was falsy, so any chat-scoped token (e.g. a paired mobile
|
||||
device) could resume such a session, inject a message, and read back its
|
||||
history and reuse the owner's endpoint credentials. Fail closed: an
|
||||
unresolvable caller also returns False.
|
||||
"""
|
||||
if not caller:
|
||||
return False
|
||||
return sess_owner == caller
|
||||
|
||||
|
||||
def setup_webhook_routes(
|
||||
webhook_manager: WebhookManager,
|
||||
auth_manager,
|
||||
@@ -228,8 +247,11 @@ def setup_webhook_routes(
|
||||
_tok_user = token_owner or getattr(request.state, "user", None) or _gcu(request)
|
||||
except Exception:
|
||||
_tok_user = None
|
||||
# Strict ownership (see _caller_owns_session): fail closed so a
|
||||
# null-owner / cross-owner session can't be resumed by an arbitrary
|
||||
# chat-scoped token.
|
||||
_sess_owner = getattr(sess, "owner", None)
|
||||
if _tok_user and _sess_owner and _sess_owner != _tok_user:
|
||||
if not _caller_owns_session(_sess_owner, _tok_user):
|
||||
raise HTTPException(404, "Session not found")
|
||||
|
||||
# --- Case 2: Direct API key + model (no pre-configured endpoint needed) ---
|
||||
|
||||
@@ -167,3 +167,54 @@ def test_gallery_owner_filter_passes_user():
|
||||
# logged-in users.
|
||||
fake_q.filter.assert_called_once()
|
||||
assert out is fake_q.filter.return_value
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# webhook._caller_owns_session (POST /api/v1/chat sync-chat endpoint)
|
||||
# ---------------------------------------------------------------------------
|
||||
# This is the FOURTH place the `owner and owner != user` pattern showed up:
|
||||
# the token-authenticated sync-chat endpoint let any chat-scoped token resume
|
||||
# a null-owner session by passing its id, leaking its history and reusing the
|
||||
# owner's endpoint credentials. The gate must fail closed, exactly like the
|
||||
# calendar/notes/gallery gates above and _verify_session_owner.
|
||||
|
||||
def _import_webhook_helper():
|
||||
"""Import routes.webhook_routes without dragging in the real webhook
|
||||
manager / database. Stub src.webhook_manager (only referenced by an
|
||||
import line) and ensure core.database exposes the names the import chain
|
||||
(core/__init__ → session_manager) looks up."""
|
||||
for _name in ("Webhook", "ChatMessage"):
|
||||
setattr(sys.modules["core.database"], _name, MagicMock())
|
||||
if "src.webhook_manager" not in sys.modules:
|
||||
wm = types.ModuleType("src.webhook_manager")
|
||||
wm.WebhookManager = MagicMock()
|
||||
wm.validate_webhook_url = MagicMock()
|
||||
wm.validate_events = MagicMock()
|
||||
sys.modules["src.webhook_manager"] = wm
|
||||
return __import__(
|
||||
"routes.webhook_routes", fromlist=["_caller_owns_session"]
|
||||
)
|
||||
|
||||
|
||||
def test_sync_chat_gate_rejects_null_owner_session():
|
||||
wh_mod = _import_webhook_helper()
|
||||
# Legacy/migrated session with no owner must NOT be resumable by a token.
|
||||
assert wh_mod._caller_owns_session(None, "alice") is False
|
||||
|
||||
|
||||
def test_sync_chat_gate_rejects_cross_owner_session():
|
||||
wh_mod = _import_webhook_helper()
|
||||
assert wh_mod._caller_owns_session("bob", "alice") is False
|
||||
|
||||
|
||||
def test_sync_chat_gate_rejects_unresolvable_caller():
|
||||
wh_mod = _import_webhook_helper()
|
||||
# If the token's owner can't be resolved, fail closed rather than opening
|
||||
# up null-owner sessions.
|
||||
assert wh_mod._caller_owns_session(None, None) is False
|
||||
assert wh_mod._caller_owns_session("alice", None) is False
|
||||
|
||||
|
||||
def test_sync_chat_gate_accepts_matching_owner():
|
||||
wh_mod = _import_webhook_helper()
|
||||
assert wh_mod._caller_owns_session("alice", "alice") is True
|
||||
|
||||
Reference in New Issue
Block a user