From e1d146ab545605ef9fa4a0a03875c9c62a12b1fe Mon Sep 17 00:00:00 2001 From: Hasit Bhatt Date: Wed, 26 Aug 2026 02:20:14 -0700 Subject: [PATCH 1/4] fix(keys): scope key list/revoke to caller workspace and block restricted keys from managing keys --- app/routes/keys.py | 53 +++++++- tests/integration/test_keys_authz.py | 196 +++++++++++++++++++++++++++ 2 files changed, 244 insertions(+), 5 deletions(-) create mode 100644 tests/integration/test_keys_authz.py diff --git a/app/routes/keys.py b/app/routes/keys.py index 2d1ca86..380b845 100644 --- a/app/routes/keys.py +++ b/app/routes/keys.py @@ -5,7 +5,7 @@ from datetime import datetime, timezone from fastapi import APIRouter, Depends, HTTPException, Response -from pydantic import BaseModel +from pydantic import BaseModel, Field from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -20,16 +20,46 @@ class CreateKey(BaseModel): name: str + # Optional restrictions for child keys. Only reachable by unrestricted + # callers (require_unrestricted above), so a restricted key can never + # mint a sibling with looser limits than its own — it can't mint at all. + model_allowlist: list[str] | None = None + budget_limit_cents: int | None = Field(default=None, gt=0) + + +def require_unrestricted(kc: KeyContext) -> None: + """Key management is reserved for unrestricted keys. + + A key that carries any restriction (`model_allowlist` or + `budget_limit_cents`) must not be able to mint, list, or revoke other + keys — otherwise it could create a sibling with no restrictions and + trivially bypass its own allowlist/budget. Unrestricted keys already + hold the maximum privilege this single-workspace edition exposes + (same trust level as PUT /v1/providers/*), so denying restricted keys + here grants nothing to anyone; it only closes the escalation path. + """ + if kc.model_allowlist is not None or kc.budget_limit_cents is not None: + raise HTTPException( + status_code=403, + detail=( + "Restricted API keys cannot manage keys. " + "Use an unrestricted key." + ), + ) @router.get("") async def list_keys( - _kc: KeyContext = Depends(get_key_context), + kc: KeyContext = Depends(get_key_context), db: AsyncSession = Depends(get_db), ) -> dict: + require_unrestricted(kc) rows = ( await db.execute( - select(ApiKey).where(ApiKey.is_deleted == 0).order_by(ApiKey.created_at) + select(ApiKey).where( + ApiKey.workspace_id == kc.workspace_id, + ApiKey.is_deleted == 0, + ).order_by(ApiKey.created_at) ) ).scalars().all() return { @@ -54,12 +84,15 @@ async def create_key( kc: KeyContext = Depends(get_key_context), db: AsyncSession = Depends(get_db), ) -> dict: + require_unrestricted(kc) full_key, key_hash, key_prefix = generate_api_key() row = ApiKey( workspace_id=kc.workspace_id, name=body.name, key_hash=key_hash, key_prefix=key_prefix, + model_allowlist=body.model_allowlist, + budget_limit_cents=body.budget_limit_cents, ) db.add(row) await db.commit() @@ -70,18 +103,28 @@ async def create_key( "name": row.name, "key_prefix": row.key_prefix, "api_key": full_key, # plaintext shown ONCE + "model_allowlist": row.model_allowlist, + "budget_limit_cents": row.budget_limit_cents, } @router.delete("/{key_id}", status_code=204) async def revoke_key( key_id: str, - _kc: KeyContext = Depends(get_key_context), + kc: KeyContext = Depends(get_key_context), db: AsyncSession = Depends(get_db), ) -> Response: + require_unrestricted(kc) row = ( await db.execute( - select(ApiKey).where(ApiKey.id == key_id, ApiKey.is_deleted == 0) + select(ApiKey).where( + ApiKey.id == key_id, + # Workspace scoping: without this, any key could revoke any + # other workspace's keys (the write path has always been + # scoped; the read/delete paths were not). + ApiKey.workspace_id == kc.workspace_id, + ApiKey.is_deleted == 0, + ) ) ).scalar_one_or_none() if row is None: diff --git a/tests/integration/test_keys_authz.py b/tests/integration/test_keys_authz.py new file mode 100644 index 0000000..8df84eb --- /dev/null +++ b/tests/integration/test_keys_authz.py @@ -0,0 +1,196 @@ +"""Key-management authorization tests. + +A restricted key (model_allowlist or budget_limit_cents set) must never be +able to mint, list, or revoke API keys — otherwise it could mint an +unrestricted sibling and bypass its own restrictions entirely. +See issue: restricted-key privilege escalation via POST /v1/keys. +""" + +import pytest + + +@pytest.fixture +async def seeded_keys(db_session): + """Seed the workspace root key plus one restricted and one budgeted key. + + Returns (root_full_key, restricted_full_key, budgeted_full_key). + """ + from app.seed import seed_initial_state + from packages.auth.hashing import generate_api_key + from packages.db.models.api_key import ApiKey + + seed = await seed_initial_state(db_session) + assert seed.api_key is not None + + def _make(**kwargs) -> str: + full_key, key_hash, key_prefix = generate_api_key() + row = ApiKey( + workspace_id="default", + name=kwargs.pop("name", "test"), + key_hash=key_hash, + key_prefix=key_prefix, + **kwargs, + ) + db_session.add(row) + return full_key + + # flush once so all rows land before any request reads them + restricted = _make(name="restricted", model_allowlist=["gpt-4o-mini"]) + budgeted = _make(name="budgeted", budget_limit_cents=500) + await db_session.commit() + return seed.api_key, restricted, budgeted + + +@pytest.fixture +async def keys_app(db_session, monkeypatch): + """FastAPI app with auth middleware and only the /v1/keys routes mounted.""" + monkeypatch.setenv("DATABASE_URL", str(db_session.bind.url)) + from fastapi import FastAPI + + from app.middleware.auth import AuthMiddleware + from packages.db import session as session_mod + + class _PassthroughFactory: + async def __aenter__(self): + return db_session + + async def __aexit__(self, *exc): + return False # propagate, don't close — fixture owns the session + + monkeypatch.setattr(session_mod, "_session_factory", lambda: _PassthroughFactory()) + + from app.routes.keys import router as keys_router + + app = FastAPI() + app.add_middleware(AuthMiddleware) + app.include_router(keys_router) + return app + + +async def _client(app): + from httpx import ASGITransport, AsyncClient + + return AsyncClient(transport=ASGITransport(app=app), base_url="http://t") + + +@pytest.mark.parametrize("which", [1, 2], ids=["allowlist-restricted", "budget-restricted"]) +async def test_restricted_key_cannot_create_keys(keys_app, seeded_keys, db_session, which): + keys, restricted, budgeted = seeded_keys + caller = (restricted, budgeted)[which - 1] + async with await _client(keys_app) as c: + r = await c.post( + "/v1/keys", + json={"name": "escalated"}, + headers={"Authorization": f"Bearer {caller}"}, + ) + assert r.status_code == 403 + # The escalation must not have persisted anything. + from sqlalchemy import func, select + + from packages.db.models.api_key import ApiKey + + count = ( + await db_session.execute(select(func.count()).select_from(ApiKey)) + ).scalar_one() + assert count == 3 # root + restricted + budgeted, nothing new + + +@pytest.mark.parametrize("which", [1, 2], ids=["allowlist-restricted", "budget-restricted"]) +async def test_restricted_key_cannot_list_keys(keys_app, seeded_keys, which): + keys, restricted, budgeted = seeded_keys + caller = (restricted, budgeted)[which - 1] + async with await _client(keys_app) as c: + r = await c.get("/v1/keys", headers={"Authorization": f"Bearer {caller}"}) + assert r.status_code == 403 + + +async def test_restricted_key_cannot_revoke_keys(keys_app, seeded_keys, db_session): + _, restricted, _budgeted = seeded_keys + from sqlalchemy import select + + from packages.db.models.api_key import ApiKey + + rows = (await db_session.execute(select(ApiKey))).scalars().all() + target_id = next(r.id for r in rows if r.name == "default") + async with await _client(keys_app) as c: + r = await c.delete( + f"/v1/keys/{target_id}", + headers={"Authorization": f"Bearer {restricted}"}, + ) + assert r.status_code == 403 + target = next(r for r in rows if r.name == "default") + assert target.is_active # untouched + + +async def test_unrestricted_key_retains_full_management(keys_app, seeded_keys): + root, _restricted, _budgeted = seeded_keys + h = {"Authorization": f"Bearer {root}"} + async with await _client(keys_app) as c: + listed = await c.get("/v1/keys", headers=h) + assert listed.status_code == 200 + + created = await c.post("/v1/keys", json={"name": "child"}, headers=h) + assert created.status_code == 201 + child_id = created.json()["id"] + + revoked = await c.delete(f"/v1/keys/{child_id}", headers=h) + assert revoked.status_code == 204 + + +# ── Workspace scoping (IDOR regression tests) ──────────────────────────── + + +async def _make_foreign_workspace_key(db_session) -> tuple[str, str]: + """A key belonging to a different workspace; returns (id, name).""" + from packages.auth.hashing import generate_api_key + from packages.db.models.api_key import ApiKey + from packages.db.models.workspace import Workspace + + db_session.add(Workspace(id="ws-other", name="Other", slug="other")) + await db_session.flush() + + full_key, key_hash, key_prefix = generate_api_key() + row = ApiKey( + workspace_id="ws-other", + name="foreign-key", + key_hash=key_hash, + key_prefix=key_prefix, + ) + db_session.add(row) + await db_session.commit() + return row.id, full_key + + +async def test_list_keys_hides_other_workspaces(keys_app, seeded_keys, db_session): + root, *_ = seeded_keys + foreign_id, _foreign_key = await _make_foreign_workspace_key(db_session) + + async with await _client(keys_app) as c: + r = await c.get( + "/v1/keys", headers={"Authorization": f"Bearer {root}"} + ) + + assert r.status_code == 200 + listed_ids = {k["id"] for k in r.json()["keys"]} + assert foreign_id not in listed_ids + + +async def test_revoke_rejects_other_workspaces_key(keys_app, seeded_keys, db_session): + from sqlalchemy import select + + from packages.db.models.api_key import ApiKey + + root, *_ = seeded_keys + foreign_id, _foreign_key = await _make_foreign_workspace_key(db_session) + + async with await _client(keys_app) as c: + r = await c.delete( + f"/v1/keys/{foreign_id}", + headers={"Authorization": f"Bearer {root}"}, + ) + + assert r.status_code == 404 + row = ( + await db_session.execute(select(ApiKey).where(ApiKey.id == foreign_id)) + ).scalar_one() + assert row.is_active # untouched From 757c2772948de648150bba11b071c40c5d056980 Mon Sep 17 00:00:00 2001 From: Hasit Bhatt Date: Wed, 26 Aug 2026 02:24:15 -0700 Subject: [PATCH 2/4] test(keys): add provisioning test for restricted/budgeted keys --- tests/integration/test_keys_authz.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/integration/test_keys_authz.py b/tests/integration/test_keys_authz.py index 8df84eb..b0a7965 100644 --- a/tests/integration/test_keys_authz.py +++ b/tests/integration/test_keys_authz.py @@ -137,6 +137,31 @@ async def test_unrestricted_key_retains_full_management(keys_app, seeded_keys): assert revoked.status_code == 204 +async def test_create_key_accepts_restrictions(keys_app, seeded_keys, db_session): + root, *_ = seeded_keys + h = {"Authorization": f"Bearer {root}"} + async with await _client(keys_app) as c: + r = await c.post( + "/v1/keys", + json={"name": "team-a", "model_allowlist": ["gpt-4o-mini"], "budget_limit_cents": 500}, + headers=h, + ) + assert r.status_code == 201, r.text + body = r.json() + assert body["model_allowlist"] == ["gpt-4o-mini"] + assert body["budget_limit_cents"] == 500 + + from sqlalchemy import select + + from packages.db.models.api_key import ApiKey + + row = ( + await db_session.execute(select(ApiKey).where(ApiKey.id == body["id"])) + ).scalar_one() + assert row.budget_limit_cents == 500 + assert row.model_allowlist == ["gpt-4o-mini"] + + # ── Workspace scoping (IDOR regression tests) ──────────────────────────── From c6b1097d02530d777c5757bf6b81c5e6f2702e92 Mon Sep 17 00:00:00 2001 From: Hasit Bhatt Date: Thu, 27 Aug 2026 19:07:00 -0700 Subject: [PATCH 3/4] fix(keys): gate privileged management behind require_unrestricted Introduce packages.auth.guards with a synchronous require_unrestricted dependency (and is_restricted helper) so restricted keys (model allowlist or budget cap) cannot mint/rotate credentials, rewrite routing, or override quality scores. Wire it into /v1/keys, /v1/providers, /v1/routing and /v1/quality management routes; scope key list/revoke to the caller workspace. The dependency is synchronous because it performs no I/O and must raise even when called inline, not return an un-awaited coroutine that fails open. --- app/routes/keys.py | 26 ++++---------------- app/routes/providers.py | 3 +++ app/routes/quality.py | 3 +++ app/routes/routing.py | 2 ++ packages/auth/guards.py | 40 +++++++++++++++++++++++++++++++ tests/unit/test_startup_guards.py | 3 ++- 6 files changed, 54 insertions(+), 23 deletions(-) create mode 100644 packages/auth/guards.py diff --git a/app/routes/keys.py b/app/routes/keys.py index 380b845..4c1f9d3 100644 --- a/app/routes/keys.py +++ b/app/routes/keys.py @@ -11,6 +11,7 @@ from app._time_util import iso_utc from app.deps import get_db, get_key_context +from packages.auth.guards import require_unrestricted from packages.auth.hashing import generate_api_key from packages.auth.types import KeyContext from packages.db.models.api_key import ApiKey @@ -21,33 +22,12 @@ class CreateKey(BaseModel): name: str # Optional restrictions for child keys. Only reachable by unrestricted - # callers (require_unrestricted above), so a restricted key can never + # callers (require_unrestricted below), so a restricted key can never # mint a sibling with looser limits than its own — it can't mint at all. model_allowlist: list[str] | None = None budget_limit_cents: int | None = Field(default=None, gt=0) -def require_unrestricted(kc: KeyContext) -> None: - """Key management is reserved for unrestricted keys. - - A key that carries any restriction (`model_allowlist` or - `budget_limit_cents`) must not be able to mint, list, or revoke other - keys — otherwise it could create a sibling with no restrictions and - trivially bypass its own allowlist/budget. Unrestricted keys already - hold the maximum privilege this single-workspace edition exposes - (same trust level as PUT /v1/providers/*), so denying restricted keys - here grants nothing to anyone; it only closes the escalation path. - """ - if kc.model_allowlist is not None or kc.budget_limit_cents is not None: - raise HTTPException( - status_code=403, - detail=( - "Restricted API keys cannot manage keys. " - "Use an unrestricted key." - ), - ) - - @router.get("") async def list_keys( kc: KeyContext = Depends(get_key_context), @@ -69,6 +49,8 @@ async def list_keys( "name": r.name, "key_prefix": r.key_prefix, "is_active": r.is_active, + "model_allowlist": r.model_allowlist, + "budget_limit_cents": r.budget_limit_cents, "last_used_at": iso_utc(r.last_used_at), "revoked_at": iso_utc(r.revoked_at), "created_at": iso_utc(r.created_at), diff --git a/app/routes/providers.py b/app/routes/providers.py index b0b3607..b928045 100644 --- a/app/routes/providers.py +++ b/app/routes/providers.py @@ -47,6 +47,7 @@ from app.deps import get_db, get_key_context from app.router_cache import usable_providers_from_db from packages.auth.encryption import encrypt_credential +from packages.auth.guards import require_unrestricted from packages.auth.types import KeyContext from packages.db.models.provider_key import ProviderKey @@ -141,6 +142,7 @@ async def set_provider_key( provider: str, body: SetProviderKey, _kc: KeyContext = Depends(get_key_context), + _restricted: None = Depends(require_unrestricted), db: AsyncSession = Depends(get_db), ) -> dict: if not body.api_key.strip(): @@ -198,6 +200,7 @@ async def set_provider_key( async def delete_provider_key( provider: str, _kc: KeyContext = Depends(get_key_context), + _restricted: None = Depends(require_unrestricted), db: AsyncSession = Depends(get_db), ) -> Response: """Hard-delete the DB row for this provider. After this, runtime diff --git a/app/routes/quality.py b/app/routes/quality.py index 37ec691..d24726a 100644 --- a/app/routes/quality.py +++ b/app/routes/quality.py @@ -38,6 +38,7 @@ load_overrides, resolve_model_metrics, ) +from packages.auth.guards import require_unrestricted from packages.auth.types import KeyContext from packages.db.models.quality_score_override import QualityScoreOverride from packages.litellm_adapter.catalog import CATALOG, CATALOG_BY_ID @@ -157,6 +158,7 @@ async def quality_status( @router.post("/refresh") async def quality_refresh( kc: KeyContext = Depends(get_key_context), + _restricted: None = Depends(require_unrestricted), db: AsyncSession = Depends(get_db), ) -> dict: """Force a fresh fetch of the AA Intelligence Index. @@ -389,6 +391,7 @@ async def upsert_override( model_id: str, body: OverrideBody = Body(...), kc: KeyContext = Depends(get_key_context), + _restricted: None = Depends(require_unrestricted), db: AsyncSession = Depends(get_db), ) -> dict: """Create or update an override. Idempotent on (workspace, model_id). diff --git a/app/routes/routing.py b/app/routes/routing.py index b2a669e..67bc80d 100644 --- a/app/routes/routing.py +++ b/app/routes/routing.py @@ -10,6 +10,7 @@ from app import cache_invalidation_bus from app.deps import get_db, get_key_context from app.seed import DEFAULT_WORKSPACE_ID +from packages.auth.guards import require_unrestricted from packages.auth.types import KeyContext from packages.db.models.routing_config import RoutingConfig @@ -55,6 +56,7 @@ async def get_routing( async def update_routing( body: UpdateRouting, _kc: KeyContext = Depends(get_key_context), + _restricted: None = Depends(require_unrestricted), db: AsyncSession = Depends(get_db), ) -> dict: row = ( diff --git a/packages/auth/guards.py b/packages/auth/guards.py new file mode 100644 index 0000000..05e92b7 --- /dev/null +++ b/packages/auth/guards.py @@ -0,0 +1,40 @@ +"""Authorization guards for privileged key-management operations. + +A *restricted* key is one that carries any limitation — a ``model_allowlist`` +or a ``budget_limit_cents`` cap. Restricted keys are issued as child keys with +reduced privilege; letting them mint/rotate provider credentials, rewrite +routing, or override quality scores would let them escalate to the full +privilege of an unrestricted key. Only unrestricted keys may perform those +operations, so the escalation path is closed everywhere, not just on +``/v1/keys``. +""" + +from __future__ import annotations + +from fastapi import Depends, HTTPException + +from app.deps import get_key_context +from packages.auth.types import KeyContext + + +def is_restricted(kc: KeyContext) -> bool: + """True if the key carries any usage restriction.""" + return kc.model_allowlist is not None or kc.budget_limit_cents is not None + + +def require_unrestricted(kc: KeyContext = Depends(get_key_context)) -> None: + """FastAPI dependency: reject restricted keys from management endpoints. + + Usable both as ``Depends(require_unrestricted)`` on a route and as a direct + ``require_unrestricted(kc)`` call. Synchronous on purpose — it performs no + I/O, only a privilege check and a raise — so it works identically whether + FastAPI awaits it as a dependency or a route calls it inline. + """ + if is_restricted(kc): + raise HTTPException( + status_code=403, + detail=( + "Restricted API keys cannot perform management operations. " + "Use an unrestricted key." + ), + ) diff --git a/tests/unit/test_startup_guards.py b/tests/unit/test_startup_guards.py index 736d144..0c8bd38 100644 --- a/tests/unit/test_startup_guards.py +++ b/tests/unit/test_startup_guards.py @@ -128,9 +128,10 @@ async def _boom(*_a, **_kw): async def test_guard_allows_missing_table_on_fresh_sqlite(tmp_sqlite_url): """A fresh DB pre-migration where provider_keys doesn't exist counts as zero rows for sqlite (inspected via engine, not string matching).""" + from sqlalchemy.ext.asyncio import async_sessionmaker + from packages.db.engine import build_engine from packages.db.guards import assert_credential_encryption_ready - from sqlalchemy.ext.asyncio import async_sessionmaker engine = build_engine(tmp_sqlite_url) factory = async_sessionmaker(engine, expire_on_commit=False) From 46a79ea0426bd4d49b9d96b613a8c1ab1e900385 Mon Sep 17 00:00:00 2001 From: Hasit Bhatt Date: Thu, 27 Aug 2026 19:18:53 -0700 Subject: [PATCH 4/4] fix(keys): gate DELETE /v1/quality/overrides behind require_unrestricted A restricted key could delete quality overrides (e.g. lift a quality block), an escalation the branch is meant to close everywhere. Gate the delete route the same way the refresh/upsert routes already are. --- app/routes/quality.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/routes/quality.py b/app/routes/quality.py index d24726a..4778373 100644 --- a/app/routes/quality.py +++ b/app/routes/quality.py @@ -464,6 +464,7 @@ async def _do_merge() -> QualityScoreOverride: async def delete_override( model_id: str, kc: KeyContext = Depends(get_key_context), + _restricted: None = Depends(require_unrestricted), db: AsyncSession = Depends(get_db), ) -> None: """Remove an override. Returns 204 even if the override didn't exist —