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 new file mode 100644 index 000000000..e8ee312cc --- /dev/null +++ b/docs/superpowers/specs/2026-05-13-rest-api-authz-fix-design.md @@ -0,0 +1,274 @@ +# 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`.