diff --git a/deeptutor/api/main.py b/deeptutor/api/main.py index 7618df9d..cf4a764d 100644 --- a/deeptutor/api/main.py +++ b/deeptutor/api/main.py @@ -375,9 +375,16 @@ async def demo_rate_limit(request, call_next): # All other routers require a valid session when AUTH_ENABLED=true. # require_auth is a no-op when AUTH_ENABLED=false, so this is safe for local use. -from deeptutor.api.routers.auth import require_admin, require_auth # noqa: E402 +from deeptutor.api.routers.auth import ( # noqa: E402 + bind_demo_visitor, + require_admin, + require_auth, +) -_auth = [Depends(require_auth)] +# require_auth installs the current user; bind_demo_visitor partitions the demo +# session store per visitor (no-op unless DEMO_MODE is on). Both run on every +# authenticated router below. +_auth = [Depends(require_auth), Depends(bind_demo_visitor)] # Partner data is anchored at the admin workspace (data/partners) and shared # process-wide, so management is admin-gated in multi-user deployments # (single-user local runs are implicitly admin — no behaviour change there). diff --git a/deeptutor/api/routers/auth.py b/deeptutor/api/routers/auth.py index 6f3136e4..2e7191c5 100644 --- a/deeptutor/api/routers/auth.py +++ b/deeptutor/api/routers/auth.py @@ -263,6 +263,27 @@ async def require_auth( return payload +async def bind_demo_visitor( + x_demo_visitor: str | None = Header(default=None, alias="X-Demo-Visitor"), +) -> None: + """Bind the per-visitor demo id from the ``X-Demo-Visitor`` header. + + A no-op unless ``DEMO_MODE`` is on, so private deployments are unaffected. + In demo mode this partitions the shared in-memory session store per visitor + (see ``get_sqlite_session_store``) so concurrent visitors don't see each + other's chat history. + + ``async def`` for the same reason as ``require_auth``: the ContextVar set + must run on the event loop to stay visible to the endpoint. HTTP callers + ignore the reset token — the request task's context is discarded when it + ends. Registered globally via the ``_auth`` dependency list in ``main``. + """ + from deeptutor.services.demo import is_demo_mode, set_visitor_id + + if is_demo_mode(): + set_visitor_id(x_demo_visitor) + + class _WsAuthFailed: """Sentinel: ws_require_auth failed and closed the WebSocket.""" @@ -291,6 +312,16 @@ async def ws_require_auth(ws: WebSocket) -> _CtxToken | _WsAuthFailed: finally: reset_current_user(user_token) """ + # Demo per-visitor isolation: read the visitor id from the WS query param + # (cookies aren't reliably set in the demo's proxy setup, so the client + # sends it explicitly). Set before the AUTH_ENABLED branch so it applies + # whether or not auth is on. Not reset here — the connection's task context + # is discarded when the handler task ends, same as the HTTP path. + from deeptutor.services.demo import is_demo_mode, set_visitor_id + + if is_demo_mode(): + set_visitor_id(ws.query_params.get("visitor")) + if not AUTH_ENABLED: return _install_current_user(None) diff --git a/deeptutor/services/demo/__init__.py b/deeptutor/services/demo/__init__.py index 6a55efc0..558911f6 100644 --- a/deeptutor/services/demo/__init__.py +++ b/deeptutor/services/demo/__init__.py @@ -7,11 +7,21 @@ get_demo_limiter, is_demo_mode, ) +from .visitor import ( + get_visitor_id, + reset_visitor_id, + sanitize_visitor_id, + set_visitor_id, +) __all__ = [ "DemoRateLimiter", "RateDecision", "client_ip", "get_demo_limiter", + "get_visitor_id", "is_demo_mode", + "reset_visitor_id", + "sanitize_visitor_id", + "set_visitor_id", ] diff --git a/deeptutor/services/demo/visitor.py b/deeptutor/services/demo/visitor.py new file mode 100644 index 00000000..ae394fb4 --- /dev/null +++ b/deeptutor/services/demo/visitor.py @@ -0,0 +1,64 @@ +"""Per-visitor identity for the public demo. + +Why this exists +--------------- +DeepTutor is single-user by design: sessions carry no owner column, and every +anonymous demo visitor resolves to the same ``local_admin_user``. In demo mode +the session store is one shared in-memory DB for the whole process, so a naive +``list_sessions`` surfaces *every* visitor's chats to *everyone* — reproduced by +opening the demo in a second browser or incognito window. + +This module carries a per-visitor id (minted client-side and sent as the +``X-Demo-Visitor`` header on HTTP and the ``visitor`` query param on the chat +WebSocket) through a request-local :class:`~contextvars.ContextVar`, mirroring +``multi_user.context``'s ``_current_user``. ``get_sqlite_session_store`` reads +it to hand each visitor a physically separate in-memory store, so history stays +both isolated between visitors and ephemeral (in-memory, gone on restart). + +It is **isolation, not authentication**: the id is an opaque client-chosen +token, not a security boundary. It only partitions the demo's ephemeral store. +Non-demo deployments never set it, so the value is inert outside the demo. +""" + +from __future__ import annotations + +from contextvars import ContextVar, Token +import re + +# Request-local visitor id. Empty string means "no visitor id supplied" — such +# callers share a single default bucket (see ``get_sqlite_session_store``). +_visitor_id: ContextVar[str] = ContextVar("deeptutor_demo_visitor_id", default="") + +# Client-chosen ids are opaque, but they become dict keys for live in-memory +# stores, so bound both the character set and the length to keep a hostile or +# buggy client from minting pathological keys. +_MAX_LEN = 64 +_ALLOWED = re.compile(r"[^A-Za-z0-9_-]") + + +def sanitize_visitor_id(raw: str | None) -> str: + """Normalise an untrusted visitor id to ``[A-Za-z0-9_-]{1,64}`` or ``""``.""" + if not raw: + return "" + cleaned = _ALLOWED.sub("", raw)[:_MAX_LEN] + return cleaned + + +def set_visitor_id(value: str | None) -> Token[str]: + """Bind the current visitor id (sanitized). Returns a reset token. + + Follows the same lifetime rules as ``multi_user.context.set_current_user``: + HTTP callers may ignore the token (the request task's context is discarded + when it ends), while long-lived WebSocket handlers keep it and call + :func:`reset_visitor_id` in their ``finally`` block. + """ + return _visitor_id.set(sanitize_visitor_id(value)) + + +def reset_visitor_id(token: Token[str]) -> None: + _visitor_id.reset(token) + + +def get_visitor_id() -> str: + """The current visitor id, or ``""`` when none was supplied.""" + return _visitor_id.get() diff --git a/deeptutor/services/session/sqlite_store.py b/deeptutor/services/session/sqlite_store.py index a2258465..ff06a4b0 100644 --- a/deeptutor/services/session/sqlite_store.py +++ b/deeptutor/services/session/sqlite_store.py @@ -106,26 +106,31 @@ def __init__(self, db_path: Path | None = None, *, in_memory: bool = False) -> N # destroyed the instant no connection is open. The keep-alive # connection below stays open for the store's lifetime to pin it. # A unique name keeps two in-memory stores from sharing one cache. - self.db_path = f"file:deeptutor-mem-{uuid.uuid4().hex}?mode=memory&cache=shared" + # ``db_path`` is a ``file:`` URI here and a filesystem ``Path`` on + # the on-disk branch below; both are accepted by ``sqlite3.connect``. + self.db_path: str | Path = ( + f"file:deeptutor-mem-{uuid.uuid4().hex}?mode=memory&cache=shared" + ) self._keepalive = sqlite3.connect(self.db_path, uri=True) self._lock = asyncio.Lock() self._initialize() return path_service = get_path_service() - self.db_path = db_path or path_service.get_chat_history_db() - self.db_path.parent.mkdir(parents=True, exist_ok=True) - self._migrate_legacy_db(path_service) + disk_path = db_path or path_service.get_chat_history_db() + disk_path.parent.mkdir(parents=True, exist_ok=True) + self.db_path = disk_path + self._migrate_legacy_db(path_service, disk_path) self._lock = asyncio.Lock() self._initialize() - def _migrate_legacy_db(self, path_service) -> None: + def _migrate_legacy_db(self, path_service, db_path: Path) -> None: """Move the legacy ``data/chat_history.db`` into ``data/user/`` once.""" legacy_path = path_service.project_root / "data" / "chat_history.db" - if self.db_path.exists() or not legacy_path.exists() or legacy_path == self.db_path: + if db_path.exists() or not legacy_path.exists() or legacy_path == db_path: return try: - os.replace(legacy_path, self.db_path) + os.replace(legacy_path, db_path) except OSError: # Fall back to leaving the legacy DB in place if an OS-level move # is not possible; the new DB path will be initialized empty. @@ -1847,22 +1852,30 @@ async def get_entry_categories(self, entry_id: int) -> list[dict[str, Any]]: _instances: dict[str, SQLiteSessionStore] = {} -# Stable memoisation key for the demo-mode ephemeral store. Distinct from any -# real on-disk path so demo and non-demo stores never collide, and fixed so a -# process reuses one in-memory DB (each ``SQLiteSessionStore(in_memory=True)`` -# otherwise mints a fresh, isolated shared-cache namespace). +# Memoisation-key prefix for demo-mode ephemeral stores. Distinct from any real +# on-disk path so demo and non-demo stores never collide. The current visitor +# id is appended (see ``get_sqlite_session_store``) so each visitor reuses one +# in-memory DB across requests while staying isolated from other visitors; the +# bare prefix is the shared bucket for callers with no visitor id. _DEMO_INSTANCE_KEY = "__demo_in_memory__" def get_sqlite_session_store() -> SQLiteSessionStore: # Demo mode: chat history must stay ephemeral (never written to disk), so - # serve a shared in-memory store instead of the on-disk one. - from deeptutor.services.demo import get_demo_limiter + # serve an in-memory store instead of the on-disk one. It is keyed per + # visitor (a client-supplied id in a ContextVar) so concurrent demo + # visitors never see each other's history; a visitor with no id shares one + # default bucket. Each in-memory store mints a unique ``db_path`` (see + # ``SQLiteSessionStore.__init__``), so ``get_turn_runtime_manager`` — which + # caches by ``db_path`` — stays isolated per visitor automatically. + from deeptutor.services.demo import get_demo_limiter, get_visitor_id if get_demo_limiter().enabled: - if _DEMO_INSTANCE_KEY not in _instances: - _instances[_DEMO_INSTANCE_KEY] = SQLiteSessionStore(in_memory=True) - return _instances[_DEMO_INSTANCE_KEY] + visitor = get_visitor_id() + key = f"{_DEMO_INSTANCE_KEY}{visitor}" if visitor else _DEMO_INSTANCE_KEY + if key not in _instances: + _instances[key] = SQLiteSessionStore(in_memory=True) + return _instances[key] db_path = get_path_service().get_chat_history_db().resolve() key = str(db_path) diff --git a/tests/api/test_demo_visitor_isolation.py b/tests/api/test_demo_visitor_isolation.py new file mode 100644 index 00000000..03ff070c --- /dev/null +++ b/tests/api/test_demo_visitor_isolation.py @@ -0,0 +1,96 @@ +"""Integration test: the demo visitor id survives the request lifecycle. + +The store-level isolation is covered by +``tests/services/demo/test_demo_visitor_isolation.py``. This test pins the part +that unit tests can't: that the real ``bind_demo_visitor`` dependency actually +sets the visitor ContextVar in a way the endpoint sees. That is exactly the +regression class behind issue #481 (a sync dependency's ContextVar set runs in a +worker thread and is discarded), so we exercise it through the ASGI stack with +``TestClient``, mirroring the minimal-app style of ``test_demo_rate_limit.py``. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +pytest.importorskip("fastapi") + +from fastapi import Depends, FastAPI +from fastapi.testclient import TestClient + +from deeptutor.api.routers.auth import bind_demo_visitor +import deeptutor.services.demo.rate_limiter as rate_limiter +from deeptutor.services.path_service import PathService +from deeptutor.services.session import sqlite_store +from deeptutor.services.session.sqlite_store import get_sqlite_session_store + + +@pytest.fixture +def demo_client(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): + monkeypatch.setenv("DEMO_MODE", "true") + monkeypatch.setattr(rate_limiter, "_limiter", None) + monkeypatch.setattr(sqlite_store, "_instances", {}) + + service = PathService.get_instance() + saved = (service._project_root, service._user_data_dir) + service._project_root = tmp_path.resolve() + service._user_data_dir = (tmp_path / "data" / "user").resolve() + + app = FastAPI(dependencies=[Depends(bind_demo_visitor)]) + + @app.post("/sessions") + async def create(title: str): + await get_sqlite_session_store().create_session(title=title) + return {"ok": True} + + @app.get("/sessions") + async def listing(): + rows = await get_sqlite_session_store().list_sessions() + return {"titles": [r["title"] for r in rows]} + + try: + yield TestClient(app) + finally: + (service._project_root, service._user_data_dir) = saved + + +def _h(visitor: str) -> dict[str, str]: + return {"X-Demo-Visitor": visitor} + + +def test_visitors_do_not_see_each_others_history(demo_client): + demo_client.post("/sessions", params={"title": "alice chat"}, headers=_h("alice")) + + # Bob (a second browser / incognito) sees none of Alice's history. + assert demo_client.get("/sessions", headers=_h("bob")).json()["titles"] == [] + # Alice still sees her own. + assert demo_client.get("/sessions", headers=_h("alice")).json()["titles"] == ["alice chat"] + + +def test_missing_header_does_not_inherit_previous_visitor(demo_client): + """A request with no visitor id must not leak into another's bucket.""" + demo_client.post("/sessions", params={"title": "alice chat"}, headers=_h("alice")) + + # No header → default bucket, which is empty (not Alice's). + assert demo_client.get("/sessions").json()["titles"] == [] + + +@pytest.mark.asyncio +async def test_ws_require_auth_binds_visitor_from_query_param(monkeypatch): + """The chat WebSocket carries the visitor id as a query param (WS upgrades + can't set custom headers), and ``ws_require_auth`` must bind it.""" + monkeypatch.setenv("DEMO_MODE", "true") + monkeypatch.setattr(rate_limiter, "_limiter", None) + + from deeptutor.api.routers.auth import ws_require_auth + from deeptutor.services.demo import get_visitor_id + + class _FakeWS: + # AUTH is disabled by default, so ws_require_auth only touches query_params. + query_params = {"visitor": "alice"} + cookies: dict[str, str] = {} + + await ws_require_auth(_FakeWS()) + assert get_visitor_id() == "alice" diff --git a/tests/services/demo/test_demo_visitor_isolation.py b/tests/services/demo/test_demo_visitor_isolation.py new file mode 100644 index 00000000..57ba8f30 --- /dev/null +++ b/tests/services/demo/test_demo_visitor_isolation.py @@ -0,0 +1,89 @@ +"""Demo mode isolates chat history per browser visitor. + +The demo serves a single process to every anonymous visitor. Before this fix +``get_sqlite_session_store`` returned one shared in-memory store for the whole +process, so ``list_sessions`` surfaced *every* visitor's chats to *everyone* +(reproduced by opening the demo in a second browser/incognito window). The fix +keys the demo store by a per-visitor id carried in a ContextVar, so each visitor +gets a physically separate in-memory DB. These tests pin that isolation and +assert the non-demo path still ignores the visitor id entirely. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from deeptutor.services.demo import reset_visitor_id, set_visitor_id +import deeptutor.services.demo.rate_limiter as rate_limiter +from deeptutor.services.path_service import PathService +from deeptutor.services.session import sqlite_store +from deeptutor.services.session.sqlite_store import get_sqlite_session_store + + +@pytest.fixture +def demo_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): + """Reset the memoised limiter + store instances and point paths at a tmp dir.""" + monkeypatch.setattr(rate_limiter, "_limiter", None) + monkeypatch.setattr(sqlite_store, "_instances", {}) + + service = PathService.get_instance() + saved = (service._project_root, service._user_data_dir) + service._project_root = tmp_path.resolve() + service._user_data_dir = (tmp_path / "data" / "user").resolve() + try: + yield service + finally: + (service._project_root, service._user_data_dir) = saved + + +@pytest.mark.asyncio +async def test_history_isolated_between_visitors(demo_env, monkeypatch): + monkeypatch.setenv("DEMO_MODE", "true") + + # Visitor A starts a chat. + token_a = set_visitor_id("visitor-a") + store_a = get_sqlite_session_store() + await store_a.create_session(title="A's private chat") + reset_visitor_id(token_a) + + # Visitor B (e.g. an incognito window) must see none of A's history. + token_b = set_visitor_id("visitor-b") + store_b = get_sqlite_session_store() + assert store_b is not store_a + assert await store_b.list_sessions() == [] + reset_visitor_id(token_b) + + # Visitor A comes back to the same store and still sees their own chat. + token_a2 = set_visitor_id("visitor-a") + store_a2 = get_sqlite_session_store() + assert store_a2 is store_a + assert len(await store_a2.list_sessions()) == 1 + reset_visitor_id(token_a2) + + +def test_same_visitor_reuses_one_store(demo_env, monkeypatch): + monkeypatch.setenv("DEMO_MODE", "true") + + token = set_visitor_id("visitor-a") + try: + assert get_sqlite_session_store() is get_sqlite_session_store() + finally: + reset_visitor_id(token) + + +def test_non_demo_ignores_visitor_id(demo_env, monkeypatch): + monkeypatch.delenv("DEMO_MODE", raising=False) + + token_a = set_visitor_id("visitor-a") + store_a = get_sqlite_session_store() + reset_visitor_id(token_a) + + token_b = set_visitor_id("visitor-b") + store_b = get_sqlite_session_store() + reset_visitor_id(token_b) + + # Off demo, the store is the single on-disk store regardless of visitor. + assert store_a is store_b + assert store_a.in_memory is False diff --git a/web/lib/api.ts b/web/lib/api.ts index 0ce83b45..cad67f52 100644 --- a/web/lib/api.ts +++ b/web/lib/api.ts @@ -1,3 +1,5 @@ +import { getDemoVisitorId } from "./demo-visitor"; + // API configuration and utility functions. // // The frontend bundle is now URL-agnostic: the browser issues requests against @@ -79,7 +81,19 @@ export async function apiFetch( init?: RequestInit & { skipAuthRedirect?: boolean }, ): Promise { const { skipAuthRedirect, ...fetchInit } = init ?? {}; - const res = await fetch(input, { credentials: "include", ...fetchInit }); + + // Tag every request with this browser's demo visitor id so the backend can + // keep demo chat history private per visitor. Ignored by non-demo backends. + // Preserves any headers the caller already set. + const visitorId = getDemoVisitorId(); + const headers = new Headers(fetchInit.headers); + if (visitorId) headers.set("X-Demo-Visitor", visitorId); + + const res = await fetch(input, { + credentials: "include", + ...fetchInit, + headers, + }); if ( res.status === 401 && diff --git a/web/lib/demo-visitor.ts b/web/lib/demo-visitor.ts new file mode 100644 index 00000000..888eaed5 --- /dev/null +++ b/web/lib/demo-visitor.ts @@ -0,0 +1,38 @@ +// Per-visitor id for the public demo. +// +// DeepTutor is single-user by design, so in DEMO_MODE the backend serves one +// shared in-memory session store to every anonymous visitor. To keep each +// visitor's chat history private, the browser mints a random id once, persists +// it in localStorage, and sends it on every request (the `X-Demo-Visitor` +// header via `apiFetch`, and the `visitor` query param on the chat WebSocket). +// The backend keys its ephemeral store by this id. +// +// This is isolation, not authentication: the id is opaque and only partitions +// the demo's ephemeral store. An incognito window has its own localStorage, so +// it naturally gets a distinct id (and distinct history). Non-demo backends +// ignore the id entirely, so sending it unconditionally is harmless. + +const STORAGE_KEY = "deeptutor-demo-visitor-id"; + +/** + * Return this browser's demo visitor id, creating and persisting one on first + * use. Returns "" during SSR or when storage is unavailable (private-mode + * quotas, disabled storage) — the backend then falls back to a shared bucket, + * which is no worse than the pre-isolation behaviour. + */ +export function getDemoVisitorId(): string { + if (typeof window === "undefined") return ""; + try { + let id = window.localStorage.getItem(STORAGE_KEY); + if (!id) { + id = + typeof crypto !== "undefined" && "randomUUID" in crypto + ? crypto.randomUUID() + : `v-${Math.random().toString(36).slice(2)}${Date.now().toString(36)}`; + window.localStorage.setItem(STORAGE_KEY, id); + } + return id; + } catch { + return ""; + } +} diff --git a/web/lib/unified-ws.ts b/web/lib/unified-ws.ts index 681a3b35..16bf9d0f 100644 --- a/web/lib/unified-ws.ts +++ b/web/lib/unified-ws.ts @@ -11,6 +11,7 @@ */ import { wsUrl } from "./api"; +import { getDemoVisitorId } from "./demo-visitor"; // ---- StreamEvent types (mirror Python StreamEventType) ---- @@ -184,7 +185,14 @@ export class UnifiedWSClient { if (this.ws && this.ws.readyState <= WebSocket.OPEN) return; this.intentionalClose = false; - const url = wsUrl("/api/v1/ws"); + // Carry the demo visitor id as a query param (WS upgrades can't set + // custom headers). Ignored by non-demo backends; keeps demo chat history + // isolated per visitor, matching the `X-Demo-Visitor` header on HTTP. + const visitorId = getDemoVisitorId(); + const path = visitorId + ? `/api/v1/ws?visitor=${encodeURIComponent(visitorId)}` + : "/api/v1/ws"; + const url = wsUrl(path); this.ws = new WebSocket(url); this.ws.onopen = () => {