Skip to content

fix(web): send sign-in email only when the firewall rules answer - #9730

Open
iscekic wants to merge 1 commit into
mainfrom
drain/sign-in-email-send-budget
Open

iscekic wants to merge 1 commit into
mainfrom
drain/sign-in-email-send-budget

Conversation

@iscekic

@iscekic iscekic commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — the native email sign-in route ran as its real module in NODE_ENV=production, with the installed @vercel/firewall@1.2.1 client performing its HTTPS rate-limit lookup against a local stand-in that implements the documented answer protocol, and with real reads and writes on the local dev Postgres, at head 49e7d1928.

Observed contract at that head:

Limiter answer for the two send-budget rules Endpoint response Sign-in code rows written
both rules permit 200 {"success":true,"challengeId":"…"} 1
a rule answers blocked 429 {"success":false,"error":"SIGNUP-RATE-LIMITED"} 0
a rule answers limited 429 {"success":false,"error":"SIGNUP-RATE-LIMITED"} 0
a rule does not answer 429 {"success":false,"error":"SIGNUP-RATE-LIMITED"} 0

Every row went through the same route module. The stand-in recorded each firewall check with its rule id and the per-address or per-caller key prefix (magic-link-email:…, sign-in-code-email-ip:…), so each response is attributable to the send-budget check. Rows created by the permitting scenario were deleted afterwards.

Method and limitations: a route-module run, not a next build / next start server or a browser session. Mail delivery was suppressed by the automated-test switch and NeverBounce was unconfigured, so no external request was made. No Sentry DSN exists on this VM, so report emission is proven by unit assertions rather than by a delivered event. The production firewall configuration cannot be read from this VM.

Unit and static checks:

  • Jest src/lib/auth, src/app/api/auth/native/otp, src/app/api/auth/magic-link, src/lib/observability/rest-routes-auth.test.ts: 32 suites, 531 tests passed. The send-budget suite (20 tests) pins both rules permitting, a rule that answers without permitting, a rule that does not answer, an exhausted budget, key derivation, and one report per rule per interval.
  • pnpm --filter web typecheck: exit 0.
  • oxlint --config .oxlintrc.json on the changed files: clean. oxfmt --check on the changed files: clean.

Change

apps/web/src/lib/auth/email-signin-eligibility.ts runs both send-budget checks through the shared checkFirewallRateLimit helper and refuses the send unless each answers that the caller may proceed. magic-link-email counts per normalized address and sign-in-code-email-ip counts per caller address; the per-address check runs first so a refusal spends no further allowance. The shared helper reports an unconfigured or unevaluable rule once per rule per minute, replacing the local reporter. The response contract is unchanged: 429 SIGNUP-RATE-LIMITED with the existing body, so no client-visible error code or status changes.

ENVIRONMENT.md now states that both rules are required configuration and that the send runs only when both answer.

Production prerequisites

  • The Vercel project's firewall configuration must contain both rate-limit rules before merge: magic-link-email, counted per normalized address, and sign-in-code-email-ip, counted per caller address. Each endpoint send requires both answers, so both must exist.

Proxy answers

The owner's proxy gave these answers in the owner's place.

  1. Question: Fail closed: Should the sign-in email send require both firewall rules to answer 'allowed' (refuse when a rule is not configured or the limiter cannot answer)?
    Answer: Yes, refuse the send
    Confidence: 0.50 · Precedent: Conflicts in part with 'If a protective guard's configuration is missing, keep serving requests and report the gap through an alert' (fix(ai-gateway): refuse inference the account cap cannot enforce #8222 closed). The work item names later owner decisions on the analogous native sign-in rules (PRs fix(web): require an answering address rate limit on native sign-in #9409, fix(web): issue native admission challenges only on a permitting limiter answer #9617) that fail closed. Email sends cost money and are an abuse path, so refuse, keep the Sentry alert, and make the rules a production prerequisite.
  2. Question: Response: What response should an unanswerable limiter produce on the sign-in endpoints?
    Answer: Keep 429 SIGNUP-RATE-LIMITED
    Confidence: 0.85 · Precedent: 'Close a PR that breaks an existing client contract, such as Kilo CLI or VS Code sign-in' (refactor(device-auth): poll sign-in status only through the token endpoint #8038 closed)
  3. Question: Implementation: How should the limiter be called?
    Answer: Reuse checkFirewallRateLimit
    Confidence: 0.80 · Precedent: 'Prefer existing libraries to hand-rolled code' and 'Replace old paths instead of keeping them alongside' ("alongside? why not instead?")
  4. Question: Prerequisite: Record the two firewall rules as a merge-blocking production prerequisite (needs:prod-action) and update ENVIRONMENT.md?
    Answer: Yes, list both rules
    Confidence: 0.85 · Precedent: 'Label a PR that needs a production action before merge … with the action-needed label from the work session's own report … do the action yourself when you can and then merge' (fabebdc394c9, Vercel rules set up, then merge)

Sign-in code and magic-link sends now run only when every applicable
firewall rule answers that the caller may proceed. The two send-budget
checks go through the shared checkFirewallRateLimit helper, so a rule
that is not configured or that cannot be evaluated is refused and
reported through the shared per-rule reporter. The refusals keep the
existing 429 SIGNUP-RATE-LIMITED response.
@iscekic iscekic added drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening labels Nov 4, 2026
@kilo-code-bot

kilo-code-bot Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

This three-file change routes both sign-in email send-budget checks through the existing checkFirewallRateLimit helper and refuses the send unless each rule answers allowed; the refactored logic, ordering, response contract, and tests hold up, with no defects found in changed code.

Assumptions: [INFERENCE] The Vercel project's firewall configuration cannot be read from this environment, so the merge prerequisite the PR states (both magic-link-email and sign-in-code-email-ip rules exist) is unverified here; because the send is now fail-closed, a missing rule would return 429 SIGNUP-RATE-LIMITED for all email sign-in rather than a skip. [INFERENCE] checkFirewallRateLimit (unchanged) does not catch a rejected checkRateLimit promise, so a limiter transport throw would propagate instead of producing the documented refusal; this predates the diff and was not raised as a finding.

Files Reviewed (3 files)
  • ENVIRONMENT.md
  • apps/web/src/lib/auth/email-signin-eligibility.ts
  • apps/web/src/lib/auth/email-signin-eligibility.test.ts

Reviewed by deepseek-flash-latest · Input: 54.8K · Output: 11.6K · Cached: 739.6K

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added merge:blocked:insufficient-e2e Verification does not cover the change merge:human Substantive functional risk requires human review. needs:prod-action Prod prerequisite (env var, secret, firewall rule, config) needed before merge and removed merge:blocked:insufficient-e2e Verification does not cover the change labels Nov 4, 2026

@eshurakov eshurakov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot - Fail-closed behavior is correct, but confirm both the magic-link-email and sign-in-code-email-ip Vercel Firewall rules are live in the production project before merge (needs:prod-action); without them, every email sign-in and magic-link send returns 429.

@iscekic

iscekic commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

bot: On the approving review remark — confirm both the magic-link-email and sign-in-code-email-ip Vercel Firewall rules are live in the production project before merge (needs:prod-action):

The dependency is recorded as the merge prerequisite rather than resolved here. The ## Production prerequisites section names both rules with the counting key each one needs, the pull request carries needs:prod-action, and ENVIRONMENT.md documents both as required configuration. This work environment has no Vercel CLI and no Vercel token, so the production firewall configuration cannot be read or changed from it; confirming the two rules stays with the owner before merge.

No code change follows from the remark: the refusal path is the owner's recorded decision and the client-visible contract is unchanged (429 SIGNUP-RATE-LIMITED, same body). The transport-throw note in the same review body is also pre-existing and is not a send path: a rejected limiter call surfaces as an error, so no message leaves in that case either.

@iscekic

iscekic commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Update on the approving review remark — both rules answer in production.

Method (at head 49e7d19, from the production project's Sentry data, not from the deployment):

  • The send path's alarm for a rule that is not configured is the Sentry message Firewall rate limit '<id>' is not configured, emitted at most once per rule per minute. It has been on main and in the production deployment since 2026-10-27 (feat(auth): count sign-in code email sends per caller #8022).
  • The project holds no such message for magic-link-email or sign-in-code-email-ip over the last 90 days.
  • The message path itself is proven in this project: Sentry carries messages of exactly that shape for other firewall rule ids, and those are closed.
  • POST /api/auth/native/otp runs both checks before any other sign-in work, and the mobile flow that calls it is live in production, so an absent message means each rule answered rather than that the check never ran.

What I could not verify from here, and why needs:prod-action stays: the limits and key expressions inside the Vercel project. This environment has no Vercel CLI and no Vercel token, and the SDK's rate-limit endpoint answers 404 to a caller that is not the deployment, so an outside probe cannot tell a configured rule from a missing one. I confirmed that by probing rule ids of both kinds and getting the same answer. The pre-merge step is therefore a read of the two rules in the project's firewall configuration.

No code change follows from the remark: the refusal path is the owner's recorded decision and the client-visible contract is unchanged (429 SIGNUP-RATE-LIMITED, same body).

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

drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening merge:human Substantive functional risk requires human review. needs:prod-action Prod prerequisite (env var, secret, firewall rule, config) needed before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants