Skip to content

feat(security): bound sign-in code guessing with per-actor rate limits - #752

Open
Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/auth-rate-limit
Open

Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/auth-rate-limit

Conversation

@Ayush7614

Copy link
Copy Markdown
Contributor

What

The routes that verify something guessable had no bound on attempts at all. POST /api/sign-in-requests/:id/submit and POST /:id/use-saved check a typed password and, worse, the short numeric second-factor code — one request per guess, forever, with no counter, lockout, or delay anywhere in the service or the routes (verified by grep: the only 429s in server/src are vendor-side handling plus one pending-count cap in the desktop handoff).

This adds the missing bound:

  1. New server/src/rate-limit.ts: a per-process sliding-window limiter as Hono middleware. Over-budget callers get 429 + Retry-After with one fixed sentence; budgets key on the signed-in actor (spoof-proof on these authed routes), unauthenticated callers share one anonymous bucket, expired buckets are pruned, and the key map itself is capped so distinct-key rotation cannot grow memory.
  2. Wiring in server/src/passwords/routes.ts: submit + use-saved share one code-attempt budget (60 per 10 minutes per person — far past mistyping, nowhere near a six-digit space), POST / creation gets its own hourly budget (30/hour). Reads and state transitions (finish/cancel/take-over, vault GET/DELETE) are untouched. The 429 path returns directly and never passes through the route error handler, so the no-secret-echo guarantee is preserved (asserted in tests).
  3. Docs: short paragraph under "Human control and secrets" in docs/architecture.md, including the honest per-process scope (N replicas ≈ N× budget).

createSignInRoutes takes an optional 4th arg, so existing call sites compile unchanged.

Verification (all run locally, all green)

  • New: 8 tests across server/tests/rate-limit.test.ts (cap, 429 + Retry-After, window reset, countdown, per-key isolation, pruning) and server/tests/signin-submit-rate-limit.test.ts (shared submit/use-saved budget, no secret echo in 429, separate creation budget, reads unaffected, per-actor isolation, window reopen) — all pass, all use a fake clock, no waiting.
  • No regressions: existing server/tests/passwords-sign-in.test.ts 17/17 pass with production defaults.
  • bunx biome lint --error-on-warnings: clean. bun run --filter server typecheck: exit 0.
  • Note: passwords-store.integration.test.ts needs a live TEST_DATABASE_URL Postgres and fails identically on clean main — pre-existing environmental, unrelated.
  • No dependencies added or changed.

Original work for this repo: no open issue or PR covers server-side rate limiting (checked open issues/PRs and the codebase before starting); branch is fresh off current upstream/main with nothing else on it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant