Skip to content

[#966] Airdrop endpoint security: rate limiting + cache headers#974

Merged
realproject7 merged 3 commits intomainfrom
task/966-airdrop-security
Apr 23, 2026
Merged

[#966] Airdrop endpoint security: rate limiting + cache headers#974
realproject7 merged 3 commits intomainfrom
task/966-airdrop-security

Conversation

@realproject7
Copy link
Copy Markdown
Owner

Summary

  • POST rate limiting (5/min per IP) on checkin, referral-code, register-referral endpoints
    • DB-based via new rate_limits table — serverless-safe (no in-memory state)
    • Auto-purge trigger removes entries older than 5 minutes
    • Shared lib/rate-limit.ts utility with checkRateLimit() and getClientIp()
  • GET cache headers on status (60s), leaderboard (30s), points (10s), snapshots (300s)
    • Uses s-maxage + stale-while-revalidate for CDN-level caching
  • Migration: 00037_rate_limits.sql creates table with index + purge trigger

Fixes #966

Test plan

  • POST endpoints return 429 after 5 requests/min from same IP
  • Rate limiting persists across serverless invocations (DB-based)
  • GET endpoints include Cache-Control headers in responses
  • Normal airdrop flows (checkin, referral, points) unaffected
  • Migration runs cleanly on Supabase

🤖 Generated with Claude Code

…T endpoints

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
plotlink Ignored Ignored Apr 23, 2026 11:51pm

Request Review

Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

The new rate-limiting utility is serverless-safe, but it is not concurrency-safe. Because it does a separate count query and then an insert, concurrent requests from the same IP can all observe the same pre-insert count and exceed the intended 5/min limit.

Findings

  • [high] checkRateLimit() is race-prone and can be bypassed under concurrent load. The current implementation does SELECT count(*) for the window and then performs a separate INSERT. If multiple requests from the same IP hit at once, they can all see count < 5 before any of them insert, so more than 5 requests/min will be accepted. For a security control, this is a real enforcement bug.
    • File: lib/rate-limit.ts:18
    • Suggestion: Move this into a single atomic database operation, such as an RPC/function that both checks and records in one transaction, or a locking strategy that prevents concurrent over-admission.

Decision

Requesting changes because the rate limit can be bypassed by concurrency, so the main security control in this PR is not reliably enforced.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

Moving the rate limit into a Supabase RPC is the right fix conceptually, but the new SQL function is not valid as written. It tries to apply FOR UPDATE to an aggregate count(*) query, which PostgreSQL does not allow, so the migration/function will fail or be unusable.

Findings

  • [high] check_rate_limit() uses SELECT count(*) ... FOR UPDATE, but FOR UPDATE cannot be applied to aggregate queries in PostgreSQL. That means the new atomic rate limiter does not actually deploy/work in its current form.
    • File: supabase/migrations/00037_rate_limits.sql:34
    • Suggestion: Use a transaction-safe locking strategy that locks rows or a per-key coordinator row before counting/inserting, or restructure the function so it does not rely on FOR UPDATE over an aggregate result.

Decision

Requesting changes because the new database-side rate-limit function is invalid as written, so the main security control in this PR still is not ready.

… aggregate

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: APPROVE

Summary

The re-review update fixes the database-side rate limiter by replacing the invalid aggregate FOR UPDATE approach with a transaction-scoped advisory lock per key. That gives the POST airdrop endpoints a valid atomic rate-limit check while keeping the GET cache headers in place.

Findings

  • No blocking findings.

Decision

Approving because the previous SQL validity issue in check_rate_limit() is resolved and the PR now satisfies the reviewed security hardening requirements for the airdrop endpoints. Checks visible to me were still pending at review time.

@realproject7 realproject7 merged commit 0ab7dc2 into main Apr 23, 2026
4 checks passed
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.

[Security] Review airdrop API endpoints for abuse vectors

2 participants