fix(keys): block restricted API keys from minting or managing other keys

[hasitpbhatt]

Orca-Code-Review — push 2

Severity	Count	Δ vs previous push
P0	0	0
P1	0	0
P2	3	+2
P3	1	+1

✅ no blocking findings

Problem

Any valid API key could call POST /v1/keys and receive a new key with model_allowlist=NULL / budget_limit_cents=NULL. Since key management had no scope check, a key deliberately restricted by the operator (allowlist enforced at chat time) could mint itself an unrestricted sibling in one request — a complete bypass of its restrictions. The same unrestricted power applied to listing and revoking any key.

Full details in #70.

Fix

Key management (GET /v1/keys, POST /v1/keys, DELETE /v1/keys/{id}) now requires an unrestricted caller: any KeyContext with a non-None model_allowlist or budget_limit_cents gets 403 forbidden.

Design note: this grants nothing to anyone. Unrestricted keys already hold the maximum privilege the single-workspace edition exposes (same trust level as PUT /v1/providers/*); the change only closes the escalation path — a restricted key can neither mint nor modify anything anymore.

Tests

New tests/integration/test_keys_authz.py (6 cases):

• allowlist-restricted and budget-restricted keys get 403 on create/list/revoke
• create attempts persist nothing (row-count assertion against the DB)
• restricted revoke leaves the target untouched
• unrestricted (seeded root) key retains full list/create/revoke flow

Verification

• pytest tests/integration/test_keys_authz.py → 6 passed
• full suite → 312 passed
• ruff check app packages tests clean

Closes #70

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟡 1 P2.

_{Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.}

[orcacode-review]

🟡 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.

[orcacode-review]

🐳 OrcaCode Review

Found 4 issues in this PR: 🟡 3 P2 · ⚪ 1 P3.

Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.

---

app/routes/quality.py (line 180): 🟡 P2 Gate POST /v1/quality/refresh with require_unrestricted like the other quality writes

This commit's stated policy is "restricted API keys (model_allowlist or budget_limit_cents) must not be able to perform management writes" and it applies require_unrestricted to PUT/DELETE /v1/quality/overrides/, PUT /v1/providers/, DELETE /v1/providers/* and PUT /v1/routing. The one remaining management write on the quality surface was missed: POST /v1/quality/refresh (quality.py:158) is still reachable by a restricted key. With force_refresh=True it bypasses the 1h AA cache (packages/litellm_adapter/quality_index.py:692-748), hits the Artificial Analysis API every call (metered 1000 req/day quota — a cost on paid tiers), persists a fresh snapshot to the shared DB, and calls broadcast_metrics_cache_invalidation() (global, all workers). A holder of a budgeted/allowlist key (the untrusted class this change is meant to confine) can therefore repeatedly: exhaust the operator's AA quota, force global resolver-cache invalidations, and write shared DB state — exactly the class of shared-operator-state mutation the commit gates everywhere else. The commit message claims "gate provider/routing/quality writes", so refresh should have received the same require_unrestricted(kc) call (before the fetch), and the new test file should cover it.

_{Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.}

[orcacode-review]

⚪ 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.

[orcacode-review]

🟡 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.

[orcacode-review]

🟡 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.