fix(#784): the contact endpoint is anonymous and had no ceiling - #791
Merged
Conversation
`contact-message` sends mail on an unauthenticated POST. The recipient is fixed server-side, so it was never the #353 open-relay defect — the worst case was spam into our own inbox rather than a stranger's — but "bounded to our mailbox" is not a limit, and the function shipped saying so in its own header comment. Now 5 submissions per 15 minutes per IP, by REUSING the limiter the auth forms already use (`check_rate_limit` / `record_failed_attempt` over `rate_limit_attempts`): SECURITY DEFINER, row-locked, sliding window, already exercised by the auth specs. A second hand-rolled limiter would be a second thing to get wrong. THE FIRST DEPLOY FAILED COMPLETELY AND THAT IS WHY THIS WORKS. All seven probe requests returned 503 — the fail-closed path — because `rate_limit_attempts` carries a CHECK constraining `attempt_type` to sign_in/sign_up/password_reset, so 'contact_form' raised a 23514. Nothing in the code review would have caught that; it surfaced only by driving the deployed endpoint. The constraint is now widened, mirrored in the monolithic file and applied to production from a FILE. Widening a CHECK permits strictly more values and rewrites no rows. Verified against the DEPLOYED function, not asserted: 5 submissions from one IP -> all 200, sent 6th and 7th -> 429 with a human message identifier recorded -> the REAL client IP, not a caller-supplied header A SECOND WRONG CONCLUSION, avoided by checking: a probe with a different `x-forwarded-for` also got 429, which reads as "the limit is global, not per-IP". It is not. Supabase's edge sets the first XFF entry itself, so the injected header never controlled the identifier and both probes genuinely came from one IP — confirmed by reading the stored identifier. Isolation was then proven at the RPC level, where the input IS controllable: IP A exhausted, IP B still allowed with remaining=5. Trusting the surface reading would have meant "fixing" a limiter that was already correct. Deliberate choices: - The attempt is counted BEFORE the send. If the send then fails the attempt is still spent — the alternative lets a caller hammer a failing provider without limit, which is when a ceiling matters most. - Every path that cannot verify the limit returns 503 rather than sending. An advisory limit on an anonymous endpoint is no limit. - `x-forwarded-for` is read FIRST-entry. Taking the last would let a caller prepend their own header and rotate identifiers at will. - `record_failed_attempt` is the limiter's increment primitive, named for its original auth use. A contact submission is not a failure; the name is wrong for this caller and the behaviour is right. Renaming means a production migration for cosmetics, so it is documented instead. Probe rows were deleted afterwards — leaving test litter in a production table is the #612 defect, and writing this one while leaking would be poor form. Closes #784 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 18, 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.
The gap
contact-messagesends mail on an unauthenticated POST. The recipient is fixedserver-side, so this was never the #353 open-relay defect — the worst case was spam into
our own inbox rather than a stranger's — but "bounded to our own mailbox" is not a limit,
and the function shipped saying so in its own header comment.
Now 5 submissions per 15 minutes per IP, by reusing the limiter the auth forms already
use (
check_rate_limit/record_failed_attemptoverrate_limit_attempts):SECURITY DEFINER, row-locked, sliding window, already exercised by the auth specs. A second
hand-rolled limiter would be a second thing to get wrong.
The first deploy failed completely, and that is why this works
All seven probe requests returned 503 — the fail-closed path — because
rate_limit_attemptscarries a CHECK constrainingattempt_typetosign_in/sign_up/password_reset, socontact_formraised a 23514.Nothing in review would have caught that. It surfaced only by driving the deployed
endpoint. The constraint is now widened, mirrored in the monolithic migration and applied
to production from a FILE. Widening a CHECK permits strictly more values and rewrites no
rows.
Verified against the DEPLOYED function
A second wrong conclusion, avoided by checking
A probe with a different
x-forwarded-foralso got 429, which reads as "the limit isglobal, not per-IP". It is not. Supabase's edge sets the first XFF entry itself, so the
injected header never controlled the identifier and both probes genuinely came from one IP
— confirmed by reading the stored identifier (
97.81.13.31).Isolation was then proven at the RPC level, where the input is controllable: IP A
exhausted → IP B still allowed with
remaining: 5. Trusting the surface reading wouldhave meant "fixing" a limiter that was already correct.
Deliberate choices
spent — the alternative lets a caller hammer a failing provider without limit, which is
when a ceiling matters most.
limit on an anonymous endpoint is no limit.
x-forwarded-foris read first-entry. Taking the last would let a caller prependtheir own header and rotate identifiers at will.
record_failed_attemptis the limiter's increment primitive, named for its originalauth use. A contact submission is not a failure; the name is wrong for this caller and
the behaviour is right. Renaming means a production migration for cosmetics, so it is
documented instead.
Probe rows were deleted afterwards — leaving test litter in a production table is the #612
defect, and writing this one while leaking would be poor form.
Closes #784