[Fix] Make API authorization default-deny with declared route policies - #133
Conversation
|
Re-reviewed the new commit addressing the earlier feedback. All 3 prior findings are resolved and no new issues found. See task The follow-up commit cleanly addresses every earlier point. The Teams auth-resume rate limit now keys on the state token so concurrent legitimate account links no longer share one bucket; the counter increment and TTL are now a single atomic Lua call; and the MCP surfaces emit a JSON-RPC error envelope matching
|
Every registered API route must now be covered by exactly one declared route policy (public, webhook, user, task-token, authenticated, admin). A central gate rejects requests before any handler runs when the path is not covered by a policy (default-deny) or when the request does not satisfy the declared policy. A route inventory test enumerates all registered routes and fails when any route is unclassified, so future routes cannot ship without a policy. Webhook entry points and the Teams auth resume endpoint also get central per-client rate limits that fail open on Redis errors.
…jections - Key the Teams auth resume rate limit on the SHA-256 of the body state token so concurrent legitimate users arriving from the web app's shared egress never collide; keep a raised, documented client-keyed global ceiling (300/min) as a volumetric backstop. - Replace the non-atomic INCR + conditional EXPIRE with a single Lua script so a partial failure can never leave an orphaned counter without a TTL. - Emit JSON-RPC error envelopes (matching handlers/mcp/proxy-utils.ts) for central rejections on the /api/mcp and /api/mcp-routing surfaces so Streamable HTTP clients see a consistent body shape.
e2c2b53 to
3eb1473
Compare
|
Addressed all three review findings in 3eb1473, rebased onto latest develop (56172b1). 1. MEDIUM — resume rate limit keyed on shared client bucket. The 2. LOW — non-atomic INCR + conditional EXPIRE. Replaced with a single Lua script ( 3. LOW — plain 401 body on MCP surfaces. Implemented rather than acknowledged: rules can declare Rebase and validation. Rebased cleanly onto develop at 56172b1; develop added no new route registrations since the branch point, and the route inventory test still passes against the current surface (it would fail on any unclassified addition). Full |
Summary
Finding 13 of the pre-release architecture audit (P1). Previously,
tokenAuthMiddlewareattempted to attach an auth context but always continued to the handler — including after an invalid credential — so every handler was individually responsible for authorization, and a new route added without its own checks was silently exposed.This PR makes authorization default-deny and central:
apps/api/src/route-policies.ts— a declarative manifest where every registered route is covered by exactly one policy rule (ordered, first match wins).apps/api/src/middleware/routePolicyMiddleware.ts— a central gate registered after token auth and before all routes. A request whose path matches no declared rule is rejected with 404 before any handler can run; a request that does not satisfy its route's declared policy is rejected with 401/403 before the handler runs.apps/api/src/__tests__/route-policy-inventory.test.ts— enumerates every route registered on the Hono app (including wildcard middleware mounts) and fails when any route is not covered by a policy rule, when a rule is dead/shadowed, or when the infrastructure-middleware exemption list contains anything but wildcard middleware. Future routes cannot ship unclassified: the test fails in CI and the route is unreachable at runtime until classified.POST /api/webhooks/teams/auth/resume: 30 req/min per client (single-use state token in the body is a brute-force target)Route inventory by policy class
public — no credentials required (token auth still attaches a context when present so health responses can include authenticated diagnostics):
GET /(LB deep health check)GET /health/api,GET /health/liveness,GET /health/controllerGET /.well-known/openid-configuration,GET /api/oidc/jwks(sandbox OIDC discovery; these also keep their existing token-auth bypass)webhook — signature/secret-authenticated inside the handler (HMAC for GitHub/GitLab/Gitea/Slack/Linear, shared secret for ADO/Telegram, Bot Framework JWT for Teams):
POST /api/webhooks/github,/gitlab,/gitea,/ado,/slack,/linear,/teams,/telegramPOST /api/webhooks/teams/auth/resume(see judgment calls)task-token — task run tokens only; user tokens rejected 403 centrally:
POST /api/artifacts,/api/artifacts/plan,/api/artifacts/visual-proof,/api/artifacts/:id/upload_complete;GET /api/artifacts/:id/urlGET /api/tasks/:taskId/artifacts,GET /api/tasks/:taskId/artifacts/:pathauthenticated — user auth token or task run token required; finer-grained token-type and run-scoping checks remain in handlers/procedures:
/api/mcp/*— slack/tasks/environments MCP routes (viamcpAuthMiddleware) plus all integration MCP endpoints (asana, grafana, linear, snowflake, vercel, and the generic integration proxies); most integration proxies additionally require run tokens in-handler/api/mcp-routing/*— roomote/linear/github router-facing MCPGET /api/task-runs/:id/logs— run tokens scoped to their own run in the handler; user tokens may stream any run/trpc/*— every tRPC procedure requires auth;userOnlyProcedure/runScopedper-procedure checks unchangedadmin —
/adminmount, HTTP basic auth (registered outside development, unchanged)user — defined in the policy vocabulary, but no current HTTP mount is exclusively user-authenticated at the boundary (user-only enforcement lives in tRPC's
userOnlyProcedure). Declared so future user-only routes have a class to land in.Judgment calls
POST /api/webhooks/teams/auth/resumeis not signature-verified; it authenticates via a single-use state token in the body (called server-to-server by the web app after account linking). Classifiedwebhookwith a strict 30/min rate limit rather than inventing a one-off class.authenticatedclass: the audit brief named user-authenticated and task-token classes, but several surfaces (tRPC, MCP, task-run logs) legitimately accept both token types with per-endpoint scoping. Those are classifiedauthenticated; tightening any of them to a single token type would break existing worker/web flows./admin: basic-auth middleware exists but no handler is mounted there today; kept classified so the mount stays covered if a dashboard returns.{ "error": "authentication_required" }instead of handler-specific messages). Statuses are unchanged (401/403), which is what clients key on.Preserved invariants
oidc-middlewaretests pass unmodified).Rate-limiting follow-up
Central limits now cover the public webhook entry points and the Teams auth resume endpoint — the only auth-adjacent unauthenticated endpoints on this API surface. Auth/setup/OAuth initiation endpoints live in
apps/web(Next.js routes / tRPC mutations), not inapps/api, so rate-limiting those is an explicit follow-up in the web app.Testing
pnpm --filter @roomote/api exec vitest run(via dotenvx .env.test): 121 files / 817 tests pass, including the new inventory and enforcement suitespnpm lint:fastandpnpm check-types:fastpasspnpm knipreports zero findings forapps/api; the repo-level knip run fails identically on cleanorigin/developin my worktree environment (skipped pnpm build scripts), i.e. pre-existing/environmental, not introduced here