Repository navigation
Make every rate-limit env var optional and fail open on deleted rules - #8818
Merged
Merged
Conversation
Aziz's ruling: no rate limits at all, and nothing may break when the rule ids are unset or their Vercel firewall rules are deleted. This extends the 8714/8773/8771 fail-open pattern to the remaining consumers: - env.ts: CMUX_FEEDBACK/CLIENT_CONFIG/ANALYTICS_RATE_LIMIT_ID become optional (client-config and analytics previously hard-failed production deploy env validation when unset; feedback failed every deploy). - analytics/events + client-config routes: unset id skips limiting instead of 503ing; a not-found rule warns and fails open; genuine check failures still 503. - waitlist + feedback routes: guard the limiter on the optional id and fail open on not-found instead of 503ing the endpoint. - enterprise/contact + feedback config resolvers no longer treat a missing rate-limit id as 'endpoint not configured'. - push and vault routes already guarded/failed open; unchanged. Also fixes a pre-existing red test on main (client-config-env expected CMUX_IROH_RATE_LIMIT_ID to be required, stale since 8714/8771). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughVercel rate-limit identifiers are now optional across affected routes. Missing identifiers skip enforcement, while missing rate-limit rules fail open with warnings. Other limiter errors and blocked requests retain their existing responses, with environment and route tests updated accordingly. ChangesVercel rate-limit configuration and handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 inconclusive)
✅ Passed checks (22 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 |
Contributor
Greptile SummaryThis PR extends the fail-open pattern to all remaining rate-limit consumers, makes every
Confidence Score: 5/5Safe to merge. The change consistently applies the intended fail-open pattern across all rate-limit consumers and removes env var requirements that were blocking deployments. All five routes correctly guard on both the Vercel flag and a non-empty rate-limit ID before calling checkRateLimit, so no firewall calls are made when IDs are absent. The not-found path now warns and continues rather than returning 503. Genuine check errors still return 503 in four of five routes. The enterprise/contact fall-through on genuine errors is pre-existing and already flagged in a prior review. web/app/api/enterprise/contact/route.ts — genuine rate-limit errors still fall through rather than returning 503, and not-found uses console.error instead of console.warn. Both are pre-existing and flagged in a prior review. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{VERCEL === '1'\nAND rateLimitId set?}
B -- No --> F[Continue to handler]
B -- Yes --> C[checkRateLimit]
C --> D{Result}
D -- rateLimited / blocked --> E[Return 429]
D -- not-found --> G[console.warn\nfail open]
G --> F
D -- other error --> H{Route?}
H -- analytics / client-config\nfeedback / waitlist --> I[Return 503]
H -- enterprise/contact --> J[console.error\nfail open pre-existing]
J --> F
D -- ok --> F
F --> K[Execute route logic]
Reviews (2): Last reviewed commit: "Merge origin/main (resolve client-config..." | Re-trigger Greptile |
![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
🤖 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 `@web/app/api/enterprise/contact/route.ts`:
- Line 54: Update the rate-limiter error handling in the route around the
VERCEL/config.rateLimitId branch so only a not-found limiter error is warned
about and allowed to continue. For any other limiter error, return an HTTP 503
response before sending the enterprise email, preserving the existing success
and not-found paths.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 14b6a26a-0ac5-4391-9884-2b8110f216b2
📒 Files selected for processing (9)
web/app/api/analytics/events/route.tsweb/app/api/client-config/route.tsweb/app/api/enterprise/contact/route.tsweb/app/api/feedback/route.tsweb/app/api/waitlist/route.tsweb/app/env.tsweb/tests/client-config-env.test.tsweb/tests/client-config-route.test.tsweb/tests/feedback-route.test.ts
| } | ||
|
|
||
| if (process.env.VERCEL === "1") { | ||
| if (process.env.VERCEL === "1" && config.rateLimitId) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Return a failure for non-not-found limiter errors.
Line 54 enters a branch whose current else if (error) only logs and then sends the enterprise email. This makes genuine limiter failures fail open too. Warn and continue only for not-found; return a 503 for other errors.
Proposed fix
if (error === "not-found") {
- console.error(
+ console.warn(
"enterprise.contact.rate_limit_not_found",
config.rateLimitId,
);
} else if (error) {
console.error("enterprise.contact.rate_limit_error", error);
+ return jsonError("service_unavailable", 503);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (process.env.VERCEL === "1" && config.rateLimitId) { | |
| if (error === "not-found") { | |
| console.warn( | |
| "enterprise.contact.rate_limit_not_found", | |
| config.rateLimitId, | |
| ); | |
| } else if (error) { | |
| console.error("enterprise.contact.rate_limit_error", error); | |
| return jsonError("service_unavailable", 503); | |
| } |
🤖 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 `@web/app/api/enterprise/contact/route.ts` at line 54, Update the rate-limiter
error handling in the route around the VERCEL/config.rateLimitId branch so only
a not-found limiter error is warned about and allowed to continue. For any other
limiter error, return an HTTP 503 response before sending the enterprise email,
preserving the existing success and not-found paths.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Extends the fail-open pattern from #8714, #8773, and #8771 to every remaining rate-limit consumer, per the no-rate-limits ruling.
Why now: the two relay/iroh env vars were just unset in Vercel prod; this deploy applies them. The remaining three ids (
CMUX_ANALYTICS_RATE_LIMIT_ID,CMUX_CLIENT_CONFIG_RATE_LIMIT_ID,CMUX_FEEDBACK_RATE_LIMIT_ID) could not be unset before this change: env.ts hard-required them (deploys would fail validation) and the routes 503'd without them — client-config gates every app boot.Changes: all rate-limit ids optional in env.ts; unset id = no rate limiting; deleted rule (not-found) = warn + fail open; genuine check failures keep failing closed; enterprise/feedback config resolvers no longer treat a missing id as 'not configured'. Also fixes a pre-existing red test on main (stale iroh-required assertion).
Verification:
bun test870 pass / 0 fail (was 6 fail against old assertions + 1 pre-existing red on main);bun run typecheckclean.After merge + deploy, all remaining
*_RATE_LIMIT_IDenv vars can be deleted with zero firewall calls made anywhere.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Make all rate-limit env vars optional and fail open when a Vercel firewall rule is deleted. This prevents outages when limits are removed and keeps production deploys passing without these vars.
Refactors
env.ts:CMUX_FEEDBACK_RATE_LIMIT_ID,CMUX_CLIENT_CONFIG_RATE_LIMIT_ID, andCMUX_ANALYTICS_RATE_LIMIT_IDare optional; removed Vercel production validation.analytics/eventsandclient-config: unset id skips limiting;not-foundwarns and continues; errors still 503; blocked requests return 429.feedback,waitlist,enterprise/contact: guard on optional id;not-foundwarns and continues; missing id no longer marks endpoint “not configured.”Migration
*_RATE_LIMIT_IDenv vars; no firewall calls will be made.Written for commit 1aaa31c. Summary will update on new commits.
Summary by CodeRabbit