Fix cross-subdomain cookie deletion and prefetch trusted parent domain#1302
Merged
Conversation
Cross-subdomain refresh cookies were not being deleted correctly because the domain option was not passed to deleteCookie/deleteCookieClient. This caused stale cookies to accumulate and auth state to persist across subdomains after sign-out. Also eagerly warms the trusted parent domain cache on app construction to avoid a race condition where navigation after sign-in could prevent the cross-subdomain cookie from being written.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughClient constructor now pre-warms a trusted-parent-domain cache in-browser for cookie-based token stores, decodes base32-encoded custom refresh-cookie names to derive parent domains, creates missing cross-subdomain refresh cookies, and uses decoded domains when deleting custom refresh cookies (browser and nextjs-cookie server flows). Changes
Sequence Diagram(s)sequenceDiagram
participant ClientApp as ClientApp
participant Browser as Browser
participant TokenStore as TokenStore
participant Server as Server
ClientApp->>Browser: constructor (browser && tokenStore = "cookie"|"nextjs-cookie")
ClientApp->>TokenStore: pre-warm _trustedParentDomainCache(window.location.hostname)
ClientApp->>ClientApp: _ensureCrossSubdomainCookieExists()
Note over ClientApp,TokenStore: cached trusted parent domain used for cookie scoping
Browser->>ClientApp: token-store change / onChange -> delete custom refresh cookies
ClientApp->>ClientApp: _getDomainFromCustomRefreshCookieName(name)
alt domain decoded
ClientApp->>Browser: deleteCookieClient(name, { domain })
else not decoded
ClientApp->>Browser: deleteCookieClient(name)
end
Server->>ClientApp: server-side store.onChange (nextjs-cookie)
ClientApp->>ClientApp: _getDomainFromCustomRefreshCookieName(name)
alt domain decoded
ClientApp->>Server: deleteCookieServer(name, { noOpIfServerComponent:true, domain })
else not decoded
ClientApp->>Server: deleteCookieServer(name, { noOpIfServerComponent:true })
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
626-634: Optional: normalize decoded domain to lowercase for canonical consistency.You already lowercase before encoding in
_getCustomRefreshCookieName; mirroring that on decode avoids mixed-case edge behavior from non-canonical cookie names.♻️ Suggested tweak
private _getDomainFromCustomRefreshCookieName(name: string): string | null { const prefix = `${this._refreshTokenCookieName}--custom-`; if (!name.startsWith(prefix)) return null; try { - return new TextDecoder().decode(decodeBase32(name.slice(prefix.length))); + return new TextDecoder().decode(decodeBase32(name.slice(prefix.length))).toLowerCase(); } catch { return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts` around lines 626 - 634, The decoded domain returned by _getDomainFromCustomRefreshCookieName should be normalized to lowercase for consistency with _getCustomRefreshCookieName; after decoding (inside the try block) call .toLowerCase() on the decoded string before returning it so the function always returns a canonical lowercase domain (still returning null on failures).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 626-634: The decoded domain returned by
_getDomainFromCustomRefreshCookieName should be normalized to lowercase for
consistency with _getCustomRefreshCookieName; after decoding (inside the try
block) call .toLowerCase() on the decoded string before returning it so the
function always returns a canonical lowercase domain (still returning null on
failures).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b006e776-69d1-4166-9265-356dab6a34d2
📒 Files selected for processing (1)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Contributor
Greptile SummaryThis PR fixes two related cross-subdomain cookie issues in Changes:
The logic is sound: non-custom cookies (legacy names, Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as StackClientApp (constructor)
participant Cache as _trustedParentDomainCache
participant API as getClientProject API
participant Store as BrowserCookieTokenStore
participant Cookie as deleteCookieClient / deleteCookie
App->>Cache: getOrWait([hostname], "write-only") [eager warm-up]
Cache->>API: _getTrustedParentDomain(hostname)
API-->>Cache: parentDomain (e.g. "example.com")
Note over App,Store: On sign-out / token update
Store->>Store: _prepareRefreshCookieUpdate()<br/>→ cookieNamesToDelete = [custom cookie names]
loop For each name in cookieNamesToDelete
Store->>Store: _getDomainFromCustomRefreshCookieName(name)<br/>→ decodes base32 → domain string or null
Store->>Cookie: deleteCookieClient(name, { domain } or {})
end
Cookie-->>Store: Cookie deleted (with correct domain scope)
Reviews (1): Last reviewed commit: "Fix cross-subdomain cookie deletion and ..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/e2e/tests/js/cookies.test.ts (1)
317-318: Avoidas anyaccess for private method calls in tests.Line 317, Line 318, and Lines 327-329 bypass type safety. Prefer a typed exported helper (or a test-only public wrapper) so these tests don’t rely on
anycasts.As per coding guidelines: “Do NOT use
as/any/type casts or anything else to bypass the type system unless you specifically asked the user about it.” and “Try to avoid theanytype.”Also applies to: 327-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/js/cookies.test.ts` around lines 317 - 318, Tests are using unsafe casts to call private methods on clientApp (_getCustomRefreshCookieName and _getDomainFromCustomRefreshCookieName); instead, add a small typed test helper or a test-only public wrapper that exposes these behaviors (e.g., export a getCustomRefreshCookieNameForTest or expose a ClientApp.testHelpers.getDomainFromCookieName) and update the test to call that typed helper instead of using (clientApp as any), ensuring the helper returns proper TypeScript types for cookieName and decoded domain so the tests no longer bypass the type system.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/tests/js/cookies.test.ts`:
- Around line 323-330: Add a test case to exercise the decode-failure branch for
custom-prefixed cookie names in the existing spec: when calling (clientApp as
any)._getDomainFromCustomRefreshCookieName with a malformed custom cookie like
`stack-refresh-${clientApp.projectId}--custom-%%%` it should return null; update
the "should return null for non-custom refresh cookie names" test (which already
uses createApp() and getDefaultRefreshCookieName) to include this malformed
`--custom-` case and assert .toBeNull() to cover the guarded decode error path.
---
Nitpick comments:
In `@apps/e2e/tests/js/cookies.test.ts`:
- Around line 317-318: Tests are using unsafe casts to call private methods on
clientApp (_getCustomRefreshCookieName and
_getDomainFromCustomRefreshCookieName); instead, add a small typed test helper
or a test-only public wrapper that exposes these behaviors (e.g., export a
getCustomRefreshCookieNameForTest or expose a
ClientApp.testHelpers.getDomainFromCookieName) and update the test to call that
typed helper instead of using (clientApp as any), ensuring the helper returns
proper TypeScript types for cookieName and decoded domain so the tests no longer
bypass the type system.
🪄 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: cc825b9b-035b-45d0-9f8d-2187cae54d04
📒 Files selected for processing (1)
apps/e2e/tests/js/cookies.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/tests/js/cookies.test.ts`:
- Around line 361-428: The test only deletes the custom cookie but doesn't seed
the legacy refresh-cookie names used by older clients; before constructing the
new StackClientApp instance (the reloadedApp), also set either
getDefaultRefreshCookieName(clientApp.projectId, false) or the legacy
"stack-refresh" (or "stack-refresh-${projectId}") with the signed-in refresh
token value (use the parsed.refresh_token and updated_at_millis as in
defaultValue) so the constructor can run its migration; then assert
cookieStore.has(customCookieName) becomes true after construction (same
waitUntil/customRecreated assertion). Locate symbols: StackClientApp,
getDefaultRefreshCookieName, getCustomRefreshCookieName, cookieStore, waitUntil.
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 627-635: The _getDomainFromCustomRefreshCookieName function
currently returns any decodable payload as a domain; change it to normalize and
validate the decoded hostname before returning: after decoding via
decodeBase32(name.slice(prefix.length)) and TextDecoder(), trim/normalize
(lowercase, remove surrounding dots/spaces), then validate against a
cookie-domain pattern (e.g., host label rules or a strict regex that allows
optional leading dot and valid hostname characters/length) and return null if it
does not match; keep references to this._refreshTokenCookieName and decodeBase32
in your implementation so callers at lines that pass the result to cookie
helpers receive only validated domains.
- Around line 792-796: _extractRefreshTokenFromCookieMap currently returns
updatedAt: null for legacy cookie names, so the constructor skips migration
because the code requires updatedAt; change the check to migrate when
refreshToken exists even if updatedAt is null: if refreshToken is present, set a
fallback updatedAt (e.g., Date.now()) before calling
this._formatRefreshCookieValue(refreshToken, updatedAtFallback) and then call
setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365,
domain: domain.data }); add a "// TODO next-release" comment near this fallback
so the shim can be removed in the next release; keep references to
_extractRefreshTokenFromCookieMap, _formatRefreshCookieValue,
setOrDeleteCookieClient and customCookieName to locate the change.
🪄 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: a93c1366-0b04-4aef-a3dd-5cdf2fd9f7d4
📒 Files selected for processing (2)
apps/e2e/tests/js/cookies.test.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Comment on lines
+361
to
+428
| it("should eagerly create cross-subdomain cookie on construction when session exists but custom cookie is missing", async ({ expect }) => { | ||
| const { cookieStore } = setupBrowserCookieEnv({ protocol: "https:" }); | ||
|
|
||
| const { clientApp, apiKey } = await createApp( | ||
| { | ||
| config: { | ||
| domains: [ | ||
| { domain: "https://example.com", handlerPath: "/handler" }, | ||
| { domain: "https://**.example.com", handlerPath: "/handler" }, | ||
| ], | ||
| }, | ||
| }, | ||
| { | ||
| client: { | ||
| tokenStore: "cookie", | ||
| noAutomaticPrefetch: true, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| // Sign in to get a valid session | ||
| const email = `${crypto.randomUUID()}@eager-cookie.test`; | ||
| const password = "password"; | ||
| await clientApp.signUpWithCredential({ email, password, verificationCallbackUrl: "http://localhost:3000", noRedirect: true }); | ||
| await clientApp.signInWithCredential({ email, password, noRedirect: true }); | ||
|
|
||
| const defaultCookieName = getDefaultRefreshCookieName(clientApp.projectId, true); | ||
| const customCookieName = getCustomRefreshCookieName(clientApp.projectId, "example.com"); | ||
|
|
||
| // Wait for the cross-subdomain cookie to be written | ||
| const customReady = await waitUntil(() => cookieStore.has(customCookieName), 10_000); | ||
| expect(customReady).toBe(true); | ||
|
|
||
| // Grab the refresh token before we manipulate cookies | ||
| const customCookieValue = cookieStore.get(customCookieName)!; | ||
| const parsed = JSON.parse(decodeURIComponent(customCookieValue)); | ||
|
|
||
| // Simulate state where user was signed in before wildcard domain was added: | ||
| // default cookie exists with the session, but no cross-subdomain cookie | ||
| cookieStore.delete(customCookieName); | ||
| const defaultValue = encodeURIComponent(JSON.stringify({ | ||
| refresh_token: parsed.refresh_token, | ||
| updated_at_millis: parsed.updated_at_millis, | ||
| })); | ||
| cookieStore.set(defaultCookieName, defaultValue); | ||
|
|
||
| expect(cookieStore.has(customCookieName)).toBe(false); | ||
| expect(cookieStore.has(defaultCookieName)).toBe(true); | ||
|
|
||
| // Construct a new client app (simulates page reload) | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| const reloadedApp = new StackClientApp({ | ||
| baseUrl: STACK_BACKEND_BASE_URL, | ||
| projectId: clientApp.projectId, | ||
| publishableClientKey: apiKey.publishableClientKey, | ||
| tokenStore: "cookie", | ||
| redirectMethod: "none", | ||
| noAutomaticPrefetch: true, | ||
| extraRequestHeaders: { "x-stack-disable-artificial-development-delay": "yes" }, | ||
| }); | ||
|
|
||
| // The cross-subdomain cookie should be eagerly created on construction | ||
| const customRecreated = await waitUntil(() => cookieStore.has(customCookieName), 10_000); | ||
| expect(customRecreated).toBe(true); | ||
|
|
||
| // Clean up | ||
| (reloadedApp as any).dispose?.(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add a regression for legacy-cookie upgrade on app construction.
This scenario only deletes the custom cookie. It does not cover the supported legacy refresh-cookie names, which are the sessions most likely to rely on this constructor-time migration path. Seed stack-refresh-${projectId} or stack-refresh with the signed-in refresh token and assert that constructing a new app recreates the parent-domain custom cookie.
Based on learnings: Applies to **/apps/e2e/**/*.{ts,tsx} — ALWAYS add new E2E tests when you change the API or SDK interface. Generally, err on the side of creating too many tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e/tests/js/cookies.test.ts` around lines 361 - 428, The test only
deletes the custom cookie but doesn't seed the legacy refresh-cookie names used
by older clients; before constructing the new StackClientApp instance (the
reloadedApp), also set either getDefaultRefreshCookieName(clientApp.projectId,
false) or the legacy "stack-refresh" (or "stack-refresh-${projectId}") with the
signed-in refresh token value (use the parsed.refresh_token and
updated_at_millis as in defaultValue) so the constructor can run its migration;
then assert cookieStore.has(customCookieName) becomes true after construction
(same waitUntil/customRecreated assertion). Locate symbols: StackClientApp,
getDefaultRefreshCookieName, getCustomRefreshCookieName, cookieStore, waitUntil.
Comment on lines
+627
to
+635
| private _getDomainFromCustomRefreshCookieName(name: string): string | null { | ||
| const prefix = `${this._refreshTokenCookieName}--custom-`; | ||
| if (!name.startsWith(prefix)) return null; | ||
| try { | ||
| return new TextDecoder().decode(decodeBase32(name.slice(prefix.length))); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate the decoded hostname before returning it.
Line 631 trusts any decodable payload as a hostname. A --custom- suffix whose base32 payload decodes to something like bad domain will currently come back as a non-null string, and Lines 893 and 953 then pass it straight into the cookie helpers as domain. Normalize the decoded value and return null unless it matches a real cookie-domain shape.
As per coding guidelines: Any assumption you make should either be validated through the type system (preferred), assertions, or tests. Optimally, two out of three.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`
around lines 627 - 635, The _getDomainFromCustomRefreshCookieName function
currently returns any decodable payload as a domain; change it to normalize and
validate the decoded hostname before returning: after decoding via
decodeBase32(name.slice(prefix.length)) and TextDecoder(), trim/normalize
(lowercase, remove surrounding dots/spaces), then validate against a
cookie-domain pattern (e.g., host label rules or a strict regex that allows
optional leading dot and valid hostname characters/length) and return null if it
does not match; keep references to this._refreshTokenCookieName and decodeBase32
in your implementation so callers at lines that pass the result to cookie
helpers receive only validated domains.
Comment on lines
+792
to
+796
| const { refreshToken, updatedAt } = this._extractRefreshTokenFromCookieMap(cookies); | ||
| if (refreshToken && updatedAt) { | ||
| const value = this._formatRefreshCookieValue(refreshToken, updatedAt); | ||
| setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365, domain: domain.data }); | ||
| } |
Contributor
There was a problem hiding this comment.
Migrate legacy refresh cookies into the new cross-subdomain format.
Line 793 requires updatedAt, but _extractRefreshTokenFromCookieMap() returns updatedAt: null for the still-supported legacy refresh-cookie names. That means already-signed-in users coming from stack-refresh-${projectId} or stack-refresh will skip this constructor-time repair and keep missing the parent-domain cookie until the next token write or a full sign-in.
💡 Suggested migration-friendly fix
const { refreshToken, updatedAt } = this._extractRefreshTokenFromCookieMap(cookies);
- if (refreshToken && updatedAt) {
- const value = this._formatRefreshCookieValue(refreshToken, updatedAt);
- setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365, domain: domain.data });
- }
+ if (!refreshToken) {
+ return;
+ }
+ // TODO next-release: remove the legacy-cookie timestamp fallback once all active sessions use structured refresh cookies
+ const value = this._formatRefreshCookieValue(refreshToken, updatedAt ?? Date.now());
+ setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365, domain: domain.data });Based on learnings: In the stack-auth/stack-auth codebase, the convention // TODO next-release is used to mark temporary backward-compatibility shims that only need to survive a single release cycle and can be safely removed in the next release.
📝 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
| const { refreshToken, updatedAt } = this._extractRefreshTokenFromCookieMap(cookies); | |
| if (refreshToken && updatedAt) { | |
| const value = this._formatRefreshCookieValue(refreshToken, updatedAt); | |
| setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365, domain: domain.data }); | |
| } | |
| const { refreshToken, updatedAt } = this._extractRefreshTokenFromCookieMap(cookies); | |
| if (!refreshToken) { | |
| return; | |
| } | |
| // TODO next-release: remove the legacy-cookie timestamp fallback once all active sessions use structured refresh cookies | |
| const value = this._formatRefreshCookieValue(refreshToken, updatedAt ?? Date.now()); | |
| setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365, domain: domain.data }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`
around lines 792 - 796, _extractRefreshTokenFromCookieMap currently returns
updatedAt: null for legacy cookie names, so the constructor skips migration
because the code requires updatedAt; change the check to migrate when
refreshToken exists even if updatedAt is null: if refreshToken is present, set a
fallback updatedAt (e.g., Date.now()) before calling
this._formatRefreshCookieValue(refreshToken, updatedAtFallback) and then call
setOrDeleteCookieClient(customCookieName, value, { maxAge: 60 * 60 * 24 * 365,
domain: domain.data }); add a "// TODO next-release" comment near this fallback
so the shim can be removed in the next release; keep references to
_extractRefreshTokenFromCookieMap, _formatRefreshCookieValue,
setOrDeleteCookieClient and customCookieName to locate the change.
N2D4
approved these changes
Apr 2, 2026
Comment on lines
+793
to
+794
| if (refreshToken && updatedAt) { | ||
| const value = this._formatRefreshCookieValue(refreshToken, updatedAt); |
There was a problem hiding this comment.
Suggested change
| if (refreshToken && updatedAt) { | |
| const value = this._formatRefreshCookieValue(refreshToken, updatedAt); | |
| if (refreshToken) { | |
| const value = this._formatRefreshCookieValue(refreshToken, updatedAt ?? Date.now()); |
Users with legacy cookies (like stack-refresh or the legacy refresh token cookie) never get cross-subdomain cookies created because _ensureCrossSubdomainCookieExists requires updatedAt to be truthy, but legacy cookies return updatedAt: null.
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
2 participants
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.
Cross-subdomain refresh cookies were not being deleted correctly because the domain option was not passed to deleteCookie/deleteCookieClient. This caused stale cookies to accumulate and auth state to persist across subdomains after sign-out.
Also eagerly warms the trusted parent domain cache on app construction to avoid a race condition where navigation after sign-in could prevent the cross-subdomain cookie from being written.
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests