From bc00a9fc7f90614336ffedd192c6490dff68e92e Mon Sep 17 00:00:00 2001 From: Mahdi Salmanzade Date: Tue, 2 Jun 2026 06:38:05 +0400 Subject: [PATCH] fix(security): fail closed on null-owner session in sync-chat endpoint (#870) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- routes/webhook_routes.py | 24 +++++++++++++++- tests/test_null_owner_gates.py | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/routes/webhook_routes.py b/routes/webhook_routes.py index 7eead00..b05b420 100644 --- a/routes/webhook_routes.py +++ b/routes/webhook_routes.py @@ -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) --- diff --git a/tests/test_null_owner_gates.py b/tests/test_null_owner_gates.py index e2a9ce6..417661d 100644 --- a/tests/test_null_owner_gates.py +++ b/tests/test_null_owner_gates.py @@ -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