Conversation
See comment in cookie.ts for more information
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdates UI styling for global error display, changes payments app configuration from stable to beta, and refactors cookie management APIs to require explicit options parameters and add partitioned cookie support with environment-aware handling for browser and server contexts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 0
🧹 Nitpick comments (5)
packages/template/src/lib/cookie.ts (5)
267-269: Remove debug console.log statement.This
console.logstatement will pollute production logs. Consider removing it or using a conditional debug flag.if (cookies1.some((c) => c.startsWith(cookie1Name + "="))) { - console.log("not using partitioned cookies because we can set & retrieve normal cookies"); return false; }
288-289: Remove debug console.log statement.This debug log should be removed before merging to production.
const numberOfCookiesWithThisName = cookies2.filter((c) => c.startsWith(cookie2Name + "=")).length; - console.log("numberOfCookiesWithThisName", numberOfCookiesWithThisName);
300-301: Remove debug console.log statement.This logging statement exposes cookie values and options in the console, which may be undesirable in production.
const partitioned = shouldSetPartitionedClient(); - console.log("setting cookie on client", name, value, options, { secure, partitioned }); Cookies.set(name, value, {
272-273: Typo in comment: "ifnd" should be "find".- // TODO: See comment at the top. Feels like we should ifnd a better way to do this + // TODO: See comment at the top. Feels like we should find a better way to do this
254-296: Consider caching the partitioned cookie support check.
shouldSetPartitionedClient()performs multiple cookie read/write operations to detect partitioned cookie support. This function is called on everysetCookieClientInternalinvocation, which could be costly. Consider caching the result after the first check, as browser support for partitioned cookies won't change during a session.+let cachedPartitionedSupport: boolean | null = null; + function shouldSetPartitionedClient() { ensureClient(); + if (cachedPartitionedSupport !== null) { + return cachedPartitionedSupport; + } + if (!(determineSecureFromClientContext())) { + cachedPartitionedSupport = false; return false; } // ... rest of the function - return numberOfCookiesWithThisName === 2; + cachedPartitionedSupport = numberOfCookiesWithThisName === 2; + return cachedPartitionedSupport; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/next.config.mjs(0 hunks)apps/dashboard/src/app/global-error.tsx(1 hunks)packages/stack-shared/src/apps/apps-config.ts(1 hunks)packages/template/src/lib/cookie.ts(8 hunks)packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts(6 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/next.config.mjs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Always add new E2E tests when changing the API or SDK interface
For blocking alerts and errors, never use toast; use alerts instead as they are less easily missed by the user
NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error); use loading indicators and async callbacks instead, or use runAsynchronously/runAsynchronouslyWithAlert for error handling
Use ES6 maps instead of records wherever you can
Files:
packages/stack-shared/src/apps/apps-config.tsapps/dashboard/src/app/global-error.tsxpackages/template/src/lib/cookie.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,css}: Keep hover/click transitions snappy and fast; avoid fade-in delays on hover. Apply transitions after action completion instead, like smooth fade-out when hover ends
Use hover-exit transitions instead of hover-enter transitions; for example, use 'transition-colors hover:transition-none' instead of fade-in on hover
Files:
packages/stack-shared/src/apps/apps-config.tsapps/dashboard/src/app/global-error.tsxpackages/template/src/lib/cookie.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
{.env*,**/*.{ts,tsx,js}}
📄 CodeRabbit inference engine (AGENTS.md)
Prefix environment variables with STACK_ (or NEXT_PUBLIC_STACK_ if public) so changes are picked up by Turborepo and improves readability
Files:
packages/stack-shared/src/apps/apps-config.tsapps/dashboard/src/app/global-error.tsxpackages/template/src/lib/cookie.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if you can avoid them; prefer using client components and hooks like usePathname instead of await params to keep pages static
Files:
apps/dashboard/src/app/global-error.tsx
apps/dashboard/**/*
📄 CodeRabbit inference engine (AGENTS.md)
When making changes in the dashboard, provide the user with a deep link to the dashboard page changed, usually in the form of http://localhost:<NEXT_PUBLIC_STACK_PORT_PREFIX>01/projects/-selector-/... or using a.localhost, b.localhost, c.localhost for port prefixes 91, 92, 93
Files:
apps/dashboard/src/app/global-error.tsx
apps/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Check existing apps for inspiration when implementing new apps or pages; update apps-frontend.tsx and apps-config.ts to add new apps
Files:
apps/dashboard/src/app/global-error.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-04T18:03:49.961Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.961Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in packages/stack-shared/src/config/schema.ts
Applied to files:
packages/stack-shared/src/apps/apps-config.ts
📚 Learning: 2025-12-03T07:19:44.433Z
Learnt from: madster456
Repo: stack-auth/stack-auth PR: 1040
File: packages/stack-shared/src/interface/crud/oauth-providers.ts:62-87
Timestamp: 2025-12-03T07:19:44.433Z
Learning: In packages/stack-shared/src/interface/crud/oauth-providers.ts and similar CRUD files, the tag "Oauth" (not "OAuth") is the correct capitalization format as it's used by the documentation generation system and follows OpenAPI conventions.
Applied to files:
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
🧬 Code graph analysis (1)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
packages/template/src/lib/cookie.ts (5)
setCookie(352-355)setOrDeleteCookie(332-335)isSecure(228-236)deleteCookieClient(337-340)deleteCookie(342-345)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Vercel Agent Review
- GitHub Check: setup-tests
- GitHub Check: build (22.x)
- GitHub Check: all-good
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: build (22.x)
- GitHub Check: docker
- GitHub Check: build (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: restart-dev-and-test
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
packages/stack-shared/src/apps/apps-config.ts (1)
79-79: Verify the intent of downgrading payments from stable to beta.This change appears unrelated to the partitioned cookie support described in the PR title. Please confirm this is intentional.
apps/dashboard/src/app/global-error.tsx (1)
29-29: LGTM!Good improvement for centering the error UI. Note that this styling change appears unrelated to partitioned cookie support.
packages/template/src/lib/cookie.ts (2)
314-321: Deletion iterates over both partitioned states correctly.The loop over
[true, false]for the partitioned flag ensures cleanup of both partitioned and non-partitioned cookies with the same name.
7-65: Excellent documentation of cookie security logic.The detailed explanation of the Secure, SameSite, and Partitioned flag handling, including the heuristics and security considerations, is very helpful for maintainability.
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (3)
608-626: Good refactoring of cookie options into a shared object.The
cookieOptionsconstant centralizes the common options, improving maintainability and ensuring consistency across cookie operations.
679-681: LGTM!The update correctly uses the new
deleteCookieClientsignature with an empty options object.
736-741: LGTM!The server-side cookie deletion correctly passes
noOpIfServerComponent: trueto handle server component contexts gracefully.
Greptile OverviewGreptile SummaryThis PR implements CHIPS (Cookies Having Independent Partitioned State) support to enable cookies in third-party/iframe contexts, particularly for cross-origin embedded use cases. Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client as Browser Client
participant Cookie as cookie.ts
participant Helper as shouldSetPartitionedClient()
participant Server as Next.js Server
Note over Client,Server: Cookie Setting Flow (Browser)
Client->>Cookie: setCookieClient(name, value, options)
Cookie->>Cookie: determineSecureFromClientContext()
Cookie-->>Cookie: Check window.location.protocol
Cookie->>Helper: shouldSetPartitionedClient()
Helper->>Helper: Check if HTTPS (Secure required)
Note over Helper: Test 1: Can set normal cookies?
Helper->>Helper: Set test cookie without Partitioned
Helper->>Helper: Read document.cookie
alt Normal cookies work
Helper-->>Cookie: return false (no partitioning needed)
else Normal cookies blocked
Note over Helper: Test 2: Does browser support CHIPS?
Helper->>Helper: Set cookie WITH Partitioned
Helper->>Helper: Set cookie WITHOUT Partitioned
Helper->>Helper: Count cookies with same name
alt Two cookies exist
Helper-->>Cookie: return true (CHIPS supported)
else One cookie exists
Helper-->>Cookie: return false (CHIPS not supported)
end
end
Cookie->>Client: Cookies.set() with SameSite=Lax/None
Note over Client,Server: Cookie Setting Flow (Server)
Server->>Cookie: setCookie(name, value, options)
Cookie->>Cookie: createNextCookieHelper()
Cookie->>Cookie: determineSecureFromServerContext()
Cookie-->>Cookie: Check stack-is-https cookie or X-Forwarded-Proto header
Cookie->>Server: rscCookies.set() with Secure flag
Note over Cookie: Partitioned NOT set on server (complexity)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
apps/dashboard/next.config.mjs, line 109-112 (link)style: removed
X-Frame-Options: SAMEORIGINheader - verify this is intentional for partitioned cookie support in iframes
5 files reviewed, 6 comments
packages/template/src/lib/cookie.ts
Outdated
| // check if there are two cookies | ||
| const cookies2 = document.cookie.split("; "); | ||
| const numberOfCookiesWithThisName = cookies2.filter((c) => c.startsWith(cookie2Name + "=")).length; | ||
| console.log("numberOfCookiesWithThisName", numberOfCookiesWithThisName); |
There was a problem hiding this comment.
style: remove debug console.log before production
| console.log("numberOfCookiesWithThisName", numberOfCookiesWithThisName); | |
| // numberOfCookiesWithThisName check |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/template/src/lib/cookie.ts
Line: 289:289
Comment:
**style:** remove debug console.log before production
```suggestion
// numberOfCookiesWithThisName check
```
How can I resolve this? If you propose a fix, please make it concise.![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: X-Frame-Options header removed without clickjacking protection
The X-Frame-Options: SAMEORIGIN security header was removed but no replacement clickjacking protection is in place. The Content-Security-Policy header is set to an empty string, meaning there's no frame-ancestors directive to prevent the dashboard from being embedded in malicious iframes. This leaves the application vulnerable to clickjacking attacks where attackers could trick users into clicking hidden elements.
apps/dashboard/next.config.mjs#L108-L113
stack-auth/apps/dashboard/next.config.mjs
Lines 108 to 113 in 0f12171
There was a problem hiding this comment.
Pull request overview
This PR implements support for partitioned cookies (CHIPS - Cookies Having Independent Partitioned State) to enable cross-site cookie functionality in third-party contexts while maintaining security. The implementation includes comprehensive detection logic to ensure partitioned cookies are only set on browsers that support them, preventing unsafe fallback to third-party cookies with SameSite=None.
Key changes:
- Added sophisticated browser detection logic to determine when to use partitioned cookies based on HTTPS context and browser CHIPS support
- Updated cookie management functions to automatically set appropriate SameSite, Secure, and Partitioned flags
- Removed X-Frame-Options header to allow iframe embedding needed for third-party cookie scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/template/src/lib/cookie.ts | Core implementation of partitioned cookie support with detection heuristics, automatic flag setting, and comprehensive documentation |
| packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts | Updated cookie function calls to use new deleteCookie function and provide required options parameters |
| packages/stack-shared/src/apps/apps-config.ts | Changed payments app stage from "stable" to "beta" (appears unrelated to cookie changes) |
| apps/dashboard/src/app/global-error.tsx | Added styling for error page centering (appears unrelated to cookie changes) |
| apps/dashboard/next.config.mjs | Removed X-Frame-Options header to support iframe embedding for partitioned cookies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| subtitle: "Payment processing and subscription management", | ||
| tags: ["operations", "gtm"], | ||
| stage: "stable", | ||
| stage: "beta", |
There was a problem hiding this comment.
Bug: Payments stage downgraded from stable to beta unexpectedly
The payments feature stage is changed from stable to beta, which is a downgrade that appears unrelated to the PR's stated purpose of "Partitioned cookie support". Downgrading a feature from stable to beta is unusual and could affect users relying on the stable payments feature. This change may have been accidentally committed from a development or testing branch, as it doesn't align with the PR's scope.
1 participant
See comment in cookie.ts for more information
Note
Adds partitioned cookie support with HTTPS detection and SameSite defaults; refactors token cookie handling; minor UI centering and payments app set to beta.
SameSite=Laxby default (orNonewhen partitioned) and HTTPS-basedSecuredetection (via helper cookie andx-forwarded-proto).maxAgesupports"session"; standardize options (no implicit defaults); set long-lived helperstack-is-httpscookie.sameSite, correctmaxAge/expires, and error handling.deleteCookie(...)instead ofsetOrDeleteCookie(..., null), and ensure deletions pass explicit options.apps/dashboard/src/app/global-error.tsx.paymentsapp stage tobetainpackages/stack-shared/src/apps/apps-config.ts.Written by Cursor Bugbot for commit d8b8082. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Updates
✏️ Tip: You can customize this high-level summary in your review settings.