Repository navigation
feat(auth): count sign-in code email sends per caller - #8022
Conversation
The eligibility check for email sign-in codes now counts each send against three Vercel Firewall rules: the existing per-address rule, a new rule keyed by the caller address, and a new rule sharing one global bucket. Each rule returns 429 SIGNUP-RATE-LIMITED when its allowance is exhausted, and a rule missing from the firewall configuration returns 503 SIGNUP-UNAVAILABLE instead of proceeding. Rule ids are documented in ENVIRONMENT.md for the firewall configuration, and tests cover the keys, response mappings, and missing-rule behavior for the native OTP and magic-link routes.
iscekic
added
drain
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe only new commit reverts the global send bucket, the mobile Assumptions: Files Reviewed (96 files)
Previous Review Summaries (8 snapshots, latest commit e6d1e25)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e6d1e25)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change is three test-only edits plus a lockfile snapshot-key repair; the new OTP test matches the route's intentional fall-through for Assumptions: Files Reviewed (4 files)
Previous review (commit 39e51f6)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change is comment-only plus a lint-driven test-helper narrowing; the previously carried suggestion about a deleted precedent is now resolved, and the new reference ( Assumptions: Files Reviewed (2 files)
Previous review (commit 54cf369)Status: 1 Suggestion Found | Recommendation: Merge Executive SummaryThe incremental change (a lint-driven test-helper rewrite plus the Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Previous review (commit bb4a2bf)Status: 1 Suggestion Found | Recommendation: Merge Executive SummaryThe incremental change (the Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (10 files)
Previous review (commit 2718214)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit wraps the Files Reviewed (1 file)
Previous review (commit d5b36eb)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe single incremental commit only wires the new Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit efce6de)Status: No Issues Found | Recommendation: Merge Executive SummaryI reviewed the incremental changes since Files Reviewed (95 files)
Previous review (commit 63bb4e6)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Reviewed by deepseek-flash-latest · Input: 93.1K · Output: 23.2K · Cached: 1.2M Review guidance: REVIEW.md from base branch |
…ge forwarded header The shared total bucket is now counted only after an address passed eligibility, in the two routes that send the code, so refusals that never send cannot spend it. The per-caller key reads the forwarded header the edge provides, matching the gateway OAuth client limiter. A rule missing from the firewall configuration is reported to Sentry through the same throttled helper used by the other firewall wrappers. Mobile maps the new unavailable code in every locale catalog, and the suites cover the send-time counting, the caller key, and the reporting.
iscekic
left a comment
iscekic
left a comment
There was a problem hiding this comment.
bot: Fixed. The per-caller key now reads x-vercel-forwarded-for, the header Vercel overwrites at the edge, with the same fallback chain as the gateway OAuth client limiter (x-real-ip, then unknown). A test pins the first-hop derivation.
|
bot: Fixed. The per-caller key now reads |
iscekic
added
merge:blocked
merge:blocked:insufficient-e2e
Resolve the eligibility result union, the shared eligibility test file, and the Khmer catalog entry so both sides' changes stay.
The jest-worker resolution and the optional wasm runtime entries were absent from the lockfile, so a frozen install could not build the workspace graph. Adding them keeps the resolved versions unchanged.
…fying objects The helper stringified the fetch mock's url and body, which lint rejects because both can be objects. It now resolves the url from each accepted shape and reads the body only when it is a string.
The terminal-race test stringified a BodyInit, which lint rejects. It reads the body when it is a string, so the assertion still fails if the request carries another body shape.
Take main's ai-gateway test-helper fixes and its lockfile repair.
iscekic
removed
merge:human
…end budget Adds a route case for an address the signup switch refuses: the shared total allowance is enforced before the code is created, answering 429 with no code stored or sent.
|
bot: End-to-end evidence on head e6d1e25, exercised on this VM against the local Postgres and the repository's local email capture.
The per-rule counters and their keys are pinned by the unit suites. The firewall SDK reports every rule as available outside production, so the 429 and 503 branches are not observable on this VM. No screenshot: the change has no visual surface. |
iscekic
added
the
merge:blocked:insufficient-e2e
iscekic
added
merge:human
iscekic
left a comment
iscekic
left a comment
There was a problem hiding this comment.
Request changes: please narrow this PR to the per-caller limit only. Drop the global send cap: one caller could use up the budget and block email sign-in for every user. When a firewall rule is missing, keep the current fail-open behavior with a throttled Sentry report, not a 503. Remove the new unavailable error copy if nothing uses it after this change.
Sign-in code eligibility applies the existing per-address firewall rule and a new caller-keyed rule before a code is issued. The caller key comes from the edge-set forwarded header, then the real-ip header, then a fixed key. An exhausted count returns the existing 429 SIGNUP-RATE-LIMITED shape. A rule that is not present in the project's firewall configuration is skipped and reported to Sentry at most once a minute. The module doc and ENVIRONMENT.md name the rule ids for the firewall configuration review. Tests cover both keys, the header fallbacks, the 429 mapping, the not-configured path, and the ordering before the domain rules.
iscekic
removed
the
merge:human
|
bot: Narrowed as requested.
Re-verified on this VM: production-mode run of the route against local Postgres with a local HTTPS edge serving the rate-limit API. An available count returns |
iscekic
changed the title
iscekic
added
the
needs:prod-action
|
bot: Prod prerequisite before merge: create the Vercel Firewall rate-limit rule |
eshurakov
left a comment
eshurakov
left a comment
There was a problem hiding this comment.
Shallow review: no critical issues found.
iscekic
removed
the
needs:prod-action
|
bot: Production prerequisite complete. The |
Evidence
Evidence: demonstrated — production-mode local run of
POST /api/auth/native/otpon this VM against local Postgres, with a local HTTPS edge in front of the handler that answers the Vercel rate-limit API contract the firewall client calls. The route returned the documented status for each configured count, and admitted requests created one code row and one suppressed email dispatch each.Local run
Setup: the route handler module runs with
NODE_ENV=production, the mode in which@vercel/firewallperforms its rate-limit lookup, against local Postgres. A local HTTPS edge answers/.well-known/vercel/rate-limit-api/<rule>with204(counted),429(count exhausted) or404(rule absent), keyed onx-vercel-rate-limit-key, and supplies the caller address headers.magic-link-emailcall and onesign-in-code-email-ipcall per request.200 {"success":true,"challengeId":…}, one code row, one email dispatch.429 {"success":false,"error":"SIGNUP-RATE-LIMITED"}with no code row and no dispatch.x-vercel-forwarded-forvalue; thex-real-ipfallback is covered by the unit suite.Not observed: the web magic-link route needs a Turnstile token in this environment, so only the native OTP route was driven; both routes call the shared
checkEmailSignInEligibility. The report for a rule that is not configured is covered by the unit suite, since the local run has no Sentry DSN.Change
Sign-in code email volume is counted against two Vercel Firewall rules in
apps/web/src/lib/auth/email-signin-eligibility.ts:magic-link-email— the existing per-normalized-address rule, same 429 shape as before.sign-in-code-email-ip— new rule keyed by the caller address taken from the edge-set forwarded header (x-vercel-forwarded-for), falling back tox-real-ipthenunknown.Both run inside
checkEmailSignInEligibilitybefore the domain checks, soPOST /api/auth/native/otpandPOST /api/auth/magic-linkshare the counters. An exhausted count returns the existing429 SIGNUP-RATE-LIMITEDshape; no new response shape is introduced.A rule that is not present in the project's firewall configuration is skipped, and the gap is reported to Sentry at most once a minute. Both rule ids are documented in
ENVIRONMENT.md(Security & Auth) for the firewall configuration review.The native OTP route keeps its opaque
200for addresses the domain rules refuse and its existing ordering; only the eligibility module and its tests changed.Tests
apps/web/src/lib/auth/email-signin-eligibility.test.ts— call order and keys, caller key from the edge header plus thex-real-ipfallback,429mapping, rule-not-configured path with one report per interval, and ordering before the domain rules.apps/web/src/app/api/auth/native/otp/route.test.ts— unchanged frommain; the route passes eligibility outcomes through as before.apps/web/src/app/api/auth/magic-link/route.test.ts— the key assertions now select calls by rule id, since each request makes two firewall calls.apps/web/src/lib/observability/rest-routes-auth.test.ts— unchanged frommain.pnpm --filter web run typecheck,oxlintand the suites above pass locally; CI on the head commit is green.Production prerequisites
sign-in-code-email-ipwith a chosen threshold, and confirmmagic-link-emailexists. Both rule ids are listed inENVIRONMENT.mdfor the firewall configuration review. Without the new rule the per-caller counter is skipped and each instance reports the missing rule to Sentry once a minute.Merge notes
Proxy answers
The owner's proxy gave these answers in the owner's place.
Question: Fail-closed: When a rate-limit rule is missing from the firewall configuration (checkRateLimit error 'not-found'), which rules should fail closed with 503?
Answer: All rules
Confidence: 0.65 · Precedent: Fix the root cause and the class of the issue: session 00eb2a1d3e4b, "if anything repeats the same mistake over and over again, fix the class". The brief also says to return 503 whenever a rate-limit rule is unavailable.
Question: 503 shape: What response should the 503 fail-closed case return on the native OTP route?
Answer: SIGNUP-UNAVAILABLE (Recommended)
Confidence: 0.70 · Precedent: Prefer existing code to hand-rolled additions: session df491946927a, "we will try and use libraries wherever we can". This reuses an existing error type that clients already map and keeps the response shapes unchanged.
Question: Scope: Where should the new limits apply? The web magic-link route calls the same checkEmailSignInEligibility function.
Answer: Shared (Recommended)
Confidence: 0.75 · Precedent: Fix the class on every affected surface: session 75f8833fe8c1, "fix this for all shared services model pickers on all surfaces (web, app)".
Question: main is currently red: every CI job fails at
pnpm install --frozen-lockfilewith a broken lockfile entry (jest-workerreferences a package version main's lockfile no longer defines). Two open PRs fix exactly that line (fix(repo): restore the green main checks #8510, fix(deps): resolve jest-worker structured-clone in the lockfile #8511, both green checks, awaiting an owner merge). This PR's new merge commit inherits main's broken lockfile, so its CI cannot go green until one of those lands. How should this PR proceed?Answer: Carry the same one-line lockfile repair here (Recommended)
Confidence: 0.60 · Precedent: Autonomy: never accept blocked or failed as an end state (session 2f71af3a787e, "a work item must converge"); fix gaps even if unrelated (session a247f3c19b1e, "fix the error even though it's unrelated")
Question: Review state: GitHub shows Kilo Code Review completed successfully on exact head 6bbe940 with summary “Code review completed successfully,” and no new comments or review object; pr_loop_state still says “reviewed head no.” How should this be resolved?
Answer: Treat exact-head success as complete
Confidence: 0.82 · Precedent: Expect kilobot to post reviews in varied shapes: session df491946927a, “kilobot is inconsistent in how it posts a review”; human ready means kilobot recommends merge or every comment was addressed (session 640cb4aa808a)
Answers 1 and 2 describe the earlier fail-closed design; the review on 2026-10-23 replaced it with a skip-and-report path for a rule that is not configured, and answer 3 still holds: the counters live in the shared eligibility check.