Repository navigation
Harden production web endpoint controls - #5341
lawrencecchen wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResolve runtime/test rate-limit IDs for feedback and push routes, treat firewall errors as temporary unavailability (503), add multipart/form-data guard (415), make JSON responses non-cacheable, extend CI DB tests, add two VM env keys, and update tests to use per-id firewall handler registries. ChangesAPI Hardening and Resilience
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 79c9e9af09
ℹ️ 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".
| }, | ||
| })); | ||
|
|
||
| const feedbackRoute = await import("../app/api/feedback/route"); |
There was a problem hiding this comment.
When this file runs as part of the normal web job's bun test, another test can import ../app/env first while the feedback env vars are still unset. Because env is a module-level singleton, this import then reuses the cached undefined feedback config and both new feedback tests receive 503 { error: "Feedback endpoint is not configured" } instead of exercising the limiter/content-type paths; cd web && bun test tests/notifications-push-route.test.ts tests/vm-route-auth.test.ts tests/feedback-route.test.ts reproduces. Please isolate/reset the env module or mock the config dependency before importing the route.
Useful? React with 👍 / 👎.
79c9e9a to
097f160
Compare
Greptile SummaryThis PR hardens production web endpoints following a pentest: feedback and push routes now fail closed (503) when the Vercel Firewall limiter is unavailable, the feedback route rejects non-multipart bodies with 415 before any parsing, and all VM API JSON responses (including SSH credential payloads) gain
Confidence Score: 4/5Safe to merge after verifying CMUX_PUSH_RATE_LIMIT_ID is set in every production Vercel environment, or after adding it to the audit's required key list. The push route now returns 503 for every authenticated user if CMUX_PUSH_RATE_LIMIT_ID is absent from the Vercel environment, whereas before it simply skipped rate limiting. Because this key is not added to requiredRuntimeEnvKeys in the audit script, the strict deploy audit won't catch a missing or accidentally deleted value. If production already has the key set this is a non-issue, but the audit gap means a future configuration drift could silently break all push notifications without any pre-deploy warning. web/scripts/cloud-vm/projects.mjs — CMUX_PUSH_RATE_LIMIT_ID should be added to requiredRuntimeEnvKeys alongside the VM credit keys that were added in this PR. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /api/feedback] --> B{feedbackConfig set?}
B -- No --> C[503 not configured]
B -- Yes --> D{rateLimitId resolved?}
D -- No --> G[skip rate limit]
D -- Yes --> E[checkRateLimit]
E --> F{result?}
F -- rateLimited/blocked --> H[429 Rate limit exceeded]
F -- error --> I[503 rate_limiter_unavailable]
F -- ok --> G
G --> J{multipart/form-data?}
J -- No --> K[415 Invalid multipart payload]
J -- Yes --> L[parse formData + send email]
M[POST /api/notifications/push] --> N{auth ok?}
N -- No --> O[401 unauthorized]
N -- Yes --> P{isPushRateLimitRequired AND no ID?}
P -- Yes --> Q[503 rate_limiter_unavailable]
P -- No --> R{rateLimitId set?}
R -- No --> S[skip Vercel rate limit]
R -- Yes --> T[checkRateLimit]
T --> U{result?}
U -- rateLimited/blocked --> V[429 rate_limited]
U -- error --> W[503 rate_limiter_unavailable]
U -- ok --> S
S --> X[read body → DB → APNs]
Reviews (4): Last reviewed commit: "Harden production web endpoint controls" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (error === "not-found") { | ||
| console.error("notifications.push.rate_limit_not_found", env.CMUX_PUSH_RATE_LIMIT_ID); | ||
| console.error("notifications.push.rate_limit_not_found", rateLimitId); | ||
| } |
There was a problem hiding this comment.
"not-found"
The feedback route in this same PR was hardened to return 503 on any error from the Vercel firewall (including "not-found"). The push route still logs and continues when the configured limiter ID can't be found, meaning push requests proceed without rate limiting if the firewall rule is deleted or misconfigured in production. Given the pentest context of this PR, parity between the two routes is worth considering — an unreachable limiter silently disables the only Vercel-layer rate check for this endpoint.
| export function configureFeedbackRateLimitForTests(rateLimitId: string | null) { | ||
| testFeedbackRateLimitId = rateLimitId; | ||
| } |
There was a problem hiding this comment.
configureFeedbackRateLimitForTests (and configurePushRateLimitForTests in the push route) are exported symbols in the production bundle that mutate module-level state. In practice this is safe because resolveFeedbackRateLimitId gates on process.env.VERCEL === "1" and ignores testFeedbackRateLimitId in production, but exporting test hooks from production route files is a pattern that can cause confusion and makes the surface slightly harder to audit. A process.env.NODE_ENV === "test" guard or a separate test-utilities file would keep the escape hatch clearly out of production symbol exports.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/feedback/route.ts`:
- Around line 76-83: Replace the two hardcoded English response strings in the
feedback route with locale lookups: instead of returning jsonError("Feedback is
temporarily unavailable", 503) and jsonError("Unsupported feedback content
type", 415), call the i18n/message getter used in this API (via next-intl or the
repo’s locale helper) inside the same handler so jsonError receives localized
text; update the corresponding message keys in every locale bundle referenced by
web/i18n/routing.ts under web/messages/ (add entries for the new keys) so all
supported locales return translated copies; ensure the lookup is performed
before calling jsonError and that you use the same key names consistently (e.g.,
feedback.rate_limit_unavailable and feedback.unsupported_content_type) to match
the new message entries.
In `@web/scripts/cloud-vm/projects.mjs`:
- Around line 33-34: The audit currently treats a key as present based only on
Object.keys(env), so update web/scripts/cloud-vm/audit-vercel-env.mjs to
validate that required Vercel env vars are non-empty strings instead of just
existing; specifically ensure the new keys
"CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST" and
"CMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_ID" are checked for truthy/non-blank
values (e.g., trim() !== "" or length > 0) and fail the --strict audit when
either is empty or whitespace-only.
In `@web/tests/feedback-route.test.ts`:
- Around line 3-24: The test mutates process.env.SKIP_ENV_VALIDATION and
process.env.VERCEL but only restores keys from envKeys; update the
setup/teardown to capture the original values for SKIP_ENV_VALIDATION and VERCEL
(alongside originalEnv) and ensure the afterAll restores them as well; modify
the afterAll cleanup that currently iterates envKeys (referencing envKeys and
originalEnv) to also reset process.env.SKIP_ENV_VALIDATION and
process.env.VERCEL back to their original values (or delete them if originally
undefined) so those flags do not leak into other suites.
🪄 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
Run ID: dc3ee307-32f6-488d-b4eb-fc78a1e06f09
📒 Files selected for processing (8)
.github/workflows/ci.ymlweb/app/api/feedback/route.tsweb/app/api/notifications/push/route.tsweb/scripts/cloud-vm/projects.mjsweb/services/vms/routeHelpers.tsweb/tests/feedback-route.test.tsweb/tests/notifications-push-route.test.tsweb/tests/vm-route-auth.test.ts
| if (error) { | ||
| console.error("feedback.route.rate_limit_unavailable", error); | ||
| return jsonError("Feedback is temporarily unavailable", 503); | ||
| } | ||
| } | ||
|
|
||
| if (!isMultipartFormData(request)) { | ||
| return jsonError("Unsupported feedback content type", 415); |
There was a problem hiding this comment.
Localize these new feedback error bodies.
Lines 78 and 83 add new production API response copy as hardcoded English strings. Because this route lives under web/app/api, these messages need to come from the repo’s locale source and be added across every supported locale instead of being embedded in the handler.
As per coding guidelines, web/**/*.{ts,tsx,js,md} production API response changes must read from next-intl or another locale-specific source and update every locale listed in web/i18n/routing.ts with matching web/messages/ entries.
🤖 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/feedback/route.ts` around lines 76 - 83, Replace the two
hardcoded English response strings in the feedback route with locale lookups:
instead of returning jsonError("Feedback is temporarily unavailable", 503) and
jsonError("Unsupported feedback content type", 415), call the i18n/message
getter used in this API (via next-intl or the repo’s locale helper) inside the
same handler so jsonError receives localized text; update the corresponding
message keys in every locale bundle referenced by web/i18n/routing.ts under
web/messages/ (add entries for the new keys) so all supported locales return
translated copies; ensure the lookup is performed before calling jsonError and
that you use the same key names consistently (e.g.,
feedback.rate_limit_unavailable and feedback.unsupported_content_type) to match
the new message entries.
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)
web/tests/notifications-push-route.test.ts (1)
58-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the push-route test override in teardown.
beforeEachinstalls a process-wide limiter override and handler, butafterAllonly restores env vars. That leavesconfigurePushRateLimitForTests("cmux-push-test")and the"cmux-push-test"registry entry live for later suites in the same Bun worker, so unrelated push-route tests can silently inherit this mocked limiter.Suggested fix
afterAll(() => { + pushRoute.configurePushRateLimitForTests(null); + firewallState.__cmuxFirewallCheckRateLimits?.delete("cmux-push-test"); for (const key of envKeys) { const value = originalEnv[key]; if (typeof value === "undefined") { delete process.env[key]; } else { process.env[key] = value; } } });🤖 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/tests/notifications-push-route.test.ts` around lines 58 - 77, The test installs a process-wide limiter override via pushRoute.configurePushRateLimitForTests("cmux-push-test") and registers a handler in firewallState.__cmuxFirewallCheckRateLimits but the afterAll teardown only restores env vars; fix by undoing those test-only changes in afterAll: call pushRoute.configurePushRateLimitForTests(undefined) (or the no-arg/reset variant) to clear the override and delete the "cmux-push-test" entry from firewallState.__cmuxFirewallCheckRateLimits (e.g., firewallState.__cmuxFirewallCheckRateLimits?.delete("cmux-push-test")), ensuring the test mutation is fully reverted.
🤖 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/tests/feedback-route.test.ts`:
- Around line 54-75: Add teardown to remove the test-only limiter and the
firewall handler after each test: call
feedbackRoute.configureFeedbackRateLimitForTests(undefined) (or null) to clear
the module-global override set by
configureFeedbackRateLimitForTests("feedback-limiter"), then remove the handler
from firewallState.__cmuxFirewallCheckRateLimits by calling its
delete("feedback-limiter") and, if the map becomes empty, clear or unset
__cmuxFirewallCheckRateLimits; also clear mocks (checkRateLimit.mockClear(),
send.mockClear()) to ensure no order-dependence between suites.
---
Outside diff comments:
In `@web/tests/notifications-push-route.test.ts`:
- Around line 58-77: The test installs a process-wide limiter override via
pushRoute.configurePushRateLimitForTests("cmux-push-test") and registers a
handler in firewallState.__cmuxFirewallCheckRateLimits but the afterAll teardown
only restores env vars; fix by undoing those test-only changes in afterAll: call
pushRoute.configurePushRateLimitForTests(undefined) (or the no-arg/reset
variant) to clear the override and delete the "cmux-push-test" entry from
firewallState.__cmuxFirewallCheckRateLimits (e.g.,
firewallState.__cmuxFirewallCheckRateLimits?.delete("cmux-push-test")), ensuring
the test mutation is fully reverted.
🪄 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
Run ID: aa6d217d-e461-4981-9487-d961b2f4b0cc
📒 Files selected for processing (8)
.github/workflows/ci.ymlweb/app/api/feedback/route.tsweb/app/api/notifications/push/route.tsweb/scripts/cloud-vm/projects.mjsweb/services/vms/routeHelpers.tsweb/tests/feedback-route.test.tsweb/tests/notifications-push-route.test.tsweb/tests/vm-route-auth.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/app/api/feedback/route.ts">
<violation number="1" location="web/app/api/feedback/route.ts:80">
P3: New user-facing API error messages are hardcoded English strings instead of using the project’s localization path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| console.error("feedback.route.rate_limit_error", error); | ||
| if (error) { | ||
| console.error("feedback.route.rate_limit_unavailable", error); | ||
| return jsonError("Feedback is temporarily unavailable", 503); |
There was a problem hiding this comment.
P3: New user-facing API error messages are hardcoded English strings instead of using the project’s localization path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/app/api/feedback/route.ts, line 80:
<comment>New user-facing API error messages are hardcoded English strings instead of using the project’s localization path.</comment>
<file context>
@@ -73,16 +75,16 @@ export async function POST(request: Request) {
- console.error("feedback.route.rate_limit_error", error);
+ if (error) {
+ console.error("feedback.route.rate_limit_unavailable", error);
+ return jsonError("Feedback is temporarily unavailable", 503);
}
}
</file context>
097f160 to
29556b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/feedback/route.ts`:
- Around line 78-80: The current branch logs limiter failures but returns
"Feedback endpoint is not configured" to callers; change the user-facing
response in the error branch inside the feedback route handler (where
resolveFeedbackConfig() has already succeeded) to return a neutral
temporary-unavailable error (e.g., machine code "rate_limiter_unavailable" or a
generic "Service temporarily unavailable") via jsonError with a 503 status
instead of exposing configuration state; keep the internal
console.error("feedback.route.rate_limit_unavailable", error) for diagnostics
but ensure jsonError does not leak config details.
In `@web/scripts/cloud-vm/projects.mjs`:
- Around line 33-34: The two env keys CMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_ID
and CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST are documented as optional but are
currently listed in requiredRuntimeEnvKeys causing production audit failures;
either remove these keys from requiredRuntimeEnvKeys (to make them truly
optional) or keep them required but update the runtime validation in the same
module to accept explicit opt-out sentinel values ("none", "disabled", "off",
"false") and treat CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST defaulting to 1 when
opted out; also update the README wording for
CMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_ID and CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST
to clearly state the production contract (required with valid value or required
with explicit opt-out value vs optional in dev/preview).
🪄 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
Run ID: 713fbc33-8ff5-48c6-b8b0-76d2f2282f77
📒 Files selected for processing (9)
.github/workflows/ci.ymlweb/app/api/feedback/route.tsweb/app/api/notifications/push/route.tsweb/scripts/cloud-vm/audit-vercel-env.mjsweb/scripts/cloud-vm/projects.mjsweb/services/vms/routeHelpers.tsweb/tests/feedback-route.test.tsweb/tests/notifications-push-route.test.tsweb/tests/vm-route-auth.test.ts
29556b9 to
b67e6f1
Compare
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/notifications/push/route.ts`:
- Around line 58-60: The code currently skips rate limiting when
resolvePushRateLimitId() returns no id; update the handler(s) that call
resolvePushRateLimitId() (both usages around the first call and the later call
at the other block) to treat a missing rateLimitId as a fail-closed condition:
detect when rateLimitId is falsy and immediately respond with a
temporary-unavailable/503 error (and log the missing CMUX_PUSH_RATE_LIMIT_ID)
instead of calling checkRateLimit or proceeding; ensure the same change is
applied to both places where resolvePushRateLimitId() is used so the production
path cannot bypass throttling.
🪄 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
Run ID: 1e55fcbd-4bc0-46f7-98ec-75a93d429921
📒 Files selected for processing (10)
.github/workflows/ci.ymlweb/app/api/feedback/route.tsweb/app/api/notifications/push/route.tsweb/scripts/cloud-vm/audit-vercel-env.mjsweb/scripts/cloud-vm/projects.mjsweb/services/vms/README.mdweb/services/vms/routeHelpers.tsweb/tests/feedback-route.test.tsweb/tests/notifications-push-route.test.tsweb/tests/vm-route-auth.test.ts
b67e6f1 to
528ef75
Compare
| export const nonBlankRequiredRuntimeEnvKeys = [ | ||
| "CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST", | ||
| "CMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_ID", | ||
| ]; |
There was a problem hiding this comment.
CMUX_PUSH_RATE_LIMIT_ID not audited but now required in production
This PR changes push-route behavior: on Vercel, a missing CMUX_PUSH_RATE_LIMIT_ID now makes isPushRateLimitRequired() && !rateLimitId true, returning 503 for every authenticated push request. Before this PR the missing-ID path simply skipped rate limiting and let the request proceed. The audit script doesn't include CMUX_PUSH_RATE_LIMIT_ID in requiredRuntimeEnvKeys, so a deployment without this env var passes the strict audit while silently breaking all push notifications for every user.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528ef75b16
ℹ️ 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".
| const missingRequired = requiredRuntimeEnvKeys.filter((key) => { | ||
| if (!present.has(key)) return true; | ||
| return nonBlankRequired.has(key) && !hasNonBlankEnvValue(env, key); |
There was a problem hiding this comment.
When CMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_ID is configured to a real Stack item but CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST is set to a non-empty invalid value such as 0, none, or abc, the new strict audit still reports ok because it only checks for a non-blank string here. The VM create path later calls positiveInteger for CMUX_VM_PLAN_FREE_CREATE_CREDIT_COST in billingGateway.ts, so the first production create for that plan will fail at runtime even though cloud-vm:env:audit -- --strict passed; the audit should validate a positive integer unless the item id is explicitly disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/api/feedback/route.ts (1)
188-195: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider aligning cache-control with the VM route contract.
The feedback route uses
Cache-Control: no-storewhile the sharedjsonResponsehelper inweb/services/vms/routeHelpers.ts(lines 93-101) setscache-control: private, no-store. Theprivatedirective provides an additional guarantee that intermediate caches (CDNs, proxies) won't store the response even if they ignoreno-store.For consistency and defense-in-depth on sensitive endpoints, consider using
private, no-storehere as well.♻️ Suggested alignment
return NextResponse.json( { ok: true }, { headers: { - "Cache-Control": "no-store", + "Cache-Control": "private, no-store", }, }, );function jsonError(message: string, status: number) { return NextResponse.json( { error: message }, { status, headers: { - "Cache-Control": "no-store", + "Cache-Control": "private, no-store", }, }, ); }Also applies to: 409-418
🤖 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/feedback/route.ts` around lines 188 - 195, The feedback route is setting Cache-Control to "no-store" while the shared jsonResponse helper uses "private, no-store"; update the NextResponse.json responses in the feedback handler (the returns that currently set headers with "Cache-Control": "no-store") to use "Cache-Control": "private, no-store" instead so they align with the jsonResponse behavior (and apply the same change to the other similar return blocks in this file).
🤖 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.
Outside diff comments:
In `@web/app/api/feedback/route.ts`:
- Around line 188-195: The feedback route is setting Cache-Control to "no-store"
while the shared jsonResponse helper uses "private, no-store"; update the
NextResponse.json responses in the feedback handler (the returns that currently
set headers with "Cache-Control": "no-store") to use "Cache-Control": "private,
no-store" instead so they align with the jsonResponse behavior (and apply the
same change to the other similar return blocks in this file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0eaea710-14d9-4711-8938-e9981129a234
📒 Files selected for processing (10)
.github/workflows/ci.ymlweb/app/api/feedback/route.tsweb/app/api/notifications/push/route.tsweb/scripts/cloud-vm/audit-vercel-env.mjsweb/scripts/cloud-vm/projects.mjsweb/services/vms/README.mdweb/services/vms/routeHelpers.tsweb/tests/feedback-route.test.tsweb/tests/notifications-push-route.test.tsweb/tests/vm-route-auth.test.ts
Stale CodeRabbit review on superseded commit; addressed in latest commit and latest CodeRabbit review is non-blocking.
Preserve current firewall opt-out and database push limiting policies while retaining multipart validation, private VM JSON responses, and explicit free-credit audit configuration. Keep current provider, authentication, delivery, and CI behavior.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
All contributors have signed the CLA ✍️ ✅ |
|
Fleet instruction update for head |
teamleaderleo
added
area: build-and-ci
CI failure attributionCI passes on Written by |
|
This is red for a reason that has nothing to do with the change. Both this and #5341 fail only Those ten files and The cause is
This PR was updated today, so that is not staleness from sitting idle. And because the branch has merged main in since, Verified against the real module in a scratch repo, driving it from both inputs: So the validator is correct and is being handed a bad baseline. A fix is in flight on Note — Raindrop g2 🫧 / Run: run_worker_20260930_3fc64ba6 |
This branch was successfully deployed
2 participants
Summary
Pentest context
Read-only production subagents tested notifications, device tokens, VM APIs, public pages, feedback, third-party routes, CORS preflights, fake tokens, malformed bodies, and method confusion. No auth bypass was found. This PR fixes the hardening gaps they found.
Verification
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes authentication-adjacent rate limiting and cache headers on VM credential responses; misconfigured limiters or env audit rules could block legitimate traffic until production env is updated.
Overview
This PR tightens production web endpoint controls after pentest-style review: limiters and payloads fail closed, sensitive VM JSON is not cacheable, and deploy/CI checks catch gaps earlier.
Feedback (
/api/feedback): When a configured Vercel Firewall limiter is active, any limiter error (not onlynot-found) returns 503rate_limiter_unavailableinstead of continuing without protection. Non-multipart/form-databodies are rejected with 415 beforeformData()parsing. Test-onlyconfigureFeedbackRateLimitForTestswires limiter behavior withoutVERCEL=1.Push (
/api/notifications/push): Production (and tests withrequired: true) require a limiter id; missing id or limiter errors return 503 before reading the body or touching the DB. Existing 429 behavior when limited is unchanged; tests assert limiter runs before oversized bodies/DB.VM APIs: Shared
jsonResponsenow setsCache-Control: private, no-storeon all VM JSON responses, including SSH credential payloads.Deploy audit & docs:
CMUX_VM_PLAN_FREE_CREATE_CREDIT_COSTandCMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_IDare required in the Vercel env audit and must be non-empty (explicit opt-out values likenonestill satisfy presence). README reflects that production must declare an intentional free-plan credit choice.CI: The Postgres migration job also runs
device-tokens-routeandnotification-rate-limittests, plus newfeedback-routecoverage.Reviewed by Cursor Bugbot for commit 528ef75. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Hardens production web endpoints: feedback rejects non-multipart bodies with 415 before parsing, VM API JSON responses are no longer cacheable, and the production env audit now requires explicit free-plan create-credit configuration.
Bug Fixes
formData()parsing; the configured firewall check runs before the body is read.jsonResponseaddsCache-Control: private, no-storeto all responses, including credential payloads.Migration
CMUX_VM_PLAN_FREE_CREATE_CREDIT_COSTandCMUX_VM_PLAN_FREE_CREATE_CREDIT_ITEM_IDin production (must be non-empty; usenone/disabled/off/falseto explicitly opt out).Written for commit f173093. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Documentation