fix(api): stop letting userId partition the public flag rate limit - #717
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Unkey Deploy
|
Greptile SummaryThis PR adds a Cloudflare-only client-IP reader and uses it to place direct public flag requests under an aggregate per-client rate limit.
Confidence Score: 3/5This PR is not safe to merge until direct-origin requests cannot use a caller-supplied Cloudflare header to bypass the aggregate flag-evaluation limit. The new ceiling is skipped whenever Files Needing Attention: apps/api/src/routes/public/flags.ts, packages/shared/src/utils/client-ip.ts
|
| Filename | Overview |
|---|---|
| apps/api/src/routes/public/flags.ts | Adds the aggregate limiter, but header presence alone lets direct-origin callers bypass it with spoofed identities. |
| packages/shared/src/utils/client-ip.ts | Adds a focused Cloudflare-header reader whose trust guarantee depends on an unenforced proxy boundary. |
| packages/shared/src/utils/client-ip.test.ts | Covers header selection correctly but does not cover direct-origin callers supplying the nominally trusted header. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Public flag request] --> Trusted{cf-connecting-ip present?}
Trusted -->|Yes| Visitor[Per-IP bucket: 600/min]
Visitor --> Evaluate[Evaluate flag]
Trusted -->|No| User[Per-user/shared bucket: 600/min]
User --> Aggregate[Per-client aggregate: 6000/min]
Aggregate --> Evaluate
Direct[Direct-origin attacker] --> Spoof[Spoof rotating cf-connecting-ip]
Spoof --> Trusted
Reviews (1): Last reviewed commit: "fix(api,shared): bound the public flag l..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
enforcePublicFlagRateLimit keyed on `ip || userId`, and userId comes straight off the query string, so a caller with no ip header minted a fresh 600/min bucket per value it invented. Drop it: the limit keys on the client ip, and falls back to one shared bucket when there is none. Callers no longer pass userId, and the scope field collapses to ip or shared now that there is no third case to report.
f0d007b to
e237fc3
Compare
Closes the P1 cubic raised on the release PR (#714) for
apps/api/src/routes/public/flags.ts.The bug
enforcePublicFlagRateLimitbuilt its bucket key fromip || userId:userIdis read straight off the query string or body. Any caller without an IP header could invent a new value per request and mint a fresh 600/min bucket every time, so the limit did nothing.The fix
Drop
userIdfrom the limiter. The key is the client IP, falling back to one shared bucket when there is no IP:Both call sites stop passing
userId, andflag_rate_limit_scopecollapses toip/sharednow that there is no third case.Net: one file, +4/−26.
Scope
Deliberately narrow. An earlier revision of this branch added a Cloudflare-only IP reader in
packages/sharedplus aSELFHOST-gated aggregate ceiling. Greptile then pointed out the obvious hole — a directly-exposed origin can setcf-connecting-iptoo, so the ceiling was skipped exactly where it was needed. That design also reintroduced IP-trust configuration thata18d88bf0had just removed on purpose. It has been dropped entirely;packages/sharedis untouched by this PR.Header spoofing against
getClientIpremains possible off the edge. That is a property of all 18 of its call sites, not this endpoint, and is not something to fix with bespoke machinery here.clientIdrotation also still mints buckets. That predates this change and is inherent to keying a public endpoint on a public identifier.Verification
bun run check-types33/33 andbun run test27/27, both re-run with--forceto bypass the turbo cache.bun run lintclean, 14/14 policy tests.The
Testfailure on the previous push was a CI flake: exit code 130 (SIGINT) with every visible test passing.packages/rpc's real test script gives 149 pass / 0 fail identically on this branch and onstaging, and the job passed on re-run.