Relay spend guards: per-IP rate limits, hard daily budget cap, structured logs#715
Conversation
…ured logs Cloudflare has no native hard billing cap, so enforce spend + abuse controls in the workers themselves. push-relay: - Close the unauthenticated-write gap: per-IP rate limit on /claim (default 10/min) plus a general per-IP limit on all routes (default 120/min), backed by a new rate_counters D1 table (fixed-window; rejects read-only once over, so the limiter never amplifies the spend it bounds). - Hard daily request budget (default 1,000,000/day ≈ well under $10/mo of overage) with an in-isolate latch so a blown budget rejects for free until midnight UTC. All three limits are wrangler vars — tune without a code change. - Structured JSON logs for rate_limited / budget_exceeded / auth_failed / apns_error / claim_conflict; observability enabled for persistent history. tunnel-relay: - Lifecycle logging (host_registered / connect_rejected / auth_failed) + observability. Its cost was already gated (signed upgrades, one control socket, max-tunnels, idle sweep) so no new limiter needed. Tests: 11 push-relay (limit trips, budget trips, per-IP isolation, health bypass) + 15 tunnel-relay, both typecheck clean. Migration applied to remote D1 and both workers redeployed; live smoke confirms /claim 429s at the limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds spend and abuse controls to the push-relay worker: a new ChangesPush-relay spend controls and logging
Tunnel-relay lifecycle logging
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Document the in-worker rate limits, daily spend budget, and structured logging in both worker READMEs and the push-notifications feature doc: the tunable wrangler vars (DAILY_REQUEST_BUDGET / IP_RATE_LIMIT_PER_MIN / CLAIM_RATE_LIMIT_PER_MIN), the rate_counters table, and how to view logs (dashboard + wrangler tail). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
- recordDailyBudget increments and reads the daily counter in one INSERT ... RETURNING statement, so a request never evaluates a count staler than its own increment; residual cross-request overshoot is bounded by in-flight concurrency (immaterial for a coarse 1M/day backstop) and now documented. - Skip the per-IP rate gates when cf-connecting-ip is absent: Cloudflare always sets it on the edge, so absent = off-edge (wrangler dev / service binding) where a shared `unknown` bucket would only throttle callers against each other. The global daily budget still applies, and the header can't be stripped on the real edge to bypass the gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d340948f07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…erve log kind - recordDailyBudget now runs before the per-IP gate, so a rate-limited 429 (which still ran the Worker + a D1 read) advances the daily spend cap. A single abusive IP can no longer generate unbounded billable 429s the cap never sees. - Per-IP and /claim gates always apply. When cf-connecting-ip is absent (off-edge only — Cloudflare always sets it on the real edge) callers share one fail-closed `unknown` bucket, so the unauthenticated write path is never bypassed. (Resolves the tension between the two prior absent-IP review findings in favor of preserving the limit.) - logEvent spreads `kind` LAST so a caller field named `kind` can't overwrite the event name; the apns_error calls' colliding `kind` field is renamed `push`, so `apns_error` log filters catch real failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 137a164568
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/push-relay/src/relay.ts (1)
1117-1165: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCount oversized requests before the shared budget check
contentLengthExceeds()returns beforerecordDailyBudget()and the IP/claim limiters, so oversized requests can still consume Worker invocations without advancingDAILY_REQUEST_BUDGETor tripping the per-IP gates. Move the 413 response behind the shared accounting step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/push-relay/src/relay.ts` around lines 1117 - 1165, The oversized-request early return in handleRequest is bypassing the shared accounting path, so move the contentLengthExceeds check to after recordDailyBudget and before any route-specific handling while keeping the existing json 413 response. Update handleRequest in relay.ts so every request, including payload-too-large cases, still flows through budgetTrippedNow, recordDailyBudget, and the per-IP/claim gating before returning.
🧹 Nitpick comments (1)
apps/push-relay/src/relay.ts (1)
486-490: 🔒 Security & Privacy | 🔵 TrivialPersisted client-IP logging — confirm retention expectations.
clientIp()feeds raw IPs intoauth_failed,rate_limited, andclaim_conflictstructured logs, which withobservability.enabledare persisted and queryable in the dashboard. This is standard abuse-log practice, but since IPs are personal data under most privacy regimes, worth confirming intended log retention/access controls align with data-handling expectations for this service.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/push-relay/src/relay.ts` around lines 486 - 490, The structured logs emitted from clientIp() in auth_failed, rate_limited, and claim_conflict are persisting raw IPs when observability.enabled is on, so update this path to meet the service’s intended data-handling policy. Either reduce exposure by masking or hashing the IP before it reaches those log events, or add an explicit retention/access-control gate around the log fields in relay.ts so the persisted payload matches the privacy expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/tunnel-relay/src/tunnelDo.ts`:
- Around line 131-134: The auth failure observability is incomplete because the
earlier 401 path in handlePipe() returns without emitting the same auth_failed
signal as the verified query failure path. Update handlePipe() so its !secret
branch logs auth_failed with the same shape used in tunnelDo.ts (including role
and machineKey when available) before returning the 401 response, mirroring the
existing logTunnel call used after verifySignedQuery().
---
Outside diff comments:
In `@apps/push-relay/src/relay.ts`:
- Around line 1117-1165: The oversized-request early return in handleRequest is
bypassing the shared accounting path, so move the contentLengthExceeds check to
after recordDailyBudget and before any route-specific handling while keeping the
existing json 413 response. Update handleRequest in relay.ts so every request,
including payload-too-large cases, still flows through budgetTrippedNow,
recordDailyBudget, and the per-IP/claim gating before returning.
---
Nitpick comments:
In `@apps/push-relay/src/relay.ts`:
- Around line 486-490: The structured logs emitted from clientIp() in
auth_failed, rate_limited, and claim_conflict are persisting raw IPs when
observability.enabled is on, so update this path to meet the service’s intended
data-handling policy. Either reduce exposure by masking or hashing the IP before
it reaches those log events, or add an explicit retention/access-control gate
around the log fields in relay.ts so the persisted payload matches the privacy
expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5098b2e-da80-4b7d-9023-7876989b065d
⛔ Files ignored due to path filters (7)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/push-notifications.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**
📒 Files selected for processing (8)
apps/push-relay/README.mdapps/push-relay/migrations/0002_rate_and_budget.sqlapps/push-relay/src/relay.tsapps/push-relay/test/relay.test.tsapps/push-relay/wrangler.jsoncapps/tunnel-relay/README.mdapps/tunnel-relay/src/tunnelDo.tsapps/tunnel-relay/wrangler.jsonc
…ne logging - checkRateLimit now increments and reads in one atomic INSERT ... ON CONFLICT ... RETURNING (window rollover handled inline via CASE), so a parallel burst at a window boundary can't all read below the limit and all be admitted — matching the budget counter's atomicity. - Add an hourly cron (scheduled handler) that prunes suppression / registration / rate-limit rows independent of request traffic, so pure /claim / 404 / unauthorized traffic from rotating IPs can't leave stale rate_counters rows to accumulate. - tunnel-relay: log auth_failed on the unknown-machine (!secret) 401 branches in handleHost/handlePipe too, closing the observability gap. Both workers redeployed; live smoke confirms the atomic /claim limit trips at the configured value on real D1 and the cron schedule registered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f025850ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- checkRateLimit's atomic upsert gains a WHERE guard on the DO UPDATE, so an already-over-limit window is a no-op (no write) and RETURNING yields no row → a sustained flood costs a read, not a write, per rejected hit. Keeps the atomicity from the prior round while restoring the read-only rejection path the write-cost finding flagged. - Lower DEFAULT_DAILY_REQUEST_BUDGET 1,000,000 → 500,000/day and correct the sizing math to count the guards' OWN ~2 D1 writes/request: at 1M/day those writes crossed the 50M/mo D1 free tier and pushed a capped month toward ~$16; at 500k/day requests+writes stay ≈ $1.50, safely under the ~$10 ceiling. Comment + README + docs updated to match. Redeployed; live smoke confirms the WHERE-guarded /claim limit trips correctly on real D1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Cloudflare has no native hard billing cap, so this enforces spend + abuse controls in the workers themselves. Already deployed to production (both workers redeployed, migration applied to remote D1, live smoke-tested); this PR lands the code in
mainso the next deploy-from-main keeps the protections.push-relay
/claim: per-IP rate limit (default 10/min) + a general per-IP limit on all routes (default 120/min), backed by a newrate_countersD1 table. Fixed-window; rejects read-only once a window is over, so the limiter never amplifies the spend it exists to bound.wrangler.jsoncvars — tunable without a code change.rate_limited/budget_exceeded/auth_failed/apns_error/claim_conflict, plusobservabilityenabled for persistent, queryable history.tunnel-relay
host_registered/connect_rejected/auth_failed) + observability. Its cost was already gated (signed upgrades, single control socket, max-tunnels cap, idle sweep), so no new limiter was needed.Verification
/healthbypass); tunnel-relay: 15 tests. Both typecheck clean./claimrequests pass, then429— the limit trips exactly at the configured value;/healthstillapnsConfigured:true.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR adds spend and abuse controls to the relay workers. The main changes are:
/claimrequest limits for push-relay.Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
What T-Rex did
Important Files Changed
Comments Outside Diff (1)
apps/push-relay/src/relay.ts, line 603-613 (link)The cleanup for non-budget
rate_countersrows only runs frompruneRelayState, while the new unauthenticated/claimlimiter createsip:*andclaim:*rows without reaching that cleanup path. A rotating-IP claim flood can leave stale counter rows in D1 until some later device upsert or publish happens to prune them, turning the abuse path into persistent table growth.Prompt To Fix With AI
Reviews (6): Last reviewed commit: "review: no D1 write on rate-limit reject..." | Re-trigger Greptile