diff --git a/navi/api/routes/sessions.py b/navi/api/routes/sessions.py index 20890cd..c157f9d 100644 --- a/navi/api/routes/sessions.py +++ b/navi/api/routes/sessions.py @@ -45,6 +45,28 @@ pinned: bool +def _session_owner(user) -> str | None: + """Owner id for session create/list. + + In local mode (auth disabled) the anonymous user is a separate user with + user_id=None — not an alias of every other user — so sessions must be + persisted ownerless and listings must scope to NULL-owner rows only. + """ + return user.id if settings.navi_auth_enabled else None + + +def _session_admin(user) -> bool: + """Listing-scope admin flag — only meaningful when auth is enabled. + + The local-mode anonymous user is nominally 'admin' so debug/admin + endpoints keep working, but the admin flag must never widen the chat + session listings beyond NULL-owner rows there. + """ + return settings.navi_auth_enabled and ( + user.role == "admin" or user.has_permission("navi.sessions.read_all") + ) + + @router.post("", status_code=201) async def create_session( payload: CreateSessionRequest, @@ -64,10 +86,10 @@ except ProfileNotFound: raise HTTPException(status_code=404, detail=f"Profile '{profile_id}' not found") - session = await store.create(profile_id, user_id=user.id) + session = await store.create(profile_id, user_id=_session_owner(user)) # Fire-and-forget: extract memory from any stale sessions (last active > 30 min ago) - asyncio.create_task(_process_stale_sessions(store, memory, user_id=user.id)) + asyncio.create_task(_process_stale_sessions(store, memory, user_id=_session_owner(user))) return { "session_id": session.id, @@ -115,10 +137,10 @@ profile_id: str | None = None, search: str | None = Query(None), ) -> dict | list[dict]: - is_admin = user.role == "admin" or user.has_permission("navi.sessions.read_all") + is_admin = _session_admin(user) if limit is None and offset is None: - sessions = await store.list_all(user_id=user.id, is_admin=is_admin) + sessions = await store.list_all(user_id=_session_owner(user), is_admin=is_admin) if profile_id and not search: sessions = [s for s in sessions if s.profile_id == profile_id] pending_ids = await scheduler.get_pending_session_ids([s.id for s in sessions]) @@ -130,7 +152,7 @@ sessions = await store.search_list( limit=page_limit + 1, offset=page_offset, - user_id=user.id, + user_id=_session_owner(user), is_admin=is_admin, search=search, ) @@ -139,7 +161,7 @@ limit=page_limit + 1, offset=page_offset, profile_id=profile_id, - user_id=user.id, + user_id=_session_owner(user), is_admin=is_admin, ) items = sessions[:page_limit] diff --git a/navi/auth/deps.py b/navi/auth/deps.py index 2d735ab..d187954 100644 --- a/navi/auth/deps.py +++ b/navi/auth/deps.py @@ -493,11 +493,15 @@ """Raise 403 if user does not own the session and lacks the required permission. Legacy sessions (user_id=None) are accessible only to admins. - When auth is disabled, all sessions are shared by the single anonymous user. + When auth is disabled, the anonymous local user is a separate user with + user_id=None: only NULL-owner sessions are visible — other users' chats + must never leak into local mode. """ from fastapi import HTTPException if not settings.navi_auth_enabled: + if session.user_id is not None: + raise HTTPException(status_code=403, detail="Access denied") return if session.user_id is None: diff --git a/navi/core/pg_session_store.py b/navi/core/pg_session_store.py index c0419df..55cfd03 100644 --- a/navi/core/pg_session_store.py +++ b/navi/core/pg_session_store.py @@ -757,9 +757,20 @@ return result == "UPDATE 1" async def list_all(self, user_id: str | None = None, is_admin: bool = False) -> list[Session]: + """Sessions visible to the caller. + + Scoping: admins see everything; a named owner sees their own rows; + ``user_id=None`` (local mode, the anonymous local user) sees only + NULL-owner sessions — never other users' chats. + """ pool = await self._get_pool() async with pool.acquire() as conn: - if not is_admin and user_id is not None: + if is_admin: + rows = await conn.fetch( + "SELECT id, profile_id, user_id, pinned, created_at, last_active, context_token_count, name, planning_logs, next_sequence, archive_threshold, session_metadata " + "FROM sessions ORDER BY pinned DESC, last_active DESC" + ) + elif user_id is not None: rows = await conn.fetch( "SELECT id, profile_id, user_id, pinned, created_at, last_active, context_token_count, name, planning_logs, next_sequence, archive_threshold, session_metadata " "FROM sessions WHERE user_id = $1 ORDER BY pinned DESC, last_active DESC", @@ -768,7 +779,7 @@ else: rows = await conn.fetch( "SELECT id, profile_id, user_id, pinned, created_at, last_active, context_token_count, name, planning_logs, next_sequence, archive_threshold, session_metadata " - "FROM sessions ORDER BY pinned DESC, last_active DESC" + "FROM sessions WHERE user_id IS NULL ORDER BY pinned DESC, last_active DESC" ) return await _build_sessions(conn, rows) @@ -793,7 +804,10 @@ params.append(value) return f"${param_idx}" - if not is_admin and user_id is not None: + if not is_admin and user_id is None: + # Local mode: the anonymous local user owns NULL sessions only. + conditions.append("user_id IS NULL") + elif not is_admin: conditions.append(f"user_id = {add_param(user_id)}") if profile_id: conditions.append(f"profile_id = {add_param(profile_id)}") @@ -833,7 +847,10 @@ params.append(value) return f"${param_idx}" - if not is_admin and user_id is not None: + if not is_admin and user_id is None: + # Local mode: the anonymous local user owns NULL sessions only. + conditions.append("user_id IS NULL") + elif not is_admin: conditions.append(f"user_id = {add_param(user_id)}") if search: like = f"%{search}%" @@ -868,7 +885,10 @@ params.append(value) return f"${param_idx}" - if not is_admin and user_id is not None: + if not is_admin and user_id is None: + # Local mode: the anonymous local user owns NULL sessions only. + conditions.append("user_id IS NULL") + elif not is_admin: conditions.append(f"user_id = {add_param(user_id)}") if search: like = f"%{search}%" diff --git a/navi/core/session.py b/navi/core/session.py index 0b35edc..f09a71f 100644 --- a/navi/core/session.py +++ b/navi/core/session.py @@ -109,8 +109,13 @@ async def list_all(self, user_id: str | None = None, is_admin: bool = False) -> list[Session]: sessions = self._sessions.values() - if not is_admin and user_id is not None: + if is_admin: + pass + elif user_id is not None: sessions = [s for s in sessions if s.user_id == user_id] + else: + # user_id=None non-admin means the anonymous local user: NULL-owner rows only. + sessions = [s for s in sessions if s.user_id is None] return sorted( sessions, key=lambda s: (s.pinned, s.last_active), @@ -155,8 +160,13 @@ sort_order: str = "desc", ) -> list[Session]: sessions = list(self._sessions.values()) - if not is_admin and user_id is not None: + if is_admin: + pass + elif user_id is not None: sessions = [s for s in sessions if s.user_id == user_id] + else: + # user_id=None non-admin: anonymous local user — NULL-owner rows only. + sessions = [s for s in sessions if s.user_id is None] if search: q = search.lower() sessions = [ diff --git a/tests/integration/test_auth_disabled.py b/tests/integration/test_auth_disabled.py index 8ab45ea..0a0f0bb 100644 --- a/tests/integration/test_auth_disabled.py +++ b/tests/integration/test_auth_disabled.py @@ -110,7 +110,8 @@ session = await store.get(data["session_id"]) assert session is not None - assert session.user_id == "anonymous" + # Local mode: sessions belong to the NULL owner, not to a user alias. + assert session.user_id is None @pytest.mark.anyio async def test_list_sessions_without_auth(self, no_auth_client): @@ -124,6 +125,35 @@ assert any(s["session_id"] == session_id for s in sessions) @pytest.mark.anyio + async def test_local_mode_never_lists_other_users_sessions(self, no_auth_client): + """Local mode is a separate user with user_id NULL — chats owned by + real users must not appear in the sidebar.""" + client, store = no_auth_client + # Another user's session, as on a shared database. + foreign = await store.create("secretary", user_id="1") + # Our own (NULL-owner) session. + resp = client.post("/sessions", json={"profile_id": "secretary"}) + own_id = resp.json()["session_id"] + + response = client.get("/sessions") + assert response.status_code == 200 + ids = [s["session_id"] for s in response.json()] + assert own_id in ids + assert foreign.id not in ids + + # Direct access by id is denied too (check_session_access). + foreign_resp = client.get(f"/sessions/{foreign.id}") + assert foreign_resp.status_code == 403 + + @pytest.mark.anyio + async def test_local_mode_can_open_legacy_null_owner_session(self, no_auth_client): + client, store = no_auth_client + legacy = await store.create("secretary", user_id=None) + + response = client.get(f"/sessions/{legacy.id}") + assert response.status_code == 200 + + @pytest.mark.anyio async def test_create_session_uses_default_profile(self, monkeypatch, no_auth_client): """When navi_default_profile_id is set, POST /sessions with no body uses it.""" import navi.config as _config diff --git a/tests/unit/api/test_websocket.py b/tests/unit/api/test_websocket.py index 4851791..a5fcbdb 100644 --- a/tests/unit/api/test_websocket.py +++ b/tests/unit/api/test_websocket.py @@ -29,7 +29,9 @@ @pytest.fixture def mock_session(): session = MagicMock() - session.user_id = "test-user-id" + # user_id=None = legacy/local-owned session (deps.settings may carry + # env-derived NAVI_AUTH_ENABLED=false in unit runs; NULL owner passes). + session.user_id = None return session diff --git a/tests/unit/auth/test_deps.py b/tests/unit/auth/test_deps.py index 892aa41..a8229f6 100644 --- a/tests/unit/auth/test_deps.py +++ b/tests/unit/auth/test_deps.py @@ -428,12 +428,18 @@ @pytest.mark.asyncio -async def test_check_session_access_allows_everything_when_disabled(_auth_env): +async def test_check_session_access_disabled_allows_null_owner_only(_auth_env): + """Local mode: the anonymous local user owns only NULL-owner sessions — + other users' chats must not be readable by id.""" _auth_env["settings"].navi_auth_enabled = False - user = User(id="u1", email="u@test.com", role="user") - session = FakeChatSession(user_id="u2") - # should not raise even for non-owner non-admin - check_session_access(session, user) + local = User(id="anonymous", email="anonymous@navi.local", role="admin") + # Legacy/local sessions (user_id=None) are visible. + check_session_access(FakeChatSession(user_id=None), local) + # A real user's session is not. + from fastapi import HTTPException + with pytest.raises(HTTPException) as exc_info: + check_session_access(FakeChatSession(user_id="u2"), local) + assert exc_info.value.status_code == 403 @pytest.mark.asyncio diff --git a/tests/unit/core/test_pg_session_store.py b/tests/unit/core/test_pg_session_store.py index ceb520b..a71ae61 100644 --- a/tests/unit/core/test_pg_session_store.py +++ b/tests/unit/core/test_pg_session_store.py @@ -374,3 +374,73 @@ # synthetic placeholder continues the rank walk assert msgs[3].metadata["display_index"] == 8 assert msgs[3].content == "[Interrupted before result]" + + +# ── Listing scope: local mode (user_id=None) is a separate owner ────────────── + + +@pytest.mark.asyncio +async def test_list_all_null_owner_scopes_to_null_rows(): + store, conn = _make_store() + conn.enqueue([]) + + await store.list_all(user_id=None, is_admin=False) + + sql = conn.calls[0][1] + assert "user_id IS NULL" in sql + + +@pytest.mark.asyncio +async def test_list_all_named_owner_filters_by_user(): + store, conn = _make_store() + conn.enqueue([]) + + await store.list_all(user_id="u1", is_admin=False) + + sql = conn.calls[0][1] + assert "user_id = $1" in sql + assert "IS NULL" not in sql + + +@pytest.mark.asyncio +async def test_list_all_admin_sees_everything(): + store, conn = _make_store() + conn.enqueue([]) + + await store.list_all(user_id="u1", is_admin=True) + + sql = conn.calls[0][1] + assert "WHERE" not in sql # no user_id filter in the SELECT list or WHERE + + +@pytest.mark.asyncio +async def test_list_page_null_owner_scopes_to_null_rows(): + store, conn = _make_store() + conn.enqueue([]) + + await store.list_page(limit=10, offset=0, user_id=None, is_admin=False) + + sql = conn.calls[0][1] + assert "user_id IS NULL" in sql + + +@pytest.mark.asyncio +async def test_search_list_null_owner_scopes_to_null_rows(): + store, conn = _make_store() + conn.enqueue([]) + + await store.search_list(limit=10, offset=0, user_id=None, is_admin=False, search="ip") + + fetches = [c for c in conn.calls if c[0] == "fetch"] + assert fetches and "user_id IS NULL" in fetches[0][1] + + +@pytest.mark.asyncio +async def test_count_all_null_owner_scopes_to_null_rows(): + store, conn = _make_store() + conn.enqueue(0) + + await store.count_all(user_id=None, is_admin=False) + + rows = [c for c in conn.calls if c[0] == "fetchrow"] + assert rows and "SELECT COUNT(*) FROM sessions WHERE user_id IS NULL" in rows[0][1]