[codex] Harden CLI auth and durable rate limits#130
Open
ohong wants to merge 3 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (27)
📝 WalkthroughWalkthroughThis PR implements durable API rate limiting backed by Supabase and upgrades CLI authentication with per-request device secrets. A new migration adds an ChangesDurable Rate Limiting & CLI Device Secrets
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/api/usage/submit/route.ts (1)
466-483:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate each
entries[]item before readingdateanddata.
body.entriesis still untrusted JSON here, but the loop dereferencesentry.dateandentry.dataimmediately. Payloads like{"entries":[null]}or{"entries":[{"date":"2026-06-09"}]}will throw and turn a bad request into a 500 instead of a 400. Add a shape check before callingisValidDate()/validateEntry().Suggested fix
const seenDates = new Set<string>(); for (const entry of body.entries) { + if ( + !entry + || typeof entry !== "object" + || typeof entry.date !== "string" + || !entry.data + || typeof entry.data !== "object" + ) { + return NextResponse.json( + { error: "Each entry must include a date and data object" }, + { status: 400 }, + ); + } + if (!isValidDate(entry.date)) { return NextResponse.json({ error: `Invalid date: ${entry.date}` }, { status: 400 }); } if (seenDates.has(entry.date)) { return NextResponse.json({ error: `Duplicate date: ${entry.date}` }, { status: 400 });🤖 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/web/app/api/usage/submit/route.ts` around lines 466 - 483, The loop over body.entries dereferences entry.date and entry.data without validating entry's shape; add a guard at the top of the loop to ensure each entry is a non-null object with a string date and a defined data field (e.g., typeof entry === 'object' && entry !== null && typeof entry.date === 'string' && 'data' in entry) and return a 400 NextResponse.json error when the shape is invalid before calling isValidDate, isWithinBackfillWindow, or validateEntry; keep using the existing symbols seenDates, isValidDate, isWithinBackfillWindow, validateEntry and preserve the same error-response pattern.apps/web/e2e/golden-path/cli-verify.spec.ts (1)
45-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test no longer exercises the redirect contract.
Lines 45-52 still look for an anchor, but
apps/web/app/cli/verify/page.tsxnow renders a button that navigates withrouter.push.isVisiblestays false, so the assertions are skipped and the test passes even ifnextis broken. Click the button and assert the resulting/login?next=...URL instead.Suggested fix
- const signInLink = page.locator('a:has-text("Sign in to authorize")'); - const isVisible = await signInLink.isVisible().catch(() => false); + const signInButton = page.getByRole("button", { name: "Sign in to authorize" }); + const isVisible = await signInButton.isVisible().catch(() => false); if (isVisible) { - const href = await signInLink.getAttribute("href"); - expect(href).toContain("/login"); - expect(href).toContain("next="); + await signInButton.click(); + await expect(page).toHaveURL(/\/login\?next=/); + await expect(page).toHaveURL(/verify_secret=/); }🤖 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/web/e2e/golden-path/cli-verify.spec.ts` around lines 45 - 52, The test currently checks for an anchor but the component in cli/verify/page.tsx now renders a button that navigates with router.push, so update the test to click the button and assert the resulting URL includes the redirect query; specifically replace the signInLink anchor checks with a locate-and-click of the button text "Sign in to authorize" (use the same text locator), then wait for navigation and assert that page.url() contains "/login?next=" to verify the redirect contract.
🧹 Nitpick comments (1)
apps/web/__tests__/unit/migration-safety.test.ts (1)
186-202: 🏗️ Heavy liftThis only proves the SQL text, not that the migration can run.
These assertions still pass if the migration is unrunnable because of statement ordering or constraint validation against legacy rows. Add one execution-based fixture that seeds an old
cli_auth_codesrow and applies this migration inside a transaction so deploy-time failures are caught here.🤖 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/web/__tests__/unit/migration-safety.test.ts` around lines 186 - 202, The current test only inspects SQL text; add an execution-based sub-test inside the "latest cli_auth_codes hardening removes public grants and pending-code select policies" test that uses getLatestMigrationMatching to get the migration content, begins a DB transaction, seeds a legacy public.cli_auth_codes row (with columns needed to trigger constraint/policy checks), applies the migration SQL (execute the content against the test DB) inside that transaction, and then asserts the migration ran without throwing and that the resulting grants/policies are as expected; ensure you use the same migrations variable and the migration content from getLatestMigrationMatching and roll back the transaction after the assertions to keep the fixture isolated.
🤖 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/web/app/api/ai/generate-caption/route.ts`:
- Around line 26-34: The handler assumes request.json() returns an object and
destructures images and usage directly which throws for non-object JSON (null,
number, string); update the logic around request.json() in route.ts to validate
that the parsed body is a plain object before destructuring: call await
request.json() into body, check typeof body === "object" && body !== null (or
use Array.isArray/body instanceof Object as needed), and if the check fails
return a 400 JSON error; then safely extract images and usage from the validated
body.
In `@apps/web/app/api/auth/cli/init/route.ts`:
- Around line 46-48: The current construction of appUrl uses NEXT_PUBLIC_APP_URL
with a hardcoded production fallback which can cause incorrect verify links;
modify the logic that computes appUrl (the code that sets appUrl and builds
verifyUrl) to use request.nextUrl.origin as the fallback when
NEXT_PUBLIC_APP_URL is unset (or alternatively throw an explicit error if an app
URL must be provided), then rebuild verifyUrl with that origin and the existing
URLSearchParams (code and verify_secret) so verifyUrl points to the actual
request origin rather than "https://straude.com".
In `@apps/web/app/cli/verify/page.tsx`:
- Around line 36-39: The conditional that checks (!code || !verifySecret) in the
page component currently returns a message that only mentions a missing
authorization code; update the user-facing message to a generic "invalid or
incomplete authorization link" (or similar) so it correctly covers both missing
code and missing verifySecret. Locate the check around the variables code and
verifySecret in page.tsx and replace the text inside the <p> element rendered
for that branch to reflect the generic message.
In `@supabase/migrations/20260609200740_security_sweep_cli_auth_rate_limits.sql`:
- Around line 5-30: The migration updates on table cli_auth_codes are ordered
such that status is changed to 'expired' before the cli_auth_codes_status_check
is widened and before new columns poll_secret_hash and verify_secret_hash exist,
which can fail; reorder operations: first ALTER TABLE to ADD COLUMN IF NOT
EXISTS poll_secret_hash, verify_secret_hash, redeemed_at, then DROP and re-ADD
the cli_auth_codes_status_check to include 'expired', then backfill or UPDATE
legacy rows (e.g., set status = 'expired' or populate secret hashes) so no rows
violate the new logic, and only after that DROP/ADD the
cli_auth_codes_active_secrets_check to enforce the stricter secret-nonnull
constraint.
- Around line 87-88: The unconditional table-wide prune using "DELETE FROM
public.api_rate_limits WHERE expires_at < v_now - interval '1 hour';" must be
removed from the request path; instead either move this cleanup to an
out-of-band job (cron/pg_cron/worker) that runs periodically for
public.api_rate_limits, or make the prune opportunistic inside the RPC by
replacing the global DELETE with a bounded/conditional operation (e.g. DELETE
... WHERE expires_at < v_now - interval '1 hour' ORDER BY expires_at LIMIT <N>
or DELETE only for the caller's bucket ids), ensuring you reference the
expires_at/v_now logic and avoid full-table writes from the hot RPC.
---
Outside diff comments:
In `@apps/web/app/api/usage/submit/route.ts`:
- Around line 466-483: The loop over body.entries dereferences entry.date and
entry.data without validating entry's shape; add a guard at the top of the loop
to ensure each entry is a non-null object with a string date and a defined data
field (e.g., typeof entry === 'object' && entry !== null && typeof entry.date
=== 'string' && 'data' in entry) and return a 400 NextResponse.json error when
the shape is invalid before calling isValidDate, isWithinBackfillWindow, or
validateEntry; keep using the existing symbols seenDates, isValidDate,
isWithinBackfillWindow, validateEntry and preserve the same error-response
pattern.
In `@apps/web/e2e/golden-path/cli-verify.spec.ts`:
- Around line 45-52: The test currently checks for an anchor but the component
in cli/verify/page.tsx now renders a button that navigates with router.push, so
update the test to click the button and assert the resulting URL includes the
redirect query; specifically replace the signInLink anchor checks with a
locate-and-click of the button text "Sign in to authorize" (use the same text
locator), then wait for navigation and assert that page.url() contains
"/login?next=" to verify the redirect contract.
---
Nitpick comments:
In `@apps/web/__tests__/unit/migration-safety.test.ts`:
- Around line 186-202: The current test only inspects SQL text; add an
execution-based sub-test inside the "latest cli_auth_codes hardening removes
public grants and pending-code select policies" test that uses
getLatestMigrationMatching to get the migration content, begins a DB
transaction, seeds a legacy public.cli_auth_codes row (with columns needed to
trigger constraint/policy checks), applies the migration SQL (execute the
content against the test DB) inside that transaction, and then asserts the
migration ran without throwing and that the resulting grants/policies are as
expected; ensure you use the same migrations variable and the migration content
from getLatestMigrationMatching and roll back the transaction after the
assertions to keep the fixture isolated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3449a9e-51b5-4e86-a06b-6d556e7d316a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
apps/web/__tests__/api/ai-caption.test.tsapps/web/__tests__/api/auth-cli.test.tsapps/web/__tests__/api/comment-email-notifications.test.tsapps/web/__tests__/api/social.test.tsapps/web/__tests__/api/upload.test.tsapps/web/__tests__/api/usage-submit.test.tsapps/web/__tests__/flows/cli-push-flow.test.tsapps/web/__tests__/flows/post-lifecycle.test.tsapps/web/__tests__/flows/social-interactions.test.tsapps/web/__tests__/flows/web-import-flow.test.tsapps/web/__tests__/unit/migration-safety.test.tsapps/web/__tests__/unit/rate-limit.test.tsapps/web/app/api/ai/generate-caption/route.tsapps/web/app/api/auth/cli/init/route.tsapps/web/app/api/auth/cli/poll/route.tsapps/web/app/api/auth/cli/verify/route.tsapps/web/app/api/comments/[id]/reactions/route.tsapps/web/app/api/comments/[id]/route.tsapps/web/app/api/follow/[username]/route.tsapps/web/app/api/messages/route.tsapps/web/app/api/posts/[id]/comments/route.tsapps/web/app/api/posts/[id]/kudos/route.tsapps/web/app/api/upload/route.tsapps/web/app/api/usage/submit/route.tsapps/web/app/cli/verify/page.tsxapps/web/e2e/golden-path/cli-verify.spec.tsapps/web/lib/api/cli-auth.tsapps/web/lib/rate-limit.tsapps/web/package.jsonpackage.jsonpackages/cli/__tests__/commands/login.test.tspackages/cli/package.jsonpackages/cli/src/commands/login.tssupabase/migrations/20260609200740_security_sweep_cli_auth_rate_limits.sql
Comment on lines
+26
to
34
| let body: { images?: unknown; usage?: CaptionUsage }; | ||
| try { | ||
| body = await request.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); | ||
| } | ||
|
|
||
| const { images, usage } = body; | ||
|
|
There was a problem hiding this comment.
Guard non-object JSON payloads before destructuring.
request.json() can return valid non-object JSON (null, number, string). Destructuring body then throws and turns a client error into a 500.
Suggested fix
- let body: { images?: unknown; usage?: CaptionUsage };
+ let body: unknown;
try {
body = await request.json();
} catch {
return NextResponse.json({ error: "Invalid JSON" }, { status: 400 });
}
- const { images, usage } = body;
+ if (!body || typeof body !== "object" || Array.isArray(body)) {
+ return NextResponse.json({ error: "Invalid request body" }, { status: 400 });
+ }
+ const { images, usage } = body as { images?: unknown; usage?: CaptionUsage };🤖 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/web/app/api/ai/generate-caption/route.ts` around lines 26 - 34, The
handler assumes request.json() returns an object and destructures images and
usage directly which throws for non-object JSON (null, number, string); update
the logic around request.json() in route.ts to validate that the parsed body is
a plain object before destructuring: call await request.json() into body, check
typeof body === "object" && body !== null (or use Array.isArray/body instanceof
Object as needed), and if the check fails return a 400 JSON error; then safely
extract images and usage from the validated body.
Comment on lines
+46
to
+48
| const appUrl = (process.env.NEXT_PUBLIC_APP_URL ?? "https://straude.com").replace(/\/+$/, ""); | ||
| const params = new URLSearchParams({ code, verify_secret: verifySecret }); | ||
| const verifyUrl = `${appUrl}/cli/verify?${params.toString()}`; |
There was a problem hiding this comment.
Derive verify_url from the current request origin instead of a production fallback.
Line 46 falls back to https://straude.com when NEXT_PUBLIC_APP_URL is unset. In any staging or self-hosted deployment, packages/cli/src/commands/login.ts will open the browser on the wrong origin and send code plus verify_secret there. Use request.nextUrl.origin as the fallback, or fail fast if the app URL is required.
Suggested fix
- const appUrl = (process.env.NEXT_PUBLIC_APP_URL ?? "https://straude.com").replace(/\/+$/, "");
+ const appUrl = (process.env.NEXT_PUBLIC_APP_URL ?? request.nextUrl.origin).replace(/\/+$/, "");🤖 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/web/app/api/auth/cli/init/route.ts` around lines 46 - 48, The current
construction of appUrl uses NEXT_PUBLIC_APP_URL with a hardcoded production
fallback which can cause incorrect verify links; modify the logic that computes
appUrl (the code that sets appUrl and builds verifyUrl) to use
request.nextUrl.origin as the fallback when NEXT_PUBLIC_APP_URL is unset (or
alternatively throw an explicit error if an app URL must be provided), then
rebuild verifyUrl with that origin and the existing URLSearchParams (code and
verify_secret) so verifyUrl points to the actual request origin rather than
"https://straude.com".
Comment on lines
+5
to
+30
| UPDATE public.cli_auth_codes | ||
| SET status = 'expired' | ||
| WHERE status IN ('pending', 'completed') | ||
| AND expires_at > now(); | ||
|
|
||
| ALTER TABLE public.cli_auth_codes | ||
| ADD COLUMN IF NOT EXISTS poll_secret_hash text, | ||
| ADD COLUMN IF NOT EXISTS verify_secret_hash text, | ||
| ADD COLUMN IF NOT EXISTS redeemed_at timestamptz; | ||
|
|
||
| ALTER TABLE public.cli_auth_codes | ||
| DROP CONSTRAINT IF EXISTS cli_auth_codes_status_check; | ||
|
|
||
| ALTER TABLE public.cli_auth_codes | ||
| ADD CONSTRAINT cli_auth_codes_status_check | ||
| CHECK (status IN ('pending', 'completed', 'expired', 'used')); | ||
|
|
||
| ALTER TABLE public.cli_auth_codes | ||
| DROP CONSTRAINT IF EXISTS cli_auth_codes_active_secrets_check; | ||
|
|
||
| ALTER TABLE public.cli_auth_codes | ||
| ADD CONSTRAINT cli_auth_codes_active_secrets_check | ||
| CHECK ( | ||
| status = 'expired' | ||
| OR (poll_secret_hash IS NOT NULL AND verify_secret_hash IS NOT NULL) | ||
| ); |
There was a problem hiding this comment.
Backfill legacy cli_auth_codes rows before validating the new checks.
This sequence can fail on existing data. status = 'expired' is written before cli_auth_codes_status_check is widened to allow expired, and the later cli_auth_codes_active_secrets_check will still reject any legacy pending / completed / used row that has null secret hashes. Reorder this so the new columns exist first, the status check is widened, all pre-secret rows are marked terminal or backfilled, and only then is the stricter secrets check validated.
🤖 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 `@supabase/migrations/20260609200740_security_sweep_cli_auth_rate_limits.sql`
around lines 5 - 30, The migration updates on table cli_auth_codes are ordered
such that status is changed to 'expired' before the cli_auth_codes_status_check
is widened and before new columns poll_secret_hash and verify_secret_hash exist,
which can fail; reorder operations: first ALTER TABLE to ADD COLUMN IF NOT
EXISTS poll_secret_hash, verify_secret_hash, redeemed_at, then DROP and re-ADD
the cli_auth_codes_status_check to include 'expired', then backfill or UPDATE
legacy rows (e.g., set status = 'expired' or populate secret hashes) so no rows
violate the new logic, and only after that DROP/ADD the
cli_auth_codes_active_secrets_check to enforce the stricter secret-nonnull
constraint.
Comment on lines
+87
to
+88
| DELETE FROM public.api_rate_limits | ||
| WHERE expires_at < v_now - interval '1 hour'; |
There was a problem hiding this comment.
Avoid global row pruning on every rate-limit check.
Every RPC call does a table-wide DELETE before touching the caller's bucket. Since this function now fronts multiple hot endpoints, that turns rate limiting into shared cleanup work and will create unnecessary write/VACUUM churn under load. Move pruning out of the request path or make it opportunistic instead of unconditional.
🤖 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 `@supabase/migrations/20260609200740_security_sweep_cli_auth_rate_limits.sql`
around lines 87 - 88, The unconditional table-wide prune using "DELETE FROM
public.api_rate_limits WHERE expires_at < v_now - interval '1 hour';" must be
removed from the request path; instead either move this cleanup to an
out-of-band job (cron/pg_cron/worker) that runs periodically for
public.api_rate_limits, or make the prune opportunistic inside the RPC by
replacing the global DELETE with a bounded/conditional operation (e.g. DELETE
... WHERE expires_at < v_now - interval '1 hour' ORDER BY expires_at LIMIT <N>
or DELETE only for the caller's bucket ids), ensuring you reference the
expires_at/v_now logic and avoid full-table writes from the hot RPC.
ohong
force-pushed
the
codex/security-sweep
branch
from
89143f6 to
0370aa3
Compare
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.
Summary
This PR implements the coordinated security sweep for the CLI login flow, public database exposure, durable abuse limits, expensive request hardening, AI caption image validation, and vulnerable dependency paths.
What changed
public.cli_auth_codesthrough a Supabase migration.api_rate_limitstable and atomiccheck_rate_limitRPC.post-imagesstorage URLs.Impact
Existing CLI login clients that only send
{ code }to/api/auth/cli/pollare intentionally no longer supported. Current CLI login stores the newpoll_secretin memory and includes it on every poll.Root Cause
The previous device-login code was redeemable by code alone, and
cli_auth_codesstill had public client access paths. Rate limiting was also process-local, so it did not survive serverless instance churn. Several high-cost routes could do too much work before durable abuse checks or strict input caps.Validation
bun install --frozen-lockfilebun audit --jsonbun run typecheckbun run lintbun run testBEGIN ... ROLLBACK, including acheck_rate_limitfirst-call allowed / second-call denied checkgit diff --checkNotes
bun --cwd apps/web test:integrationwas not run locally because the current local Supabase migration history contains applied migration20260602000000, but that migration file is not present in this checkout. I did not repair local migration history automatically.Summary by CodeRabbit
New Features
Security & Validation
Tests
Chores