auth: support Vercel preview origins - #239
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change centralizes canonical URL resolution, adds Vercel fallbacks and validation, refactors OAuth metadata and loopback handling, tightens CSRF origin checks, and adds tests and documentation for the updated behavior. ChangesCanonical URL resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Prefer PUBLIC_URL for OAuth callbacks, then VERCEL_BRANCH_URL and VERCEL_URL. Allow same-origin CSRF requests on each exact configured deployment origin without making those origins cross-origin peers. Co-Authored-By: OpenAI Codex <noreply@openai.com>
b992520 to
e1d0668
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/utils/config.ts (1)
105-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isLoopbackHostis duplicated verbatim insrc/lib/auth/csrf.ts(lines 105-107).Same three-hostname check defined independently in both files. Since this predicate underpins both
PUBLIC_URLnormalization here and the CSRF dev-loopback exception incsrf.ts, keeping two copies risks silent divergence (e.g., one gets updated to add0.0.0.0or IPv6 variants and the other doesn't), which would create an inconsistency between what's accepted as a "loopback" origin at config time vs. request time.♻️ Proposed fix — export once, import elsewhere
-function isLoopbackHost(hostname: string): boolean { +export function isLoopbackHost(hostname: string): boolean { return hostname === "localhost" || hostname === "127.0.0.1" || hostname === "[::1]" }Then in
src/lib/auth/csrf.ts:-import { ALLOWED_REQUEST_ORIGINS } from "`@/lib/utils/config`" +import { ALLOWED_REQUEST_ORIGINS, isLoopbackHost } from "`@/lib/utils/config`" ... -function isLoopbackHost(hostname: string): boolean { - return hostname === "localhost" || hostname === "127.0.0.1" || hostname === "[::1]" -}🤖 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 `@src/lib/utils/config.ts` around lines 105 - 112, Export the existing isLoopbackHost predicate from config.ts and remove the duplicate definition in csrf.ts. Import and reuse that shared symbol in csrf.ts so PUBLIC_URL normalization and the CSRF loopback exception use the same hostname checks.src/lib/auth/csrf.ts (1)
82-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isLoopbackHostduplicates the identical helper insrc/lib/utils/config.ts(lines 110-112).Same three-way hostname check maintained in two places. Export it once from
config.tsand import here to avoid drift on this security-relevant predicate.🤖 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 `@src/lib/auth/csrf.ts` around lines 82 - 107, Remove the local isLoopbackHost helper from csrf.ts and reuse the identical exported helper from config.ts. Export isLoopbackHost in config.ts, import it into csrf.ts, and keep isAllowedDevelopmentLoopback’s existing checks and behavior unchanged.
🤖 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.
Nitpick comments:
In `@src/lib/auth/csrf.ts`:
- Around line 82-107: Remove the local isLoopbackHost helper from csrf.ts and
reuse the identical exported helper from config.ts. Export isLoopbackHost in
config.ts, import it into csrf.ts, and keep isAllowedDevelopmentLoopback’s
existing checks and behavior unchanged.
In `@src/lib/utils/config.ts`:
- Around line 105-112: Export the existing isLoopbackHost predicate from
config.ts and remove the duplicate definition in csrf.ts. Import and reuse that
shared symbol in csrf.ts so PUBLIC_URL normalization and the CSRF loopback
exception use the same hostname checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7974ce32-fffb-4cd3-84bf-05b25d26a1dd
📒 Files selected for processing (9)
.env.local.exampleAGENTS.mdREADME.mdsrc/lib/auth/__tests__/csrf.test.tssrc/lib/auth/__tests__/oauth-config.test.tssrc/lib/auth/csrf.tssrc/lib/auth/oauth-client.tssrc/lib/utils/__tests__/config.test.tssrc/lib/utils/config.ts
Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
(reply generated by OpenAI Codex) Addressed the duplicated Validated with 1,200 Vitest tests, source and test typechecks, ESLint, and |
|
(reply generated by OpenAI Codex) Regarding the docstring-coverage warning in CodeRabbit’s PR comment: leaving the remaining private helpers undocumented. The exported OAuth/config contracts introduced by this PR have focused contract comments; adding narration solely to meet a generated percentage would conflict with the repository’s comment policy and is outside this fix. |
Summary
PUBLIC_URL→VERCEL_BRANCH_URL→VERCEL_URLBehavior
When
PUBLIC_URLexists, OAuth metadata and callbacks continue using it. Otherwise the stable Vercel branch URL is used, with the commit deployment URL as the final fallback. A login started on another accepted Vercel origin completes on the selected canonical origin and receives its session cookie there.Review documents
Breaking changes
None.
Out of scope
*.vercel.apptrustclient_idnamespacingRollout constraints
client_idTest plan
npm test— 145 files, 1200 tests passednpx tsc --noEmitnpx eslint src/ --ext .ts,.tsxnpx next buildgit diff --check origin/staging..HEADSummary by CodeRabbit
Security Improvements
OAuth & Configuration
Documentation
Tests