fix(keys): scope /v1/keys list and revoke queries to the caller's workspace

[hasitpbhatt]

Orca-Code-Review — push 1

Severity	Count
P0	0
P1	1
P2	4
P3	1

❌ 1 finding blocks merge

Problem

GET /v1/keys and DELETE /v1/keys/{id} did not filter on ApiKey.workspace_id — any authenticated key could enumerate every key in the database (names, prefixes, usage timestamps) and revoke any of them, including keys belonging to other workspaces. Notably, create_key in the same file did scope its write, making the asymmetry easy to miss.

Full details in #74.

Fix

Added ApiKey.workspace_id == kc.workspace_id to both the list query and the revoke query in app/routes/keys.py. Foreign-workspace ids now resolve to 404 without leaking existence.

Tests

Two new cases in tests/integration/test_keys_authz.py:

• test_list_keys_hides_other_workspaces — a key from a second workspace never appears in workspace A's listing
• test_revoke_rejects_other_workspaces_key — cross-workspace revoke returns 404 and leaves the row active

Verification

• pytest tests/integration/test_keys_authz.py → 8 passed
• Full suite: 323 passed
• ruff check app packages tests: clean

Stacked on #73 → depends on #71/#73 merge order only for clean diffs.

Closes #74

[orcacode-review]

🐳 OrcaCode Review

Found 6 issues in this PR: 🟠 1 P1 · 🟡 4 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/analytics.py (line 43): 🟡 P2 Extend the new workspace scoping to analytics (and other read paths) that still leak across workspaces

Commit 9240492 fixes cross-workspace data access for keys (list_keys/revoke_key now filter ApiKey.workspace_id == kc.workspace_id, with tests that seed a ws-other workspace and assert the default workspace cannot see/revoke its keys). The same workspace boundary is not applied to the sibling read paths that consume RequestLog/config: /v1/analytics/recent, /spend, /latency, /savings (and /v1/unreachable) select RequestLog with only is_deleted == 0, no workspace_id filter; /v1/routing GET/PUT hardcodes DEFAULT_WORKSPACE_ID; /v1/providers GET/PUT/DELETE have no workspace scoping at all. A key minted in ws-other (the exact scenario this change's own tests construct) can still read the default workspace's full request history, spend, model/provider usage, and can modify the default workspace's routing config and provider credentials. If the workspace scoping applied to keys is the intended threat model, these routes should be scoped by kc.workspace_id in the same change; otherwise the IDOR fix is incomplete.

---

app/routes/keys.py (line 77): ⚪ P3 Return model_allowlist/budget_limit_cents from list_keys so restrictions stay auditable

create_key (modified in the same commit) now persists and returns model_allowlist and budget_limit_cents, but list_keys — the sibling in the same module, also modified in this commit — omits both fields from its per-key dict. After a restricted key is created, no API surface (and the SPA, which renders /v1/keys) can show which keys are allowlisted/budgeted; the operator's only record is the one-time create response. The list is where a consumer would verify that a budgeted child key actually carries its budget, and it silently presents keys as indistinguishable. Add the two fields to the list_keys row dict.

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

[orcacode-review]

🟠 P1 Make budget enforcement atomic per key — the read-check-write is a TOCTOU race that lets concurrent requests overshoot the cap

The new budget enforcement is a plain check-then-act: get_lifetime_spend_microcents runs SELECT SUM(cost_microcents) (packages/auth/spend.py) and rejects only if the already-committed total is over the cap. The request's own billable cost row is inserted and committed only AFTER the upstream call completes — blocking path: db.add(log); await db.commit() in the finally at line 733-735; streaming path: _finalize() at stream end via a separate session (line 560-562). There is no serialization anywhere: no SELECT ... FOR UPDATE on the key row, no conditional UPDATE with rowcount check, no per-key lock, and the engine sets no isolation level (Postgres default READ COMMITTED, SQLite default). So N concurrent requests for the same key can all read spend below the cap, all be served, and all commit their cost rows — the key's realized spend exceeds budget_limit_cents by up to N × per-request cost. Because the check-to-record window spans the entire upstream call/stream (seconds to minutes), any budgeted key under concurrent load near its cap overshoots; the commit's own claim ("lifetime cap … so an exhausted key costs the operator nothing") is false under concurrency and the operator is billed past the configured cap. Fix: make the accounting atomic — e.g. add a per-key spent_microcents column and commit spend with a conditional UPDATE (UPDATE api_keys SET spent_microcents = spent_microcents + :cost WHERE id = :key_id AND spent_microcents + :cost <= budget_limit_microcents), treating rowcount == 0 as exhausted and writing the request-log row in the same transaction; at minimum, re-check the sum after the upstream call and before committing the billable row under a key-row lock.

[orcacode-review]

🟡 P2 Budget enforcement fails open when the request-log spend write fails

The new lifetime-cap enforcement's only accounting source is requests_log rows summed by get_lifetime_spend_microcents. Every site that writes those rows is best-effort and swallows commit failures while still returning success to the client: the blocking path's finally (db.add(log); try: await db.commit() except Exception as commit_err: logger.warning("request_log_commit_failed", ...)), the cache-hit path (same pattern), and the streaming _finalize (both branches swallow, and both callers wrap _finalize() in except Exception: pass). When the commit fails — e.g. the connection drops between the upstream call and the commit, or the table is briefly locked/write-blocked while reads keep working — the request is served with 200 and its cost is permanently invisible to the budget check. The cap is then silently undercounted and an exhausted key keeps being served, so the operator's stated limit is exceeded with only a warning log as a signal. The enforcement is only as reliable as a best-effort write that explicitly ignores failure; it should either fail closed when the accounting row cannot be persisted (reject/retry) or use an accounting source whose writes are not swallowed.

[orcacode-review]

🟡 P2 Count 499/503 stream rows (which carry real upstream cost) toward the budget

The budget excludes every row with status_code >= 400. But on the streaming path, a client disconnect (status 499) or a mid-stream upstream failure (status 503) still records the aggregated usage from the chunks already delivered (agg_usage populated from the stream), so those rows carry real, provider-billed cost — tokens were consumed upstream before the stream ended. Those rows are systematically excluded from get_lifetime_spend_microcents, so actual operator spend is undercounted: a leaked key can burn real money through streams that never complete (disconnect right after the last chunk, or repeated failing streams) without ever consuming its budget. The "billable = status < 400" assumption does not match what the provider bills.

[orcacode-review]

🟡 P2 Bound budget_limit_cents to the Integer column range, or a large value 500s on Postgres

Pydantic accepts any int > 0 (gt=0), but ApiKey.budget_limit_cents is a SQLAlchemy Integer column (32-bit). On the supported Postgres backend (DATABASE_URL=postgresql+asyncpg), POST /v1/keys with budget_limit_cents > 2,147,483,647 cents (~$21.5M) passes validation, then db.commit() raises an IntegrityError that the generic Exception handler turns into a 500 "server_error" instead of a 422, and the key creation fails. No upper bound is enforced anywhere between the client-supplied value and the storage engine's limit; the SQLite default silently accepts it, making the failure backend-dependent.

[hasitpbhatt]

Closing as part of PR-hygiene cleanup. This was part of an interdependent stack (#71 -> #75 -> #77 -> #81 -> #83 -> #85) rather than an independent branch from latest main. The unique work will be re-raised as clean, independent PRs branched directly from main, brought to distinguished engineering quality, and passed through a rigorous review gate before re-opening. Keys scoping + restricted-key blocking is already superseded by #89; encryption fail-closed / log-redaction / unhandled-exception work has largely landed via #87/#88; remaining unique parts (cache v2 key space, latency logging, async exception handler) will be re-raised independently. Reopen if you want to keep this branch.