Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions app/routes/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,33 @@ class CreateKey(BaseModel):
name: str


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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 P2 Keep GET /v1/keys returning 200 for restricted keys, or update the dashboard auth probe

The shipped dashboard (design/app.js) authenticates every pasted API key by calling GET /v1/keys: checkAuth() (line 206-213) does await api("/v1/keys") and treats any non-2xx as "key invalid"; it runs on login submit (line 1411) and on every page boot (line 1453). Before this commit, GET /v1/keys returned 200 for any valid key, which is the contract that probe relies on. This commit adds require_unrestricted(kc) to list_keys, so a valid key that carries model_allowlist or budget_limit_cents now gets 403 from the probe. The dashboard then shows "That key didn't work. Double-check the prefix sk-orca-…" (auth.key_invalid), wipes the key from localStorage, and shows the login gate — even though the dashboard tabs for overview/providers/routing/analytics/quality/hosted involve no key management and restricted keys are still permitted to use them at the API level. A whole class of valid credentials is presented as invalid and the entire dashboard becomes unusable with them. The existing equivalent (pre-change behavior of this endpoint) did authenticate restricted keys; the new version does not, and nothing was updated on the dashboard side. Fix: have the probe use an endpoint that only checks key validity (e.g. /v1/models or /health-with-auth), or exempt an existence-only check from require_unrestricted, or update checkAuth to tolerate 403 for restricted keys.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 P2 Don't use list_keys as the dashboard auth canary for restricted keys (or keep list_keys readable and gate only mutations)

GET /v1/keys is now gated with require_unrestricted, and the only consumer of that endpoint is the dashboard's auth canary: design/app.js checkAuth() calls api("/v1/keys") and treats ANY non-2xx as a bad key — the 403 detail ("Restricted API keys cannot manage keys") is swallowed, the auth gate shows "Invalid API key", and the stored key is wiped from localStorage. Before this change, GET /v1/keys returned 200 for every valid key, so an operator could open the dashboard with a restricted key and use all tabs. Nothing in the auth gate, the README, or the API documents that the dashboard requires an unrestricted key — the change assumes it, but nothing enforces or communicates it. A valid restricted key now yields "invalid key" and locks the operator out of the entire dashboard (including the read-only analytics/providers tabs that the gate is not meant to protect), with no explanation.

rows = (
await db.execute(
select(ApiKey).where(ApiKey.is_deleted == 0).order_by(ApiKey.created_at)
Expand All @@ -39,6 +61,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),
Expand All @@ -54,6 +78,7 @@ 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,
Expand All @@ -76,9 +101,10 @@ async def create_key(
@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)
Expand Down
6 changes: 6 additions & 0 deletions app/routes/providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
from app.config import get_settings
from app.deps import get_db, get_key_context
from app.router_cache import usable_providers_from_db
from app.routes.keys import require_unrestricted
from packages.auth.encryption import encrypt_credential
from packages.auth.types import KeyContext
from packages.db.models.provider_key import ProviderKey
Expand Down Expand Up @@ -143,6 +144,10 @@ async def set_provider_key(
_kc: KeyContext = Depends(get_key_context),
db: AsyncSession = Depends(get_db),
) -> dict:
# A restricted key (model allowlist or budget cap) must not be able to
# overwrite operator credentials or delete providers — that would let it
# escape its restrictions and redirect the whole workspace's traffic.
require_unrestricted(_kc)
if not body.api_key.strip():
raise HTTPException(status_code=422, detail="api_key cannot be empty")

Expand Down Expand Up @@ -200,6 +205,7 @@ async def delete_provider_key(
_kc: KeyContext = Depends(get_key_context),
db: AsyncSession = Depends(get_db),
) -> Response:
require_unrestricted(_kc)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ P3 Place require_unrestricted after the docstring so the endpoint keeps its docstring/OpenAPI description

In delete_provider_key (providers.py:208), upsert_override (quality.py:395) and delete_override (quality.py:468) the new require_unrestricted(...) call was inserted BEFORE the function docstring, so the docstring is now a bare no-op string expression instead of the function's docstring: the endpoint's __doc__ becomes None. FastAPI builds each operation's OpenAPI description from the endpoint docstring, so /openapi.json and /docs silently lose the descriptions of these three endpoints — a consumer-visible change to the API interface that the pre-change code (docstring as first statement) did not have. The old versions attached the docstring to the function; the new versions do not.

"""Hard-delete the DB row for this provider. After this, runtime
resolution falls back to the env value (if `.env` has one) or
treats the provider as unconfigured.
Expand Down
3 changes: 3 additions & 0 deletions app/routes/quality.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
)
from app.cache_invalidation_bus import broadcast_metrics_cache_invalidation
from app.deps import get_db, get_key_context
from app.routes.keys import require_unrestricted

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 P2 Move from app.routes.keys import require_unrestricted below the app.quality_scores import so ruff isort passes

The repo enforces import sorting in writing: pyproject.toml [tool.ruff.lint] select = ["E","W","F","I","B"] and ci.yml runs ruff check . with continue-on-error: false. The new line from app.routes.keys import require_unrestricted was inserted between from app.deps import ... (line 35) and from app.quality_scores import (...) (line 37). isort/ruff sorts module strings alphabetically: "app.quality_scores" < "app.routes.keys" ("quality" < "routes"), so app.quality_scores must come first. The other two files that got the same import (providers.py:49, routing.py:12) are correctly placed (after app.router_cache / before app.seed), but quality.py's block is now out of order and ruff raises I001, failing CI on every push/PR touching app/. Fix: move the app.routes.keys import to after the app.quality_scores import block.

from app.quality_scores import (
fetch_aa_index,
load_overrides,
Expand Down Expand Up @@ -391,6 +392,7 @@ async def upsert_override(
kc: KeyContext = Depends(get_key_context),
db: AsyncSession = Depends(get_db),
) -> dict:
require_unrestricted(kc)
"""Create or update an override. Idempotent on (workspace, model_id).

`session.merge()` is select-then-write under the hood, so two
Expand Down Expand Up @@ -463,6 +465,7 @@ async def delete_override(
kc: KeyContext = Depends(get_key_context),
db: AsyncSession = Depends(get_db),
) -> None:
require_unrestricted(kc)
"""Remove an override. Returns 204 even if the override didn't exist —
the operator's intent ('no override on this model') is satisfied either
way, no need to surface the distinction."""
Expand Down
2 changes: 2 additions & 0 deletions app/routes/routing.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

from app import cache_invalidation_bus
from app.deps import get_db, get_key_context
from app.routes.keys import require_unrestricted
from app.seed import DEFAULT_WORKSPACE_ID
from packages.auth.types import KeyContext
from packages.db.models.routing_config import RoutingConfig
Expand Down Expand Up @@ -57,6 +58,7 @@ async def update_routing(
_kc: KeyContext = Depends(get_key_context),
db: AsyncSession = Depends(get_db),
) -> dict:
require_unrestricted(_kc)
row = (
await db.execute(
select(RoutingConfig).where(
Expand Down
205 changes: 205 additions & 0 deletions tests/integration/test_keys_authz.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
"""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


@pytest.fixture
async def mgmt_app(db_session, monkeypatch):
"""App mounting the privilege-sensitive write routes alongside keys."""
monkeypatch.setenv("DATABASE_URL", str(db_session.bind.url))
from fastapi import FastAPI

from app.middleware.auth import AuthMiddleware
from app.routes.keys import router as keys_router
from app.routes.providers import router as providers_router
from app.routes.quality import router as quality_router
from app.routes.routing import router as routing_router
from packages.db import session as session_mod

class _PassthroughFactory:
async def __aenter__(self):
return db_session

async def __aexit__(self, *exc):
return False

monkeypatch.setattr(session_mod, "_session_factory", lambda: _PassthroughFactory())

app = FastAPI()
app.add_middleware(AuthMiddleware)
app.include_router(keys_router)
app.include_router(providers_router)
app.include_router(routing_router)
app.include_router(quality_router)
return app


@pytest.mark.parametrize("which", [1, 2], ids=["allowlist-restricted", "budget-restricted"])
async def test_restricted_key_cannot_write_provider_credentials(
mgmt_app, seeded_keys, which
):
_keys, restricted, budgeted = seeded_keys
caller = (restricted, budgeted)[which - 1]
h = {"Authorization": f"Bearer {caller}"}
async with await _client(mgmt_app) as c:
r = await c.put(
"/v1/providers/openai",
json={"api_key": "sk-test-123"},
headers=h,
)
assert r.status_code == 403


async def test_restricted_key_cannot_write_routing(mgmt_app, seeded_keys):
_keys, restricted, _budgeted = seeded_keys
h = {"Authorization": f"Bearer {restricted}"}
async with await _client(mgmt_app) as c:
r = await c.put("/v1/routing", json={"strategy": "balanced"}, headers=h)
assert r.status_code == 403


async def test_unrestricted_key_can_write_provider_and_routing(mgmt_app, seeded_keys):
root, _restricted, _budgeted = seeded_keys
h = {"Authorization": f"Bearer {root}"}
async with await _client(mgmt_app) as c:
# Gate only — we assert the restricted-key block doesn't apply to root.
p = await c.put(
"/v1/providers/openai", json={"api_key": "sk-test-123"}, headers=h
)
assert p.status_code != 403
r = await c.put("/v1/routing", json={"strategy": "balanced"}, headers=h)
assert r.status_code != 403