fix(web): wire three unused rate-limit ids and guard against new ones#2040
fix(web): wire three unused rate-limit ids and guard against new ones#2040DPS0340 wants to merge 3 commits into
Conversation
|
@tembo please review |
| let source: string; | ||
| try { | ||
| source = readFileSync(join(WEB_ROOT, file), "utf8"); | ||
| } catch { |
There was a problem hiding this comment.
Catching all readFileSync errors here can mask real failures and make the scan silently incomplete. Maybe only ignore ENOENT (deleted-from-worktree case) and rethrow otherwise.
| } catch { | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | |
| continue; // deleted from the worktree but still in the index | |
| } | |
| throw error; | |
| } |
| * Uses git's own file list so the search sees exactly the tracked sources and | ||
| * never walks node_modules or .next. | ||
| */ | ||
| function referencedKeys(): Set<string> { |
There was a problem hiding this comment.
referencedKeys() shells out to git + reads every TS/TSX file; since it’s called in multiple tests, memoizing the result would keep this suite from doing the full scan more than once.
|
Both taken, pushed in Swallowing every read error. You're right, and it's the worse of the two because of what this test is for: a silently shrinking scan makes the guard pass while checking less, which is the exact failure mode the test exists to prevent. Now only Memoizing. Done. Measured against the real repo: so it's three fewer full walks of the 960 tracked files across the suite. Re-ran the assertions against the actual repo after both changes — the answer depends on the real tracked file set, so I check it there rather than in a fixture: @tembo please re-review. |
| { cwd: WEB_ROOT, encoding: "utf8", maxBuffer: 32 * 1024 * 1024 }, | ||
| ) | ||
| .split("\0") | ||
| .filter((file) => file && file !== DECLARATION_FILE); |
There was a problem hiding this comment.
This scan includes __tests__/**, so a RATE_LIMIT_IDS.X referenced only from tests could make an unwired id look "wired". Excluding tests from tracked keeps this guard focused on runtime call sites.
| .filter((file) => file && file !== DECLARATION_FILE); | |
| .filter((file) => file && file !== DECLARATION_FILE && !file.startsWith("__tests__/")); |
|
Right again, and this one was the most consequential of the three — pushed in It's worse than "a test could mask an unwired id": this file itself names every key, so once it lands, the scan would see all 13 referenced and the guard would be permanently vacuous. It only passed review because the assertions happen to be written as I proved the hole rather than assuming it. Staged a probe test whose only content is a reference to So the test-inclusive scan both hides a genuinely unwired id and fires a false "stale allowance" failure telling you to delete a Current state of the tracked-file set, tests excluded: For the record, no id is test-only in @tembo please re-review. |
What
Wires three declared-but-never-called rate-limit ids to the unauthenticated endpoints they name, and adds a test so a new unwired id fails CI instead of being found by reading.
Closes #2039.
Why
RATE_LIMIT_IDSdeclares 13 ids, each with a doc comment naming the abuse it guards. Counting references outside the declaration file, 7 were never called, so those endpoints ran with no limit:This is quiet because there is already one silent-failure mode by design — the file's own comment notes that an id with no matching Vercel Firewall dashboard rule fails open. A declared-but-uncalled id is a second one, and it is invisible from the dashboard side too: someone can create
rl_guest_checkoutin the Firewall, see it configured, and reasonably assume the endpoint is protected while no code ever calls it.What this PR wires
Three endpoints I read end to end and confirmed are unauthenticated with no existing limit:
api/settings/billing/guest-checkout— no auth of any kind. ReadspriceId/quantityfrom the body and callsstripe().checkout.sessions.create(...). Limited before anything is read from the body.api/analytics/track— anonymous by design (provideOptionalAuth). Each accepted call is a Tinybird ingest and can fan out intocreateAnonymousViewNotification/sendFirstViewEmail. Exactly the cost the comment describes.api/tools/loom-download— unauthenticated, downloads a remote video and can run it through ffmpeg. Limited before any fetching starts.All three follow the existing
api/docs/askpattern:isRateLimited(id, { headers: request.headers }), return 429. They inherit the helper's existing best-effort semantics, so a firewall outage or a self-hosted deploy without the Firewall behaves exactly as it does today.What this PR deliberately does NOT wire
AUTH_OTP_VERIFY,AUTH_OTP_SEND,MESSENGER_MESSAGEandDESKTOP_LOGS. I could not confidently locate the single right call site for each, and guessing at an auth or messaging path is not something I want to do blind in a security change. They are listed in aKNOWN_UNWIREDmap in the test with a reason each, so:Tell me which of the four you want next and I will follow up with the call sites.
Testing
New
__tests__/unit/rate-limit-ids.test.ts, 4 tests:lib/rate-limit.ts, except the documentedKNOWN_UNWIREDentriesKNOWN_UNWIREDthat is now referenced fails, so the list cannot linger and mask a future regressionThe scan uses
git ls-filesfor its file list, so it sees exactly the tracked sources and never walksnode_modulesor.next.Verification. I ran the test's logic against the real repo rather than a copy, since the result depends on the actual file set:
Revert check. Reverting just the
analytics/trackwiring and re-running:so the test genuinely detects an unwired id rather than passing by construction.
What I did not do. I have not exercised these three routes against a live deploy with the Firewall rules present —
isRateLimitedreturnsfalseoutside production, so the added branch is inert in a local run. The three ids still need matching Rate Limit rules in the Vercel Firewall dashboard for the protection to take effect, per the helper's own documentation; this PR only makes the code path exist.