Repository navigation
fix(#1245, #1237): the Edge Functions count and decide in one call (stage A3) - #1291
Merged
Merged
Conversation
…tage A3) contact-message and create-lead checked the limit, then recorded the attempt: two round trips, so every request in a concurrent burst passed the check before any record landed. create-lead recorded only after a successful insert, and ignored that call's error. Both now make one consume_rate_limit call (atomic since A1, live) before the side effect: before the mail send, and before the leads insert. An insert that then fails still spends the attempt; legitimate clients never hit that path, and it caps database work the insert path used to do without limit. The answer goes through a new import-free helper, _shared/limiter-verdict.ts. The handlers used to test `limit && limit.allowed === false`, so a null answer, or one without `allowed`, let the request through: a limiter failing open on exactly the answers nobody expects. Now anything but an explicit allowance is "cannot check" and answers 503. Its unit test goes red under the old rule. tests/unit/edge-function-limiter.test.ts parses each function's deploy closure with the TypeScript AST, so comments naming the old pair can neither satisfy nor break it. It pins exactly one consume call, before the side effect, through the verdict, and no string naming the old pair anywhere in supabase/functions. It runs its own mutations on every run: rename, delete-but-comment, call twice, indirection through a constant, drop the verdict, move after the insert, move after the send. A CONTROL adds a comment naming the old pair and stays green. The same file was red against the pre-A3 handlers. RLS: consume_rate_limit in the exact shape the handlers send (booking_lead, the IP as p_ip_address), and a non-address x-forwarded-for value erroring, so the handler fails closed rather than proceeding. Comments that described the old pair as these functions' limiter are corrected, all outside function bodies: the drift gate compares bodies byte for byte, and every body is unchanged here (checked). The deploy planner's refusal no longer claims the verify_jwt table does not exist; it refuses pending the owner decision in #1188. deno check: all 18 functions clean. Still open under #1237: the IP is the first x-forwarded-for entry, which a client may control. This limits honest clients until that is measured. Needs an Edge Function deploy after merge; merging is not deploying. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage A3 of #1245, and the first half of #1237: the contact-form and booking Edge Functions now spend their rate limit in one atomic call, before the side effect. Neither issue is finished — A4 and #1237's header question remain — so this refers to both.
What was wrong
Each function checked the limit, then recorded the attempt: two round trips. In a concurrent burst, every request passed the check before any record landed, so the ceiling held only against callers polite enough to wait their turn. An RLS CONTROL test in the repo demonstrates the burst getting through.
create-leadalso recorded only after a successful insert, and ignored that call's error.Both handlers tested
limit && limit.allowed === false. So a null answer, or an answer withoutallowed, let the request through — the limiter failed open on exactly the answers nobody expects.The change
One call, first. Both functions make one
consume_rate_limitcall, which has been live since A1 and counts and decides in a single statement. It comes before the mail send, and before theleadsinsert. An insert that then fails still spends the attempt. Legitimate clients never take that path (record-lead.tsmints a fresh id and never retries), and it caps database work that path used to do without limit.Fail closed.
_shared/limiter-verdict.tsis import-free, so a unit test can load it. It treats anything but an explicit allowance as "cannot check", which becomes a 503. Its test goes red under the old rule.AST guard.
tests/unit/edge-function-limiter.test.tsparses each function's deploy closure with the TypeScript AST, so comments can neither satisfy nor break it. It pins:supabase/functions.It runs its own mutations on every run. Each of these goes red:
A CONTROL comment naming the old pair stays green. The whole file was red against the pre-A3 handlers.
RLS. One case calls
consume_rate_limitin the exact shape the handlers send:booking_lead, with the IP asp_ip_address. Another confirms a non-addressx-forwarded-forvalue is an error, so the handler fails closed.Comments. Comments describing the old pair are corrected, all outside function bodies. The drift gate compares bodies byte for byte, and every body in the migration is unchanged here (checked mechanically: the non-comment text is identical to main). The deploy planner's refusal no longer claims the
verify_jwttable doesn't exist.Local gates:
deno check: all 18 functions clean;test:scripts1,074/1,074;Deploying it
Merging is not deploying. After merge I deploy both functions to the production project. The project ref is asserted equal to the repo's
BACKEND_EPOCH_PROJECT_REF; a stale note had named another project, and that note is now corrected. Each function goes up with:verify_jwt: false, taken from the declared table;create-leadfirst.Before deploying, I save each function's live metadata and bundle as the rollback reference.
Verification:
verify_jwtis present and false, and the bundle carries the consume call. The same matcher, run on the saved pre-deploy bundle, must give the opposite answer.create-leadPOST reusing an existing lead's id returns 409 and writes no lead. The new code will already have spent an attempt; the old code would not have.X-Forwarded-For, and the identifier on the limiter row shows which IP the function actually used.contact-messagegets no live probe: a failed probe would send real mail. The AST guard covers its wiring. Probe rows are deleted by exact key.Refs #1245, #1237
🤖 Generated with Claude Code