From e2fd8dc7a8965de0f181717ac65c24c4c79eeb2a Mon Sep 17 00:00:00 2001 From: Mgeeeek Date: Wed, 13 May 2026 21:34:05 +0530 Subject: [PATCH] chore: drop internal planning docs from public branch --- .../2026-05-13-rest-api-authz-fix-plan.md | 1529 ----------------- .../2026-05-13-rest-api-authz-fix-design.md | 274 --- 2 files changed, 1803 deletions(-) delete mode 100644 docs/superpowers/plans/2026-05-13-rest-api-authz-fix-plan.md delete mode 100644 docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md diff --git a/docs/superpowers/plans/2026-05-13-rest-api-authz-fix-plan.md b/docs/superpowers/plans/2026-05-13-rest-api-authz-fix-plan.md deleted file mode 100644 index e8df74abb..000000000 --- a/docs/superpowers/plans/2026-05-13-rest-api-authz-fix-plan.md +++ /dev/null @@ -1,1529 +0,0 @@ -# REST API Authorization Fix Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Admin-gate six routes in mem0's self-hosted OSS REST server, add an inside-route guard on `GET /memories`, surgically fix the docs/code mismatch in `docs/open-source/features/rest-api.mdx`, and stand up a minimal `server/tests/` pytest scaffold — without regressing any of the four supported deployment modes (single-admin JWT, single-admin per-user key, legacy `ADMIN_API_KEY` env, `AUTH_DISABLED=true`). - -**Architecture:** One new helper (`_ensure_admin`) and one FastAPI dependency (`require_admin`) in `server/auth.py`. Swap `Depends(verify_auth)` → `Depends(require_admin)` on six routes across three files. Add one inside-route guard on the no-filter branch of `GET /memories`. Surgical edits to four lines in the docs. Pytest scaffold uses SQLite in-memory + `MagicMock` for the memory backend — no Postgres, no pgvector, no Docker required for tests. No DB migration, no SDK changes, no payload schema changes. - -**Tech Stack:** Python 3.11, FastAPI 0.115, SQLAlchemy 2.x (DeclarativeBase), SQLite (test-only), pytest, httpx (FastAPI TestClient), unittest.mock. Reuse existing project conventions in `server/`. - -**Spec:** `docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md` (commit `8f285c75`). - ---- - -## File Map - -**Create:** - -| Path | Responsibility | -|---|---| -| `server/tests/__init__.py` | empty package marker | -| `server/tests/conftest.py` | pytest fixtures: SQLite engine, TestClient, admin/non-admin User, JWT helpers, env-var monkeypatches | -| `server/tests/test_auth_helpers.py` | unit tests for `_ensure_admin` (sync) and integration tests for `require_admin` via a throwaway probe route | -| `server/tests/test_admin_gating.py` | integration tests for the six admin-gated routes + inside-route guard | -| `server/tests/test_unchanged_routes.py` | regression tests for memory CRUD and `/api-keys/*` | -| `server/requirements-dev.txt` | pytest, pytest-asyncio, httpx | -| `.github/workflows/server-ci.yml` | pytest CI on PRs touching `server/` | - -**Modify:** - -| Path | Change | -|---|---| -| `server/auth.py` | append `_ensure_admin` + `require_admin` | -| `server/main.py` | swap dep on `GET /configure`, `POST /configure`, `POST /reset`; refactor `GET /memories` for inside-route guard | -| `server/routers/entities.py` | swap dep on `GET /entities` and `DELETE /entities/{type}/{id}`; import `require_admin` | -| `server/routers/requests.py` | swap dep on `GET /requests`; drop unused `user` parameter; trim imports | -| `docs/open-source/features/rest-api.mdx` | surgical line edits (lines 134, 185, endpoint tables, 267) | - ---- - -## Task 1: Pytest scaffold for `server/` - -**Files:** -- Create: `server/tests/__init__.py` -- Create: `server/requirements-dev.txt` -- Create: `server/tests/conftest.py` -- Create: `server/tests/test_smoke.py` (deleted at end of task — used only to prove scaffold works) - -- [ ] **Step 1: Create the test package marker** - -```bash -mkdir -p server/tests -``` - -Write `server/tests/__init__.py`: - -```python -``` - -(Empty file. Marks `server/tests` as a package.) - -- [ ] **Step 2: Create the dev-deps file** - -Write `server/requirements-dev.txt`: - -```text --r requirements.txt - -pytest>=8.0,<9.0 -pytest-asyncio>=0.23,<1.0 -httpx>=0.27,<1.0 -``` - -- [ ] **Step 3: Install dev deps locally** - -Run: `pip install -r server/requirements-dev.txt` -Expected: clean install, no errors. - -- [ ] **Step 4: Create the conftest with shared fixtures** - -Write `server/tests/conftest.py`: - -```python -"""Shared pytest fixtures for server/ tests. - -Strategy: -- SQLite in-memory engine for the auth DB (Users, APIKey, RefreshTokenJti, etc). -- get_memory_instance() is patched to return a MagicMock so routes that touch - the memory backend don't need pgvector. -- The FastAPI app is imported lazily after env vars are set so module-level - constants in server.auth (JWT_SECRET, AUTH_DISABLED, ADMIN_API_KEY) bind - to the test values, not whatever the host shell has. -""" - -from __future__ import annotations - -import os -import sys -import uuid -from pathlib import Path -from unittest.mock import MagicMock - -import pytest -from fastapi.testclient import TestClient -from sqlalchemy import create_engine -from sqlalchemy.orm import sessionmaker - -# Make server/ importable as a top-level package (matches uvicorn's CWD). -SERVER_DIR = Path(__file__).resolve().parent.parent -sys.path.insert(0, str(SERVER_DIR)) - -# Required for JWT issuance during tests. -os.environ.setdefault("JWT_SECRET", "test-secret-do-not-use-in-prod-" + "x" * 32) -os.environ.setdefault("OPENAI_API_KEY", "test-key") -os.environ.setdefault("POSTGRES_HOST", "localhost") # silence db url builder - - -@pytest.fixture -def test_engine(): - """Fresh in-memory SQLite engine per test (no cross-test pollution).""" - engine = create_engine( - "sqlite:///:memory:", - connect_args={"check_same_thread": False}, - ) - # Import Base AFTER env vars are set. - from db import Base # noqa: E402 - - # Importing models registers them on Base.metadata. - import models # noqa: F401, E402 - - Base.metadata.create_all(engine) - return engine - - -@pytest.fixture -def test_session_factory(test_engine): - return sessionmaker(bind=test_engine, autoflush=False, expire_on_commit=False) - - -@pytest.fixture -def mock_memory(): - """A MagicMock standing in for the global Memory instance.""" - mock = MagicMock() - mock.get_all.return_value = {"results": []} - mock.search.return_value = {"results": []} - mock.add.return_value = {"results": [], "events": []} - mock.get.return_value = {"id": "memory-id", "memory": "stub"} - mock.history.return_value = [] - mock.delete.return_value = None - mock.delete_all.return_value = None - mock.reset.return_value = None - mock.vector_store.list.return_value = [[]] - return mock - - -@pytest.fixture -def client(test_session_factory, mock_memory, monkeypatch): - """FastAPI TestClient with overridden DB session and mocked memory instance.""" - from db import get_db # noqa: E402 - import server_state # noqa: E402 - - monkeypatch.setattr(server_state, "get_memory_instance", lambda: mock_memory) - - from main import app # noqa: E402 - - # Also patch the binding inside main.py since it imported get_memory_instance by name. - monkeypatch.setattr("main.get_memory_instance", lambda: mock_memory) - - def _override_get_db(): - db = test_session_factory() - try: - yield db - finally: - db.close() - - app.dependency_overrides[get_db] = _override_get_db - try: - with TestClient(app) as c: - yield c - finally: - app.dependency_overrides.pop(get_db, None) - - -@pytest.fixture -def db_session(test_session_factory): - """A direct DB session for tests that need to insert User rows.""" - session = test_session_factory() - try: - yield session - finally: - session.close() - - -def _make_user(db_session, *, role: str, email: str | None = None): - from auth import hash_password - from models import User - - user = User( - id=uuid.uuid4(), - name=f"{role}-user", - email=email or f"{role}-{uuid.uuid4().hex[:8]}@example.com", - password_hash=hash_password("test-password-123"), - role=role, - ) - db_session.add(user) - db_session.commit() - db_session.refresh(user) - return user - - -@pytest.fixture -def admin_user(db_session): - return _make_user(db_session, role="admin") - - -@pytest.fixture -def member_user(db_session): - """Non-admin User. No public endpoint produces these today; we insert directly.""" - return _make_user(db_session, role="member") - - -@pytest.fixture -def admin_jwt(admin_user): - from auth import create_access_token - - return create_access_token(str(admin_user.id), admin_user.role) - - -@pytest.fixture -def member_jwt(member_user): - from auth import create_access_token - - return create_access_token(str(member_user.id), member_user.role) - - -@pytest.fixture -def auth_admin_header(admin_jwt): - return {"Authorization": f"Bearer {admin_jwt}"} - - -@pytest.fixture -def auth_member_header(member_jwt): - return {"Authorization": f"Bearer {member_jwt}"} - - -@pytest.fixture -def admin_api_key_env(monkeypatch): - """Activates the legacy ADMIN_API_KEY escape hatch.""" - import auth - - key = "admin-api-key-test-value-" + "y" * 16 - monkeypatch.setattr(auth, "ADMIN_API_KEY", key) - return {"X-API-Key": key} - - -@pytest.fixture -def auth_disabled_env(monkeypatch): - """Activates AUTH_DISABLED=true.""" - import auth - - monkeypatch.setattr(auth, "AUTH_DISABLED", True) - return {} # no headers needed -``` - -- [ ] **Step 5: Add a smoke test to prove the scaffold loads** - -Write `server/tests/test_smoke.py`: - -```python -def test_scaffold_imports(client): - """Asserts the TestClient fixture wires up cleanly.""" - response = client.get("/auth/setup-status") - assert response.status_code == 200 - assert response.json() == {"needsSetup": True} -``` - -- [ ] **Step 6: Run the smoke test** - -Run: `cd server && pytest tests/test_smoke.py -v` -Expected: `1 passed`. If imports fail, fix the `sys.path` insert in conftest.py and re-run. - -- [ ] **Step 7: Delete the smoke test (scaffold proven)** - -```bash -rm server/tests/test_smoke.py -``` - -- [ ] **Step 8: Commit** - -```bash -git add server/tests/__init__.py server/tests/conftest.py server/requirements-dev.txt -git commit -m "test(server): add pytest scaffold with SQLite + mocked memory backend" -``` - ---- - -## Task 2: `_ensure_admin` and `require_admin` (TDD) - -**Files:** -- Create: `server/tests/test_auth_helpers.py` -- Modify: `server/auth.py` (append at end of file) - -- [ ] **Step 1: Write the failing tests** - -Write `server/tests/test_auth_helpers.py`: - -```python -"""Unit tests for _ensure_admin and require_admin in server/auth.py.""" - -from __future__ import annotations - -from types import SimpleNamespace - -import pytest -from fastapi import HTTPException - - -def _fake_request(auth_type: str = "none"): - return SimpleNamespace(state=SimpleNamespace(auth_type=auth_type)) - - -def test_ensure_admin_passes_for_admin_user(admin_user): - from auth import _ensure_admin - - _ensure_admin(_fake_request("bearer"), admin_user) # must not raise - - -def test_ensure_admin_passes_for_admin_api_key_auth_type(): - from auth import _ensure_admin - - _ensure_admin(_fake_request("admin_api_key"), None) # must not raise - - -def test_ensure_admin_passes_for_auth_disabled(): - from auth import _ensure_admin - - _ensure_admin(_fake_request("disabled"), None) # must not raise - - -def test_ensure_admin_rejects_non_admin_user(member_user): - from auth import _ensure_admin - - with pytest.raises(HTTPException) as exc: - _ensure_admin(_fake_request("bearer"), member_user) - assert exc.value.status_code == 403 - assert "Admin role required" in exc.value.detail - - -def test_ensure_admin_rejects_no_user_with_no_legacy_path(): - from auth import _ensure_admin - - with pytest.raises(HTTPException) as exc: - _ensure_admin(_fake_request("none"), None) - assert exc.value.status_code == 403 - - -def test_ensure_admin_rejects_bearer_with_no_user(): - """Defense-in-depth: a 'bearer' auth_type without a User should not pass.""" - from auth import _ensure_admin - - with pytest.raises(HTTPException) as exc: - _ensure_admin(_fake_request("bearer"), None) - assert exc.value.status_code == 403 -``` - -- [ ] **Step 2: Run tests, confirm they fail** - -Run: `cd server && pytest tests/test_auth_helpers.py -v` -Expected: 6 errors (`ImportError: cannot import name '_ensure_admin' from 'auth'`). - -- [ ] **Step 3: Implement `_ensure_admin` and `require_admin`** - -Edit `server/auth.py`. Append at the end of the file (after the existing `require_auth` function): - -```python - - -def _ensure_admin(request: Request, user: User | None) -> None: - """Single source of truth for admin gating. - - Allows: legacy ADMIN_API_KEY env (X-API-Key header), AUTH_DISABLED=true, - or a registered User with role == 'admin'. Raises 403 otherwise. - - Pure function — reusable from FastAPI deps and inline route guards. - """ - auth_type = getattr(request.state, "auth_type", "none") - if auth_type in {"admin_api_key", "disabled"}: - return - if user is not None and user.role == "admin": - return - raise HTTPException(status_code=403, detail="Admin role required.") - - -async def require_admin( - request: Request, - user: User | None = Depends(verify_auth), -) -> None: - """FastAPI dependency. Admin-only gate; returns nothing.""" - _ensure_admin(request, user) -``` - -- [ ] **Step 4: Run tests, confirm they pass** - -Run: `cd server && pytest tests/test_auth_helpers.py -v` -Expected: `6 passed`. - -- [ ] **Step 5: Commit** - -```bash -git add server/auth.py server/tests/test_auth_helpers.py -git commit -m "feat(server): add _ensure_admin helper and require_admin FastAPI dep" -``` - ---- - -## Task 3: Admin-gate `GET /configure` and `POST /configure` - -**Files:** -- Create: `server/tests/test_admin_gating.py` -- Modify: `server/main.py:303-318` - -- [ ] **Step 1: Write the failing tests** - -Write `server/tests/test_admin_gating.py`: - -```python -"""Integration tests for admin-gated routes. - -Covers, for each affected route, the regression matrix: -- admin JWT -> success -- ADMIN_API_KEY -> success (no DB user required) -- AUTH_DISABLED -> success -- member JWT -> 403 -- no auth -> 401 -""" - -from __future__ import annotations - -import pytest - - -# --- GET /configure --- - - -def test_get_configure_admin_jwt(client, auth_admin_header): - response = client.get("/configure", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_get_configure_admin_api_key(client, admin_api_key_env): - response = client.get("/configure", headers=admin_api_key_env) - assert response.status_code == 200 - - -def test_get_configure_auth_disabled(client, auth_disabled_env): - response = client.get("/configure") - assert response.status_code == 200 - - -def test_get_configure_member_forbidden(client, auth_member_header): - response = client.get("/configure", headers=auth_member_header) - assert response.status_code == 403 - - -def test_get_configure_no_auth_unauthorized(client): - response = client.get("/configure") - assert response.status_code == 401 - - -# --- POST /configure --- - - -def _valid_config(): - return { - "vector_store": {"provider": "pgvector", "config": {"host": "h"}}, - "llm": {"provider": "openai", "config": {"api_key": "x", "model": "m"}}, - "embedder": {"provider": "openai", "config": {"api_key": "x", "model": "e"}}, - } - - -def test_post_configure_admin_jwt(client, auth_admin_header): - response = client.post("/configure", headers=auth_admin_header, json=_valid_config()) - assert response.status_code == 200 - - -def test_post_configure_admin_api_key(client, admin_api_key_env): - response = client.post("/configure", headers=admin_api_key_env, json=_valid_config()) - assert response.status_code == 200 - - -def test_post_configure_auth_disabled(client, auth_disabled_env): - response = client.post("/configure", json=_valid_config()) - assert response.status_code == 200 - - -def test_post_configure_member_forbidden(client, auth_member_header): - response = client.post("/configure", headers=auth_member_header, json=_valid_config()) - assert response.status_code == 403 - - -def test_post_configure_no_auth_unauthorized(client): - response = client.post("/configure", json=_valid_config()) - assert response.status_code == 401 -``` - -- [ ] **Step 2: Run the tests, confirm member tests FAIL with 200 instead of 403** - -Run: `cd server && pytest tests/test_admin_gating.py::test_get_configure_member_forbidden tests/test_admin_gating.py::test_post_configure_member_forbidden -v` -Expected: both FAIL with `assert 200 == 403` (the routes currently accept any authenticated caller). - -- [ ] **Step 3: Swap the dep on both `/configure` routes** - -Edit `server/main.py`. Change lines 303-305 (`GET /configure`): - -From: -```python -@app.get("/configure", summary="Get current Mem0 configuration") -def get_config(_auth=Depends(verify_auth)): - return _redact_config(get_current_config()) -``` - -To: -```python -@app.get("/configure", summary="Get current Mem0 configuration") -def get_config(_admin=Depends(require_admin)): - return _redact_config(get_current_config()) -``` - -Change lines 313-318 (`POST /configure`): - -From: -```python -@app.post("/configure", summary="Configure Mem0") -def set_config(config: Dict[str, Any], _auth=Depends(verify_auth)): - """Set memory configuration.""" - _validate_bundled_providers(config) - update_config(config) - return {"message": "Configuration set successfully"} -``` - -To: -```python -@app.post("/configure", summary="Configure Mem0") -def set_config(config: Dict[str, Any], _admin=Depends(require_admin)): - """Set memory configuration.""" - _validate_bundled_providers(config) - update_config(config) - return {"message": "Configuration set successfully"} -``` - -Update the import at the top of `server/main.py` (line 16) — add `require_admin`: - -From: -```python -from auth import ADMIN_API_KEY, AUTH_DISABLED, JWT_SECRET, verify_auth -``` - -To: -```python -from auth import ADMIN_API_KEY, AUTH_DISABLED, JWT_SECRET, require_admin, verify_auth -``` - -- [ ] **Step 4: Run the configure tests, confirm all pass** - -Run: `cd server && pytest tests/test_admin_gating.py -k "configure" -v` -Expected: `10 passed`. - -- [ ] **Step 5: Commit** - -```bash -git add server/main.py server/tests/test_admin_gating.py -git commit -m "feat(server): admin-gate GET/POST /configure" -``` - ---- - -## Task 4: Admin-gate `POST /reset` - -**Files:** -- Modify: `server/main.py:475-482` -- Modify: `server/tests/test_admin_gating.py` (append) - -- [ ] **Step 1: Append failing tests for `/reset`** - -Append to `server/tests/test_admin_gating.py`: - -```python - - -# --- POST /reset --- - - -def test_post_reset_admin_jwt(client, auth_admin_header): - response = client.post("/reset", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_post_reset_admin_api_key(client, admin_api_key_env): - response = client.post("/reset", headers=admin_api_key_env) - assert response.status_code == 200 - - -def test_post_reset_auth_disabled(client, auth_disabled_env): - response = client.post("/reset") - assert response.status_code == 200 - - -def test_post_reset_member_forbidden(client, auth_member_header): - response = client.post("/reset", headers=auth_member_header) - assert response.status_code == 403 - - -def test_post_reset_no_auth_unauthorized(client): - response = client.post("/reset") - assert response.status_code == 401 -``` - -- [ ] **Step 2: Run, confirm member test FAILS** - -Run: `cd server && pytest tests/test_admin_gating.py::test_post_reset_member_forbidden -v` -Expected: FAIL with `assert 200 == 403`. - -- [ ] **Step 3: Swap the dep on `POST /reset`** - -Edit `server/main.py`. Change lines 475-482: - -From: -```python -@app.post("/reset", summary="Reset all memories") -def reset_memory(_auth=Depends(verify_auth)): - """Completely reset stored memories.""" - try: - get_memory_instance().reset() - return {"message": "All memories reset"} - except Exception: - raise upstream_error() -``` - -To: -```python -@app.post("/reset", summary="Reset all memories") -def reset_memory(_admin=Depends(require_admin)): - """Completely reset stored memories.""" - try: - get_memory_instance().reset() - return {"message": "All memories reset"} - except Exception: - raise upstream_error() -``` - -- [ ] **Step 4: Run, confirm all pass** - -Run: `cd server && pytest tests/test_admin_gating.py -k "reset" -v` -Expected: `5 passed`. - -- [ ] **Step 5: Commit** - -```bash -git add server/main.py server/tests/test_admin_gating.py -git commit -m "feat(server): admin-gate POST /reset" -``` - ---- - -## Task 5: Admin-gate `GET /entities` and `DELETE /entities/{type}/{id}` - -**Files:** -- Modify: `server/routers/entities.py` -- Modify: `server/tests/test_admin_gating.py` (append) - -- [ ] **Step 1: Append failing tests for `/entities`** - -Append to `server/tests/test_admin_gating.py`: - -```python - - -# --- GET /entities --- - - -def test_get_entities_admin_jwt(client, auth_admin_header): - response = client.get("/entities", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_get_entities_admin_api_key(client, admin_api_key_env): - response = client.get("/entities", headers=admin_api_key_env) - assert response.status_code == 200 - - -def test_get_entities_auth_disabled(client, auth_disabled_env): - response = client.get("/entities") - assert response.status_code == 200 - - -def test_get_entities_member_forbidden(client, auth_member_header): - response = client.get("/entities", headers=auth_member_header) - assert response.status_code == 403 - - -def test_get_entities_no_auth_unauthorized(client): - response = client.get("/entities") - assert response.status_code == 401 - - -# --- DELETE /entities/{type}/{id} --- - - -def test_delete_entity_admin_jwt(client, auth_admin_header): - response = client.delete("/entities/user/alice", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_delete_entity_admin_api_key(client, admin_api_key_env): - response = client.delete("/entities/user/alice", headers=admin_api_key_env) - assert response.status_code == 200 - - -def test_delete_entity_auth_disabled(client, auth_disabled_env): - response = client.delete("/entities/user/alice") - assert response.status_code == 200 - - -def test_delete_entity_member_forbidden(client, auth_member_header): - response = client.delete("/entities/user/alice", headers=auth_member_header) - assert response.status_code == 403 - - -def test_delete_entity_no_auth_unauthorized(client): - response = client.delete("/entities/user/alice") - assert response.status_code == 401 -``` - -- [ ] **Step 2: Run, confirm member tests FAIL** - -Run: `cd server && pytest tests/test_admin_gating.py -k "entities" -v` -Expected: 2 failures on the `_member_forbidden` tests. - -- [ ] **Step 3: Swap the deps on both entity routes** - -Edit `server/routers/entities.py`. - -Change the imports (line 8): - -From: -```python -from auth import verify_auth -``` - -To: -```python -from auth import require_admin -``` - -Change lines 44-45: - -From: -```python -@router.get("", response_model=list[Entity]) -def list_entities(_auth=Depends(verify_auth)): -``` - -To: -```python -@router.get("", response_model=list[Entity]) -def list_entities(_admin=Depends(require_admin)): -``` - -Change lines 71-72: - -From: -```python -@router.delete("/{entity_type}/{entity_id}", response_model=MessageResponse) -def delete_entity(entity_type: EntityType, entity_id: str, _auth=Depends(verify_auth)): -``` - -To: -```python -@router.delete("/{entity_type}/{entity_id}", response_model=MessageResponse) -def delete_entity(entity_type: EntityType, entity_id: str, _admin=Depends(require_admin)): -``` - -- [ ] **Step 4: Run, confirm all entity tests pass** - -Run: `cd server && pytest tests/test_admin_gating.py -k "entities or entity" -v` -Expected: `10 passed`. - -- [ ] **Step 5: Commit** - -```bash -git add server/routers/entities.py server/tests/test_admin_gating.py -git commit -m "feat(server): admin-gate GET /entities and DELETE /entities/{type}/{id}" -``` - ---- - -## Task 6: Admin-gate `GET /requests` - -**Files:** -- Modify: `server/routers/requests.py` -- Modify: `server/tests/test_admin_gating.py` (append) - -- [ ] **Step 1: Append failing tests for `/requests`** - -Append to `server/tests/test_admin_gating.py`: - -```python - - -# --- GET /requests --- - - -def test_get_requests_admin_jwt(client, auth_admin_header): - response = client.get("/requests", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_get_requests_admin_api_key(client, admin_api_key_env): - response = client.get("/requests", headers=admin_api_key_env) - assert response.status_code == 200 - - -def test_get_requests_auth_disabled(client, auth_disabled_env): - response = client.get("/requests") - assert response.status_code == 200 - - -def test_get_requests_member_forbidden(client, auth_member_header): - response = client.get("/requests", headers=auth_member_header) - assert response.status_code == 403 - - -def test_get_requests_no_auth_unauthorized(client): - response = client.get("/requests") - assert response.status_code == 401 -``` - -- [ ] **Step 2: Run, confirm member test FAILS** - -Run: `cd server && pytest tests/test_admin_gating.py -k "requests" -v` -Expected: 1 failure on `test_get_requests_member_forbidden` (currently passes auth via `require_auth`). - -- [ ] **Step 3: Swap dep + drop unused `user` parameter** - -Edit `server/routers/requests.py`. - -Change the imports block: - -From: -```python -from datetime import datetime -import uuid - -from fastapi import APIRouter, Depends, Query -from pydantic import BaseModel -from sqlalchemy import select -from sqlalchemy.orm import Session - -from auth import require_auth -from db import get_db -from models import RequestLog, User -``` - -To: -```python -from datetime import datetime -import uuid - -from fastapi import APIRouter, Depends, Query -from pydantic import BaseModel -from sqlalchemy import select -from sqlalchemy.orm import Session - -from auth import require_admin -from db import get_db -from models import RequestLog -``` - -(Dropped `require_auth` and the now-unused `User` import.) - -Change the route handler (lines 31-37): - -From: -```python -@router.get("", response_model=list[RequestLogItem]) -def list_requests( - user: User = Depends(require_auth), - db: Session = Depends(get_db), - limit: int = Query(default=50, ge=1, le=200), -): -``` - -To: -```python -@router.get("", response_model=list[RequestLogItem]) -def list_requests( - _admin=Depends(require_admin), - db: Session = Depends(get_db), - limit: int = Query(default=50, ge=1, le=200), -): -``` - -- [ ] **Step 4: Run all admin-gating tests, confirm all pass** - -Run: `cd server && pytest tests/test_admin_gating.py -v` -Expected: `30 passed` (5 routes × 5 cases each = 25, plus 5 from delete_entity = 30). - -- [ ] **Step 5: Commit** - -```bash -git add server/routers/requests.py server/tests/test_admin_gating.py -git commit -m "feat(server): admin-gate GET /requests" -``` - ---- - -## Task 7: Inside-route guard on `GET /memories` (no-filter branch) - -**Files:** -- Modify: `server/main.py:387-403` -- Modify: `server/tests/test_admin_gating.py` (append) - -- [ ] **Step 1: Append failing tests** - -Append to `server/tests/test_admin_gating.py`: - -```python - - -# --- GET /memories (no filter branch only) --- - - -def test_get_memories_no_filter_admin_jwt(client, auth_admin_header): - response = client.get("/memories", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_get_memories_no_filter_admin_api_key(client, admin_api_key_env): - response = client.get("/memories", headers=admin_api_key_env) - assert response.status_code == 200 - - -def test_get_memories_no_filter_auth_disabled(client, auth_disabled_env): - response = client.get("/memories") - assert response.status_code == 200 - - -def test_get_memories_no_filter_member_forbidden(client, auth_member_header): - """The info-disclosure branch (_list_all_memories) is admin-only.""" - response = client.get("/memories", headers=auth_member_header) - assert response.status_code == 403 - - -def test_get_memories_filtered_member_succeeds(client, auth_member_header): - """Filtered queries are unchanged — member can still call them.""" - response = client.get("/memories?user_id=alice", headers=auth_member_header) - assert response.status_code == 200 - - -def test_get_memories_no_auth_unauthorized(client): - response = client.get("/memories") - assert response.status_code == 401 -``` - -- [ ] **Step 2: Run, confirm member-no-filter FAILS** - -Run: `cd server && pytest tests/test_admin_gating.py -k "memories_no_filter_member" -v` -Expected: FAIL with `assert 200 == 403`. - -- [ ] **Step 3: Refactor `GET /memories` to use the inside-route guard** - -Edit `server/main.py`. - -Update the import at line 16 to also pull `_ensure_admin`: - -From: -```python -from auth import ADMIN_API_KEY, AUTH_DISABLED, JWT_SECRET, require_admin, verify_auth -``` - -To: -```python -from auth import ADMIN_API_KEY, AUTH_DISABLED, JWT_SECRET, _ensure_admin, require_admin, verify_auth -``` - -Replace the `GET /memories` handler (lines 387-403). From: - -```python -@app.get("/memories", summary="Get memories") -def get_all_memories( - user_id: Optional[str] = None, - run_id: Optional[str] = None, - agent_id: Optional[str] = None, - _auth=Depends(verify_auth), -): - """Retrieve stored memories. Lists all memories when no identifier is provided.""" - try: - if not any([user_id, run_id, agent_id]): - return _list_all_memories() - filters = { - k: v for k, v in {"user_id": user_id, "run_id": run_id, "agent_id": agent_id}.items() if v is not None - } - return get_memory_instance().get_all(filters=filters) - except Exception: - raise upstream_error() -``` - -To: - -```python -@app.get("/memories", summary="Get memories") -def get_all_memories( - request: Request, - user_id: Optional[str] = None, - run_id: Optional[str] = None, - agent_id: Optional[str] = None, - user: User | None = Depends(verify_auth), -): - """Retrieve stored memories. Lists all memories when no identifier is provided. - - Note: the unfiltered listing branch is admin-only — it would otherwise leak - every payload in the vector store across tenants once multi-user lands. - Filtered queries remain available to any authenticated caller.""" - try: - if not any([user_id, run_id, agent_id]): - _ensure_admin(request, user) - return _list_all_memories() - filters = { - k: v for k, v in {"user_id": user_id, "run_id": run_id, "agent_id": agent_id}.items() if v is not None - } - return get_memory_instance().get_all(filters=filters) - except HTTPException: - raise - except Exception: - raise upstream_error() -``` - -Two changes worth flagging: -- The signature now takes `request: Request` and renames `_auth` → `user` so the guard can read it. -- The `try` block now re-raises `HTTPException` before falling through to `upstream_error()`. Without this, the 403 we just added would get swallowed and turned into a generic 502/500. - -Also add `User` to the imports if it's not already there. Check `server/main.py:31` — it imports `RequestLog, User` from `models`, so we're covered. - -- [ ] **Step 4: Run, confirm all `/memories` tests pass** - -Run: `cd server && pytest tests/test_admin_gating.py -k "memories" -v` -Expected: `6 passed`. - -- [ ] **Step 5: Run the full admin-gating suite to confirm no regression** - -Run: `cd server && pytest tests/test_admin_gating.py -v` -Expected: `36 passed`. - -- [ ] **Step 6: Commit** - -```bash -git add server/main.py server/tests/test_admin_gating.py -git commit -m "feat(server): admin-gate unfiltered GET /memories branch" -``` - ---- - -## Task 8: Regression tests for unchanged routes - -**Files:** -- Create: `server/tests/test_unchanged_routes.py` - -These tests assert that the routes we *deliberately* left alone still behave correctly for admins (the only user type that exists today). They lock the behavior in place so future refactors can't silently change it. - -- [ ] **Step 1: Write the regression tests** - -Write `server/tests/test_unchanged_routes.py`: - -```python -"""Regression tests for routes deliberately left untouched by the authz fix. - -These prove the per-route changes did not accidentally regress memory CRUD, -search, or the API-key management endpoints.""" - -from __future__ import annotations - - -def test_post_memories_admin_succeeds(client, auth_admin_header): - response = client.post( - "/memories", - headers=auth_admin_header, - json={ - "messages": [{"role": "user", "content": "I love pizza."}], - "user_id": "alice", - }, - ) - assert response.status_code == 200 - - -def test_post_memories_requires_identifier(client, auth_admin_header): - response = client.post( - "/memories", - headers=auth_admin_header, - json={"messages": [{"role": "user", "content": "hi"}]}, - ) - assert response.status_code == 400 - - -def test_get_memories_filtered_admin_succeeds(client, auth_admin_header): - response = client.get("/memories?user_id=alice", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_post_search_admin_succeeds(client, auth_admin_header): - response = client.post( - "/search", - headers=auth_admin_header, - json={"query": "pizza", "user_id": "alice"}, - ) - assert response.status_code == 200 - - -def test_get_memory_by_id_admin_succeeds(client, auth_admin_header): - response = client.get("/memories/some-id", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_delete_memories_filtered_admin_succeeds(client, auth_admin_header): - response = client.delete("/memories?user_id=alice", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_delete_memories_no_filter_returns_400(client, auth_admin_header): - """Even admin must supply an identifier for filtered delete.""" - response = client.delete("/memories", headers=auth_admin_header) - assert response.status_code == 400 - - -def test_get_configure_providers_admin_succeeds(client, auth_admin_header): - """GET /configure/providers stays on verify_auth (not admin-gated).""" - response = client.get("/configure/providers", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_get_configure_providers_member_succeeds(client, auth_member_header): - """Members can still query bundled providers — not admin-gated.""" - response = client.get("/configure/providers", headers=auth_member_header) - assert response.status_code == 200 - - -def test_api_keys_list_admin_succeeds(client, auth_admin_header): - response = client.get("/api-keys", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_api_keys_list_member_succeeds(client, auth_member_header): - """API-key management remains scoped to the caller (require_auth, not require_admin).""" - response = client.get("/api-keys", headers=auth_member_header) - assert response.status_code == 200 - - -def test_auth_me_admin_succeeds(client, auth_admin_header): - response = client.get("/auth/me", headers=auth_admin_header) - assert response.status_code == 200 - - -def test_setup_status_no_auth(client): - """Open route — no auth required even after the fix.""" - response = client.get("/auth/setup-status") - assert response.status_code == 200 -``` - -- [ ] **Step 2: Run the regression suite** - -Run: `cd server && pytest tests/test_unchanged_routes.py -v` -Expected: `13 passed`. - -- [ ] **Step 3: Run the full server test suite to confirm everything is green** - -Run: `cd server && pytest tests/ -v` -Expected: `55 passed` (6 helpers + 36 admin-gating + 13 unchanged). - -- [ ] **Step 4: Commit** - -```bash -git add server/tests/test_unchanged_routes.py -git commit -m "test(server): regression coverage for routes untouched by authz fix" -``` - ---- - -## Task 9: Docs — surgical edits to `rest-api.mdx` - -**Files:** -- Modify: `docs/open-source/features/rest-api.mdx` - -- [ ] **Step 1: Update the auth-table row for per-user API keys (line 134)** - -Find this line (in the auth-modes table around lines 131-136): - -```markdown -| Per-user API key | `X-API-Key: m0sk_...` | Programmatic access scoped to a single dashboard user | -``` - -Replace with: - -```markdown -| Per-user API key | `X-API-Key: m0sk_...` | Programmatic access tied to the single admin user today. Per-user scoping arrives with the upcoming multi-user invite flow | -``` - -- [ ] **Step 2: Remove the inaccurate "inherit scope" sentence (line 185)** - -Find this line: - -```markdown -Per-user keys inherit the creating user's scope. List or revoke them via `GET /api-keys` and `DELETE /api-keys/{id}`. -``` - -Replace with: - -```markdown -List or revoke per-user keys via `GET /api-keys` and `DELETE /api-keys/{id}`. The dashboard user issued the key remains its owner; admin-only routes such as `/configure`, `/reset`, `/entities`, and `/requests` require the issuing user to have the admin role. -``` - -- [ ] **Step 3: Annotate admin-only endpoints in the reference tables** - -In the "Memory operations" table (around lines 247-260), change: - -```markdown -| `POST` | `/configure` | Set memory configuration. Rejects unbundled providers with a 400 | -| `GET` | `/configure` | Get the current memory configuration | -``` - -To: - -```markdown -| `POST` | `/configure` | **Admin only.** Set memory configuration. Rejects unbundled providers with a 400 | -| `GET` | `/configure` | **Admin only.** Get the current memory configuration | -``` - -And: - -```markdown -| `POST` | `/reset` | Reset all memories | -``` - -To: - -```markdown -| `POST` | `/reset` | **Admin only.** Reset all memories | -``` - -In the "Entities" table (around lines 290-296), change: - -```markdown -| `GET` | `/entities` | Distinct `user_id` / `agent_id` / `run_id` values with memory counts | -| `DELETE` | `/entities/{entity_type}/{entity_id}` | Cascade-delete all memories for an entity; `entity_type` is `user`, `agent`, or `run` | -``` - -To: - -```markdown -| `GET` | `/entities` | **Admin only.** Distinct `user_id` / `agent_id` / `run_id` values with memory counts | -| `DELETE` | `/entities/{entity_type}/{entity_id}` | **Admin only.** Cascade-delete all memories for an entity; `entity_type` is `user`, `agent`, or `run` | -``` - -In the "Request logs" table: - -```markdown -| `GET` | `/requests?limit=N` | Recent API call log (JWT or admin key) | -``` - -To: - -```markdown -| `GET` | `/requests?limit=N` | **Admin only.** Recent API call log | -``` - -- [ ] **Step 4: Add a "coming soon" marker to the multi-user promise (line ~267)** - -Find this line in the auth-endpoints section: - -```markdown -| `POST` | `/auth/register` | Register the first admin. Registration closes after the first admin is created; additional accounts are provisioned by the existing admin. | -``` - -Replace with: - -```markdown -| `POST` | `/auth/register` | Register the first admin. Registration closes after the first admin is created; additional accounts will be provisioned by the existing admin (multi-user invite flow coming soon). | -``` - -- [ ] **Step 5: Verify `docs/llms.txt` is still in sync** - -Run: `python scripts/check-llms-txt-coverage.py` -Expected: exit 0 (no changes — we did not add or remove any `.mdx` pages, just edited an existing one). - -- [ ] **Step 6: Commit** - -```bash -git add docs/open-source/features/rest-api.mdx -git commit -m "docs(rest-api): document admin-only routes and clarify per-user-key scope" -``` - ---- - -## Task 10: CI workflow for `server/` - -**Files:** -- Create: `.github/workflows/server-ci.yml` - -- [ ] **Step 1: Create the workflow file** - -Write `.github/workflows/server-ci.yml`: - -```yaml -name: Server CI - -on: - workflow_dispatch: - push: - branches: [main] - paths: - - 'server/**' - - '.github/workflows/server-ci.yml' - pull_request: - paths: - - 'server/**' - - '.github/workflows/server-ci.yml' - -jobs: - test: - runs-on: ubuntu-latest - strategy: - matrix: - python-version: ["3.10", "3.11", "3.12"] - steps: - - uses: actions/checkout@v4 - - - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 - with: - python-version: ${{ matrix.python-version }} - - - name: Install dev dependencies - working-directory: server - run: pip install -r requirements-dev.txt - - - name: Run tests - working-directory: server - env: - JWT_SECRET: ci-test-secret-do-not-use-in-prod-xxxxxxxxxxxxxxxxxxxxxxxxxxxxx - OPENAI_API_KEY: ci-test-key - run: pytest tests/ -v -``` - -- [ ] **Step 2: Verify locally that the workflow would pass** - -Run: `cd server && JWT_SECRET=ci-test-secret-do-not-use-in-prod-xxxxxxxxxxxxxxxxxxxxxxxxxxxxx OPENAI_API_KEY=ci-test-key pytest tests/ -v` -Expected: `55 passed`. - -- [ ] **Step 3: Commit** - -```bash -git add .github/workflows/server-ci.yml -git commit -m "ci(server): add pytest workflow on PRs touching server/" -``` - ---- - -## Task 11: Final verification + PR draft - -- [ ] **Step 1: Run the full test suite one more time** - -Run: `cd server && pytest tests/ -v` -Expected: `55 passed, 0 failed`. - -- [ ] **Step 2: Review the diff** - -Run: `git log --oneline main..HEAD` and `git diff main --stat` -Expected: ~10 commits, modifications confined to `server/`, `docs/open-source/features/rest-api.mdx`, `docs/superpowers/`, and `.github/workflows/server-ci.yml`. Roughly 30 LOC of production code change, the rest is tests, docs, and CI. - -- [ ] **Step 3: Draft the PR description** - -Use this template: - -```markdown -## Linked Issue - -Closes — security report from vonbrubeck@gmail.com (2026-05-01) on REST API per-user API key authorization. - -## Description - -Hardens the self-hosted OSS REST server's authorization model by admin-gating -six global / destructive / info-disclosing routes and adding an inside-route -guard on `GET /memories`. Also adds the first `server/tests/` pytest scaffold -so this change (and future ones) have automated regression coverage. - -Scope: B+ from `docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md`. -Deferred to follow-up issue: -- `owner_id` on memory payloads -- Multi-user invite/provision flow -- Rate-limiting `/generate-instructions` -- Per-owner filtering on `/requests` - -Zero behavior change on every supported deployment mode today (single-admin -JWT, single-admin per-user API key, legacy `ADMIN_API_KEY` env, -`AUTH_DISABLED=true`). - -## Type of Change - -- [x] Bug fix (non-breaking change that fixes an issue) -- [ ] New feature -- [ ] Breaking change -- [ ] Refactor -- [x] Documentation update - -## Breaking Changes - -None. The new 403 path is reachable only by a non-admin `User`, which the -current schema (partial unique index in migration 004, closed `/auth/register`) -cannot produce via public APIs. - -## Test Coverage - -- [x] Added unit tests for `_ensure_admin` -- [x] Added integration tests for the 6 admin-gated routes + `GET /memories` inside-route guard -- [x] Added regression tests for routes deliberately left unchanged -- [x] Tested manually: `ADMIN_API_KEY` env, `AUTH_DISABLED=true`, admin JWT, admin per-user key - -## Checklist - -- [x] Code follows the project's style guidelines -- [x] Self-review performed -- [x] New tests prove the fix works -- [x] All tests pass locally -- [x] Documentation updated -``` - -- [ ] **Step 4: Open the PR** - -```bash -git push -u origin -gh pr create --title "fix(server): admin-gate global REST routes; fix docs/code authz mismatch" \ - --body-file <(cat <<'EOF' -[paste PR body from Step 3] -EOF -) -``` - -- [ ] **Step 5: After PR opens, file the follow-up issue** - -Body of the follow-up GitHub issue (single body, no separate design doc): - -```markdown -## Follow-up: REST API multi-user authorization (owner_id + invite flow) - -Tracked from # per `docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md`. - -The B+ fix in # admin-gates the global/destructive routes but does not -add per-user scoping to the memory CRUD routes. That requires: - -### 1. `owner_id` on memory payloads -Decide whether to store `owner_id` as a JSONB payload key or as a first-class -column on the vector-store row. Spec the backfill story for existing -deployments (assign all existing rows to the single admin). Wire the write -path (`POST /memories`) to inject `owner_id` from the authenticated `User`, -and every read path (`GET /memories` filtered, `POST /search`, all -`memory_id` ops) to filter or check ownership. - -### 2. Multi-user invite flow -The docs at `rest-api.mdx` already promise this. Spec the UX: roles -(admin/member/viewer?), per-org structure, invite endpoint, accept flow, -role transitions. - -### 3. Rate-limit `POST /generate-instructions` -Burns LLM tokens charged to the server's `OPENAI_API_KEY`. Adding a limit -could regress existing batch callers — gather usage data first, then decide. - -### 4. Per-owner filtering on `GET /requests` -Requires `owner_id` on the `RequestLog` table (DB migration). -``` - ---- - -## Self-Review - -Checking the plan against the spec: - -**Spec coverage:** - -| Spec section | Task(s) | -|---|---| -| `_ensure_admin` helper + `require_admin` dep | Task 2 | -| Full gate on `/configure` (GET + POST) | Task 3 | -| Full gate on `/reset` | Task 4 | -| Full gate on `/entities` (GET + DELETE) | Task 5 | -| Full gate on `/requests` | Task 6 | -| Inside-route guard on `GET /memories` | Task 7 | -| Routes deliberately NOT changed | Task 8 (regression coverage) | -| Docs surgical edits | Task 9 | -| Test scaffolding (`server/tests/`) | Tasks 1, 2, 3-8 | -| CI workflow | Task 10 | -| Regression matrix coverage (5 modes) | Built into every gating test | -| Acceptance criterion: ADMIN_API_KEY / AUTH_DISABLED automated coverage | Tasks 3-7 each include both | -| Follow-up issue body | Task 11, Step 5 | - -**Type and method consistency:** - -- `_ensure_admin(request, user)` signature is consistent in `server/auth.py`, the unit tests in Task 2, and the inside-route call site in Task 7. -- `require_admin` is called via `_admin=Depends(require_admin)` in every modified route (Tasks 3-6). -- Test fixtures (`admin_user`, `member_user`, `auth_admin_header`, `auth_member_header`, `admin_api_key_env`, `auth_disabled_env`) are defined in `conftest.py` (Task 1) and reused unchanged in Tasks 2-8. -- `JWT_SECRET` and `OPENAI_API_KEY` env vars are set in conftest (Task 1) and again in the CI workflow (Task 10) — same names. - -**Placeholder scan:** none. Every step contains either a complete code block, an exact command, or a precise instruction with a from→to diff. - -**KISS / DRY / minimal-changes audit:** - -- Single helper (`_ensure_admin`) reused by both the FastAPI dep and the inline route guard — DRY. -- Six dep swaps are mechanically identical — one pattern, six call sites — KISS. -- No new infrastructure beyond what the spec required (no Postgres for tests, no separate design doc for the follow-up). -- `GET /configure/providers` deliberately not admin-gated, per the trim agreed during brainstorming. - ---- - -## Execution Handoff - -Plan complete and saved to `docs/superpowers/plans/2026-05-13-rest-api-authz-fix-plan.md`. Two execution options: - -**1. Subagent-Driven (recommended)** — I dispatch a fresh subagent per task, review between tasks, fast iteration. - -**2. Inline Execution** — Execute tasks in this session using executing-plans, batch execution with checkpoints. - -Which approach? diff --git a/docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md b/docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md deleted file mode 100644 index e8ee312cc..000000000 --- a/docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md +++ /dev/null @@ -1,274 +0,0 @@ -# REST API Authorization Fix — Design - -**Date:** 2026-05-13 -**Branch (target):** `main` -**Scope:** `server/` (self-hosted OSS REST server) + `docs/open-source/features/rest-api.mdx` -**Trigger:** Security report from `vonbrubeck@gmail.com` (2026-05-01) titled -"Mem0 OSS REST per-user API keys can access and delete other users' memories." - -## Problem - -The self-hosted OSS REST server documents per-user API keys as "scoped to a -single dashboard user" (`docs/open-source/features/rest-api.mdx:131-136, 185`), -but the memory, configure, entities, and request-log routes resolve the caller -via `verify_auth` (`server/auth.py:145-161`) and then **discard the resolved -`User`**. Caller-supplied `user_id`/`agent_id`/`run_id` values are forwarded -straight to the memory backend, raw `memory_id` operations skip ownership -checks, and `/configure`/`/reset` accept any authenticated principal. - -The report's source-level claims are accurate against the current HEAD. Every -cited line behaves as described. - -## Threat model — what the report gets right, and what it under-weighs - -**Correct:** the contract in the docs and the behavior in the code do not -match. The routes do not enforce a per-user scope, and `/configure`, `/reset`, -`/entities` accept any authenticated caller as if they were admins. - -**Under-weighed by the report:** today only one `User` can exist with -`role = "admin"`. Migration `004_unique_admin_role.py` enforces this via a -partial unique index on `role = 'admin'`, and `/auth/register` -(`server/routers/auth.py:100-114`) is closed after the first user is -registered. There is **no public endpoint** to create non-admin users. The -Alice-vs-Bob attack sketch therefore cannot be constructed through the public -API as the code stands — the cross-tenant attack surface has exactly one -tenant. - -The vulnerability is real but **latent**: it detonates the moment a second -`User` row appears, whether via a future invite endpoint (the docs at -`rest-api.mdx:267` already promise "additional accounts are provisioned by the -existing admin") or via direct database manipulation. - -## Decision: scope B+ (admin-gate now, defer owner_id) - -We fix the directly-fixable surface — global, destructive, and -info-disclosing routes — and align the docs with reality. We do **not** add -`owner_id` to memory payloads in this PR. The `owner_id` design needs the -multi-user invite UX to be specified first (admin/member/viewer? per-org? -hierarchical?), and JSONB payload schemas are very hard to back out once -vector stores carry production data. That work is tracked as a follow-up -issue. - -Rejected alternatives: - -- **A — Full multi-tenant now.** Premature schema commitment for an - unspecified product feature. Violates KISS, locks us into payload shape we'd - regret. -- **C — Doc-only (retract the multi-user promise).** Worse signal than "not - yet"; the report's findings would remain "known limitation" rather than - fixed. - -## Authorization model - -One new helper in `server/auth.py`: - -```python -def _ensure_admin(request: Request, user: User | None) -> None: - """Single source of truth for admin gating. - Allows: legacy ADMIN_API_KEY env (X-API-Key), AUTH_DISABLED=true, or - a registered User with role == 'admin'. Raises 403 otherwise.""" - auth_type = getattr(request.state, "auth_type", "none") - if auth_type in {"admin_api_key", "disabled"}: - return - if user is not None and user.role == "admin": - return - raise HTTPException(status_code=403, detail="Admin role required.") - - -async def require_admin( - request: Request, - user: User | None = Depends(verify_auth), -) -> None: - """FastAPI dependency. Pure gate — returns nothing.""" - _ensure_admin(request, user) -``` - -Properties: - -- Pure gate: returns `None`, never invents a synthetic `User`. -- `_ensure_admin` is shared by the dep and by the inside-route guard on - `GET /memories` (DRY). -- FastAPI dedupes the sub-dep, so `verify_auth` runs once per request. - -## Regression matrix (current deployment modes × affected routes) - -| Mode | What `verify_auth` returns today | After fix on admin-gated routes | -|---|---|---| -| Single admin JWT | admin `User` | passes (`role == "admin"`) | -| Single admin per-user key (`m0sk_…`) | admin `User` | passes (same) | -| Legacy `ADMIN_API_KEY` env (X-API-Key) | `None`, `auth_type="admin_api_key"` | passes (auth-type short-circuit) | -| `AUTH_DISABLED=true` | `None`, `auth_type="disabled"` | passes (auth-type short-circuit) | -| Hypothetical non-admin User | n/a (cannot exist today) | 403 | - -**Zero behavior change on every supported deployment today.** The new 403 is -reachable only by a `User` type the current schema cannot produce. - -## Per-route changes - -### Full gate (`verify_auth` → `require_admin`) - -| Route | File:line | Reason | -|---|---|---| -| `GET /configure` | `server/main.py:303-305` | Pairs with `POST /configure`; redacted config still exposes provider/topology to future non-admins | -| `POST /configure` | `server/main.py:313-318` | Global config mutation; **report-flagged** | -| `POST /reset` | `server/main.py:475-482` | Destructive global op; **report-flagged** | -| `GET /entities` | `server/routers/entities.py:44-68` | Info disclosure of every user/agent/run ID; **report-flagged** | -| `DELETE /entities/{type}/{id}` | `server/routers/entities.py:71-77` | Destructive global op masquerading as tenant action; **report-flagged** | -| `GET /requests` | `server/routers/requests.py:31-47` | Cross-user log leak the day a non-admin exists; **meta-reviewer-flagged** | - -Mechanical change at each call site: replace `_auth=Depends(verify_auth)` -with `_admin=Depends(require_admin)`. In `requests.py`, drop the unused -`user: User = Depends(require_auth)` parameter. - -### Inside-route guard (filtered branch unchanged) - -`GET /memories` (`server/main.py:387-403`) — outer dep stays -`verify_auth`. Inside the route, on the no-filter branch (which currently -calls `_list_all_memories`), call `_ensure_admin(request, user)`. The -filtered branch (`?user_id=alice`) is untouched. - -```python -@app.get("/memories", summary="Get memories") -def get_all_memories( - request: Request, - user_id: Optional[str] = None, - run_id: Optional[str] = None, - agent_id: Optional[str] = None, - user: User | None = Depends(verify_auth), -): - try: - if not any([user_id, run_id, agent_id]): - _ensure_admin(request, user) - return _list_all_memories() - filters = { - k: v for k, v in {"user_id": user_id, "run_id": run_id, "agent_id": agent_id}.items() - if v is not None - } - return get_memory_instance().get_all(filters=filters) - except Exception: - raise upstream_error() -``` - -Rationale: full-gating the route would change behavior for the documented -per-user-key pattern (filtered queries). The inside-route guard only changes -the info-disclosure branch — the only branch the report's "list all" attack -uses. - -### Deliberately NOT changed in this PR - -| Route | Why | -|---|---| -| `POST /memories` | Real fix needs `owner_id` (follow-up) | -| `POST /search` | Same | -| `GET /memories/{memory_id}`, `PUT`, `DELETE`, `/history` | Same — ownership check requires `owner_id` | -| `DELETE /memories?user_id=…` (filtered branch) | Same | -| `POST /generate-instructions` | Rate-limit could regress batch callers; defer | -| `GET /configure/providers` | Static bundled-provider tuple; not sensitive, not flagged | -| `/api-keys/*`, `/auth/*` | Already scoped to caller via `require_auth` | - -## Docs changes — `docs/open-source/features/rest-api.mdx` - -Surgical line edits, no restructure: - -1. **Line 134** (per-user API key row): rewrite to - `Programmatic access tied to the single admin user today. Per-user - scoping arrives with the upcoming multi-user invite flow.` -2. **Line 185** (`Per-user keys inherit the creating user's scope`): - delete the sentence; it describes a contract the server does not enforce. -3. **Endpoint reference tables (lines 247-296):** add an `Admin only` marker - to `/configure` (GET, POST), `/reset`, `/entities` (GET, DELETE), and - `/requests`. -4. **Line 267**: keep "additional accounts are provisioned by the existing - admin" but mark `(coming soon)` and remove any implication that today's - per-user keys are already scoped. - -## Tests — `server/tests/` (new) - -`server/` currently has no pytest scaffolding. We add the minimum needed to -guard the regression matrix. - -**New files:** - -- `server/tests/__init__.py` -- `server/tests/conftest.py` — fixtures: - - SQLite in-memory engine + session factory; bind `Base.metadata.create_all`. - - `get_memory_instance` patched to return a `MagicMock` so memory routes - don't need pgvector. - - `client` fixture: FastAPI `TestClient` with the SQLite session injected - via `app.dependency_overrides`. - - `admin_user` fixture: inserts a `User(role="admin")` row and issues a JWT. - - `non_admin_user` fixture: inserts a `User(role="member")` row (direct DB - insert — no public endpoint produces this today) and issues a JWT. - - `admin_api_key_env` and `auth_disabled_env` fixtures: monkey-patch the - auth module to simulate those modes. -- `server/tests/test_admin_gating.py` — parametrized over the 6 admin-gated - routes and the `GET /memories` no-filter branch: - - admin JWT → expected success status (200/204) - - admin per-user key → expected success - - `ADMIN_API_KEY` env → expected success - - `AUTH_DISABLED=true` → expected success - - non-admin JWT → 403 - - no auth → 401 -- `server/tests/test_unchanged_routes.py` — sanity coverage that filtered - memory queries, `POST /memories`, `POST /search`, and `/api-keys/*` behave - identically before/after. - -**Dev dependencies** added to `server/requirements.txt` (or a new -`server/requirements-dev.txt`): - -- `pytest` -- `pytest-asyncio` -- `httpx` (for `TestClient`) - -**CI:** add a `server-ci.yml` workflow (model on `cli-python-ci.yml`) that -runs `pytest server/tests/` on PRs touching `server/`. - -## Out of scope — tracked, not shipped - -A single GitHub issue captures the deferred items. Each is one paragraph in -the issue body, not a separate design doc. - -1. **`owner_id` on memory payloads.** Schema decision (payload key vs first- - class column), backfill story for existing vector stores, SDK - compatibility, write-path injection (`POST /memories`), read-path filter - on every memory route, ownership check on `memory_id` operations. -2. **Multi-user invite flow.** UX (roles, per-org, hierarchies), endpoint - surface (`POST /auth/invite`, `POST /auth/accept`), role transitions. -3. **Rate-limit `POST /generate-instructions`.** Gather usage data first; - adding a limit could regress existing batch callers. -4. **Per-owner `GET /requests` filtering.** Requires `owner_id` on the - `RequestLog` table. - -## Backwards compatibility - -- **No DB migration in this PR.** -- **No SDK changes**; request and response shapes are identical. -- **No environment variable changes.** -- **No breaking change** to existing single-admin deployments — every - supported auth mode keeps working. - -## Acceptance criteria - -1. All five existing deployment modes (single-admin JWT, single-admin - per-user key, `ADMIN_API_KEY` env, `AUTH_DISABLED=true`, plus authenticated - non-admin which cannot exist today) succeed on every route they succeed - on today. -2. The six admin-gated routes return 403 to a synthetic non-admin User - (test-only DB insert). -3. `GET /memories` with no filter returns 403 to a synthetic non-admin; - filtered calls are unchanged. -4. `ADMIN_API_KEY` and `AUTH_DISABLED` escape hatches are covered by - automated tests, not just by code review. -5. `docs/open-source/features/rest-api.mdx` accurately describes the current - per-user-key reality and marks the multi-user promise as "coming soon." -6. CI runs the new test suite on every PR touching `server/`. -7. Follow-up GitHub issue filed and linked from the PR description. - -## Open questions for the implementation plan - -- Test DB: SQLite in-memory vs SQLite file vs containerized Postgres? Default - to in-memory unless `pgvector`-specific code paths are required (they - aren't, given the `MagicMock` strategy). -- Should the new `server-ci.yml` workflow also run `ruff check`? (Likely - yes — match the rest of the repo.) -- Whether to add a quick-start `make test` target in `server/Makefile`.