Cross domain handoffs#1458
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes redirect-URL validation, enforces relative oauthCallback targets, adds hosted-aware OAuth callback detection/URIs, implements nested cross-domain auth handoffs with pending auth-resolution tracking, and updates client/backend wiring, tests, demo config, and docs. ChangesHosted OAuth callbacks and nested cross-domain auth
Sequence DiagramsequenceDiagram
participant Browser
participant HostedAuth as Hosted Auth Server
participant CallbackPage as Hosted Callback Page
participant ClientApp
participant TokenStore as Persistent Token Store
participant SessionConsumer
Browser->>HostedAuth: authenticate -> redirect with code/state
HostedAuth->>Browser: redirect to hosted callback URL (code/state)
Browser->>CallbackPage: load hosted callback
CallbackPage->>ClientApp: callOAuthCallback(options)
ClientApp->>ClientApp: _trackPendingAuthResolution (promise)
ClientApp->>TokenStore: exchange code -> tokens
TokenStore->>ClientApp: tokens stored
ClientApp->>ClientApp: resolve pending auth promise
SessionConsumer->>ClientApp: _getSession(awaitPendingAuthResolutions: true)
ClientApp->>ClientApp: _awaitPendingAuthResolutions()
ClientApp->>SessionConsumer: return session
ClientApp->>Browser: if nested params -> build nested redirect and navigate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 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 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.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for cross-domain “hosted callback” and nested cross-domain auth handoffs, while centralizing redirect URL trust validation.
Changes:
- Enforce relative OAuth callback handler targets in
resolveHandlerUrls, with unit coverage. - Add hosted OAuth callback redirect-URI selection and nested cross-domain handoff flow in the client app implementation.
- Introduce shared redirect URL validation utilities and reuse them in the backend tenancy redirect validation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/template/src/lib/stack-app/url-targets.ts | Adds validation that OAuth callback targets must be relative. |
| packages/template/src/lib/stack-app/url-targets.test.ts | Adds tests ensuring absolute OAuth callback targets are rejected. |
| packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts | Implements hosted callback redirect URI logic, pending auth-resolution gating, nested cross-domain auth flow, and shared trusted-URL validation. |
| packages/template/src/lib/auth.ts | Adds option to suppress warnings when OAuth callback params are missing; threads option through callback handling. |
| packages/stack-shared/src/utils/redirect-urls.tsx | New shared helpers for redirect URL validation + trusted parent domain derivation. |
| packages/stack-shared/src/interface/crud/projects.ts | Exposes allow_localhost on client project config schema. |
| examples/demo/src/stack.tsx | Updates demo to use hosted default handler target. |
| apps/e2e/tests/js/cross-domain-auth.test.ts | Updates existing expectations and adds new nested cross-domain auth E2E coverage. |
| apps/backend/src/lib/redirect-urls.tsx | Replaces local redirect validation logic with shared stack-shared implementation. |
| .claude/CLAUDE-KNOWLEDGE.md | Documents the new “pending auth resolution” gating approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/stack-shared/src/utils/redirect-urls.tsx (1)
4-7: 💤 Low valueAlign
trustedDomainselement type across exported APIs.
TrustedDomainConfig.trustedDomainsisreadonly (string | null | undefined)[]butgetTrustedParentDomaintakesreadonly string[]. Callers reading trusted domains from a single source end up either coercing or filtering at the call site. Unifying the element type (and filtering nulls inside the function, as already done invalidateRedirectUrl) would simplify usage.♻️ Suggested signature unification
-export function getTrustedParentDomain(currentDomain: string, trustedDomains: readonly string[]): string | null { +export function getTrustedParentDomain( + currentDomain: string, + trustedDomains: readonly (string | null | undefined)[], +): string | null { const hostPatterns = trustedDomains + .filter((domain): domain is string => domain != null) .map((domain) => {Also applies to: 88-88
🤖 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 `@packages/stack-shared/src/utils/redirect-urls.tsx` around lines 4 - 7, TrustedDomainConfig.trustedDomains uses readonly (string | null | undefined)[] but getTrustedParentDomain currently expects readonly string[]; unify the element type by changing getTrustedParentDomain's trusted domains parameter to accept readonly (string | null | undefined)[] (same as TrustedDomainConfig.trustedDomains) and perform internal filtering of null/undefined values inside getTrustedParentDomain (mirroring validateRedirectUrl) so callers don’t need to coerce or filter; update any other exported APIs that accept trusted domain arrays to use the unified element type as well (e.g., the other occurrence referenced alongside getTrustedParentDomain).packages/template/src/lib/stack-app/url-targets.test.ts (1)
99-115: 💤 Low valuePrefer
toThrowErrorMatchingInlineSnapshotfor the new assertions.Both new cases assert via
.toThrowError(/regex/). Consider snapshotting the error message instead so that intent and message stay in sync. As per coding guidelines: "When writing tests, prefer.toMatchInlineSnapshotover other selectors in Vitest tests."♻️ Proposed change
it("rejects absolute OAuth callback string targets", () => { - expect(() => resolveHandlerUrls({ - projectId: "project-id", - urls: { - oauthCallback: "https://app.example.test/oauth-callback", - }, - })).toThrowError(/OAuth callback URLs must be relative/); + expect(() => resolveHandlerUrls({ + projectId: "project-id", + urls: { + oauthCallback: "https://app.example.test/oauth-callback", + }, + })).toThrowErrorMatchingInlineSnapshot(); }); it("rejects absolute OAuth callback custom targets", () => { - expect(() => resolveHandlerUrls({ - projectId: "project-id", - urls: { - oauthCallback: { type: "custom", url: "https://app.example.test/oauth-callback", version: 0 }, - }, - })).toThrowError(/OAuth callback URLs must be relative/); + expect(() => resolveHandlerUrls({ + projectId: "project-id", + urls: { + oauthCallback: { type: "custom", url: "https://app.example.test/oauth-callback", version: 0 }, + }, + })).toThrowErrorMatchingInlineSnapshot(); });🤖 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 `@packages/template/src/lib/stack-app/url-targets.test.ts` around lines 99 - 115, Replace the regex-based assertions in the two tests that call resolveHandlerUrls with snapshot assertions: change the expect(...).toThrowError(/OAuth callback URLs must be relative/) calls to use toThrowErrorMatchingInlineSnapshot and capture the exact error message string produced by resolveHandlerUrls (for both the string target and the { type: "custom", url, version } case) so the tests assert the precise error text; update the two test cases (the "rejects absolute OAuth callback string targets" and "rejects absolute OAuth callback custom targets") to call toThrowErrorMatchingInlineSnapshot and record the inline snapshot of the thrown error message.
🤖 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 `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 692-704: The pending-auth deadlock happens because handlers passed
to _trackPendingAuthResolution await async work (callOAuthCallback /
_maybeHandleNestedCrossDomainAuth) and callers like MFA fallback call
_getSession which awaits the same pending chain; fix by ensuring the tracked
handlers are launched asynchronously and not awaited on the same promise chain —
change calls where you do this._trackPendingAuthResolution(async () => { await
this.callOAuthCallback(...) }) and similar for _maybeHandleNestedCrossDomainAuth
to start the async work without blocking the tracker (e.g. invoke the async
handler in a detached microtask: Promise.resolve().then(() =>
this.callOAuthCallback({...})) or otherwise call the handler without awaiting
inside the tracked callback) so _getSession and MFA fallback cannot deadlock;
update all occurrences (including the blocks around callOAuthCallback and
_maybeHandleNestedCrossDomainAuth mentioned) to use this non-blocking invocation
pattern.
---
Nitpick comments:
In `@packages/stack-shared/src/utils/redirect-urls.tsx`:
- Around line 4-7: TrustedDomainConfig.trustedDomains uses readonly (string |
null | undefined)[] but getTrustedParentDomain currently expects readonly
string[]; unify the element type by changing getTrustedParentDomain's trusted
domains parameter to accept readonly (string | null | undefined)[] (same as
TrustedDomainConfig.trustedDomains) and perform internal filtering of
null/undefined values inside getTrustedParentDomain (mirroring
validateRedirectUrl) so callers don’t need to coerce or filter; update any other
exported APIs that accept trusted domain arrays to use the unified element type
as well (e.g., the other occurrence referenced alongside
getTrustedParentDomain).
In `@packages/template/src/lib/stack-app/url-targets.test.ts`:
- Around line 99-115: Replace the regex-based assertions in the two tests that
call resolveHandlerUrls with snapshot assertions: change the
expect(...).toThrowError(/OAuth callback URLs must be relative/) calls to use
toThrowErrorMatchingInlineSnapshot and capture the exact error message string
produced by resolveHandlerUrls (for both the string target and the { type:
"custom", url, version } case) so the tests assert the precise error text;
update the two test cases (the "rejects absolute OAuth callback string targets"
and "rejects absolute OAuth callback custom targets") to call
toThrowErrorMatchingInlineSnapshot and record the inline snapshot of the thrown
error message.
🪄 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: 9719bf2b-3cdb-43f9-89c9-fcadc4c413cf
📒 Files selected for processing (10)
.claude/CLAUDE-KNOWLEDGE.mdapps/backend/src/lib/redirect-urls.tsxapps/e2e/tests/js/cross-domain-auth.test.tsexamples/demo/src/stack.tsxpackages/stack-shared/src/interface/crud/projects.tspackages/stack-shared/src/utils/redirect-urls.tsxpackages/template/src/lib/auth.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/url-targets.test.tspackages/template/src/lib/stack-app/url-targets.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR implements nested cross-domain handoffs: when a signed-in user on
Confidence Score: 3/5The nested cross-domain auth flow is architecturally sound and well-tested, but two gaps in the new hosted-callback path could cause real failures before this is safe to ship. On the source domain, the redirectUri extracted from URL params is forwarded to _createCrossDomainAuthRedirectUrl without any client-side trust check — trust validation exists on the target-domain side but is asymmetrically absent here. Separately, _getOAuthCallbackRedirectUri({ forCallback: true }) strips only code and state, leaving OAuth error params in the redirect URI; a page that previously saw an OAuth error will misclassify a subsequent successful OAuth callback as a failure. packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts requires the most attention — specifically _maybeHandleNestedCrossDomainAuth (source-domain branch) and _getOAuthCallbackRedirectUri. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SourceApp as Source App (a.com)
participant TargetHandler as Hosted Handler (b.com)
participant Backend as Stack Backend
User->>SourceApp: Click link to hosted handler
SourceApp->>SourceApp: _addNestedCrossDomainAuthParamsToRedirectUrl
Note over SourceApp: Appends refreshTokenId + callbackUrl to URL
SourceApp->>TargetHandler: Redirect with nested auth params
TargetHandler->>TargetHandler: _maybeHandleNestedCrossDomainAuth
TargetHandler->>TargetHandler: _isTrusted(callbackUrl)
TargetHandler->>TargetHandler: Generate PKCE state and challenge
TargetHandler->>SourceApp: Redirect with OAuth request params
SourceApp->>SourceApp: _maybeHandleNestedCrossDomainAuth (source role)
SourceApp->>SourceApp: Verify session matches refreshTokenId
SourceApp->>Backend: _createCrossDomainAuthRedirectUrl
Backend-->>SourceApp: URL with one-time auth code
SourceApp->>TargetHandler: Redirect with code and state
TargetHandler->>TargetHandler: callOAuthCallback
TargetHandler->>Backend: Exchange code for session tokens
Backend-->>TargetHandler: Session tokens
TargetHandler->>User: Signed in on target domain
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts:844-872
**Missing client-side trust validation for `redirectUri` on source domain**
When a.com acts as the OAuth-code issuer in the nested flow, the `redirectUri` comes directly from URL query params supplied by b.com and is passed straight into `_createCrossDomainAuthRedirectUrl` without first calling `_isTrusted()` on it. Per custom rule `45e85dee`, redirect URLs must always be validated before use. An attacker who can lure the user to a crafted a.com URL with a matching `refreshTokenId` and a malicious `redirect_uri` would receive the cross-domain auth code. Adding an `_isTrusted(redirectUri)` guard before the `_createCrossDomainAuthRedirectUrl` call mirrors the existing check already applied on b.com via `_isTrusted(callbackUrlString)`.
### Issue 2 of 3
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts:786-800
**OAuth error params not stripped from hosted redirect URI**
`_getOAuthCallbackRedirectUri({ forCallback: true })` removes only `code` and `state`, but not the OAuth error params consumed by `consumeOAuthCallbackQueryParams` (`error`, `error_description`, `errorCode`, `message`, `details`). If a previous failed OAuth attempt left `errorCode` in the browser URL and the user retries OAuth from the same page, those stale params are baked into the registered `redirect_uri`. After the new successful OAuth flow completes, `consumeOAuthCallbackQueryParams` detects the stale error params first and reports the callback as a failure even though `code`+`state` are also present. Fixed-path callbacks like `/handler/oauth-callback` are never affected by this.
### Issue 3 of 3
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts:723-745
**Auth-resolution errors propagate unguarded to all session consumers**
When a pending resolution throws (e.g., `_maybeHandleNestedCrossDomainAuth` detecting an untrusted callback URL or mismatched session), the raw rejection is stored in `_pendingAuthResolutionPromises`. Every subsequent call to `_getSession` hits `Promise.all(...)` on those promises and surfaces internal errors to ordinary application code calling `getUser()`. A user navigating to a hosted handler via a link with stale nested auth params would see the page break entirely until the `finally` cleanup runs.
Reviews (1): Last reviewed commit: "Cross domain handoffs" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
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)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
692-698:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t auto-start hosted OAuth callback handling on every page load.
When
oauthCallbackis hosted, the constructor now queuescallOAuthCallback()for every app instance. That makes hosted apps pay this path on ordinary pages, and pages that happen to use genericcode/stateorerrorCode/messagequery params can be misclassified as Stack OAuth returns. Please gate this on a Stack-owned callback signal, not just on the hosted config.🤖 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 `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts` around lines 692 - 698, The constructor currently auto-queues callOAuthCallback() when isBrowserLike() && this._isOAuthCallbackUrlHosted(), which triggers hosted OAuth handling on every page; change the gating to require a Stack-owned callback signal: add or use a validation method (e.g. _isStackOAuthCallbackRequest()) and only call this._trackPendingAuthResolution(...) when that method returns true. Implement _isStackOAuthCallbackRequest() to verify the current window location matches the expected Stack callback (for example by hostname/path matching the configured oauth callback URL or by requiring a Stack-specific query marker or signed param in addition to the usual code/state or errorCode/message), then update the check in the constructor to: if (isBrowserLike() && this._isOAuthCallbackUrlHosted() && this._isStackOAuthCallbackRequest()) { ... } and keep calling await this.callOAuthCallback({ dontWarnAboutMissingQueryParams: true }) inside the existing _trackPendingAuthResolution wrapper.
🤖 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 `@packages/stack-shared/src/utils/redirect-urls.tsx`:
- Around line 38-52: hostPatternWithoutPort()/hostPatternHasExplicitPort()
currently only treats numeric ports as explicit so patterns ending with ":*"
remain and break normalization; update hostPatternHasExplicitPort to treat a
single "*" (and still numeric strings) as an explicit port (i.e., return true
when port === "*" OR every char is a digit) so hostPatternWithoutPort will strip
both numeric and wildcard port suffixes before hostname matching (refer to
functions hostPatternWithoutPort and hostPatternHasExplicitPort).
---
Outside diff comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 692-698: The constructor currently auto-queues callOAuthCallback()
when isBrowserLike() && this._isOAuthCallbackUrlHosted(), which triggers hosted
OAuth handling on every page; change the gating to require a Stack-owned
callback signal: add or use a validation method (e.g.
_isStackOAuthCallbackRequest()) and only call
this._trackPendingAuthResolution(...) when that method returns true. Implement
_isStackOAuthCallbackRequest() to verify the current window location matches the
expected Stack callback (for example by hostname/path matching the configured
oauth callback URL or by requiring a Stack-specific query marker or signed param
in addition to the usual code/state or errorCode/message), then update the check
in the constructor to: if (isBrowserLike() && this._isOAuthCallbackUrlHosted()
&& this._isStackOAuthCallbackRequest()) { ... } and keep calling await
this.callOAuthCallback({ dontWarnAboutMissingQueryParams: true }) inside the
existing _trackPendingAuthResolution wrapper.
🪄 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: 82310389-c795-4816-8e0d-e0355ecc71b2
📒 Files selected for processing (5)
apps/e2e/tests/js/cross-domain-auth.test.tspackages/stack-shared/src/utils/redirect-urls.tsxpackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/url-targets.test.tspackages/template/src/lib/stack-app/url-targets.ts
Contributor
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)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
694-699:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep hosted OAuth error callbacks on the auto-start path.
callOAuthCallback()still recognizeserrorCode+message, but the constructor now only auto-starts when_currentUrlLooksLikeStackOAuthCallback()seescode+stateplus the verifier cookie. Hosted OAuth failures will now land on the callback page and never get processed. Keep the cookie requirement for the success path, but preserve the existing error-shape branch here too.Suggested fix
protected _currentUrlLooksLikeStackOAuthCallback(): boolean { if (typeof window === "undefined") { return false; } const currentUrl = new URL(window.location.href); + if (currentUrl.searchParams.has("errorCode") && currentUrl.searchParams.has("message")) { + return true; + } const state = currentUrl.searchParams.get("state"); if (!currentUrl.searchParams.has("code") || state == null) { return false; } return getCookieClient(`stack-oauth-outer-${state}`) != null; }Also applies to: 794-804
🤖 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 `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts` around lines 694 - 699, The current auto-start gate uses _currentUrlLooksLikeStackOAuthCallback() plus the verifier cookie, which blocks hosted OAuth error callbacks; update the wrapper in the _trackPendingAuthResolution call to allow an alternative branch that invokes callOAuthCallback when the URL contains the OAuth error shape (errorCode and message) even if the verifier cookie is missing. Specifically, change the if-condition around _isOAuthCallbackUrlHosted()/_currentUrlLooksLikeStackOAuthCallback() so it triggers when either (a) the success path conditions are met (existing _currentUrlLooksLikeStackOAuthCallback() and verifier cookie) or (b) the URL looks like an OAuth error (check for errorCode and message), and then call callOAuthCallback({ dontWarnAboutMissingQueryParams: true }) in that branch; apply the same change to the second similar block (the one around lines 794-804) to preserve hosted error callbacks.
🧹 Nitpick comments (1)
apps/e2e/tests/js/cross-domain-auth.test.ts (1)
189-191: ⚡ Quick winUse inline snapshots for these new Vitest assertions.
Please switch these new expectations to
.toMatchInlineSnapshot(...)to match the repo’s test-style rule.Example adjustment
- expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toBe(false); + expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toMatchInlineSnapshot(`false`); globalThis.document.cookie = "stack-oauth-outer-oauth-state=verifier"; - expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toBe(true); + expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toMatchInlineSnapshot(`true`);As per coding guidelines
**/*.test.{ts,tsx}: “When writing tests, prefer.toMatchInlineSnapshotover other selectors in Vitest tests.”Also applies to: 254-257, 291-293
🤖 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/e2e/tests/js/cross-domain-auth.test.ts` around lines 189 - 191, Replace the boolean equality assertions on (clientApp as any)._currentUrlLooksLikeStackOAuthCallback() with inline snapshots: change expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toBe(false) to expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toMatchInlineSnapshot('false') and the .toBe(true) case to .toMatchInlineSnapshot('true'); apply the same change for the other identical assertions in this test file (the other occurrences around the later assertions that check the same _currentUrlLooksLikeStackOAuthCallback() result).
🤖 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 `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 694-699: The current auto-start gate uses
_currentUrlLooksLikeStackOAuthCallback() plus the verifier cookie, which blocks
hosted OAuth error callbacks; update the wrapper in the
_trackPendingAuthResolution call to allow an alternative branch that invokes
callOAuthCallback when the URL contains the OAuth error shape (errorCode and
message) even if the verifier cookie is missing. Specifically, change the
if-condition around
_isOAuthCallbackUrlHosted()/_currentUrlLooksLikeStackOAuthCallback() so it
triggers when either (a) the success path conditions are met (existing
_currentUrlLooksLikeStackOAuthCallback() and verifier cookie) or (b) the URL
looks like an OAuth error (check for errorCode and message), and then call
callOAuthCallback({ dontWarnAboutMissingQueryParams: true }) in that branch;
apply the same change to the second similar block (the one around lines 794-804)
to preserve hosted error callbacks.
---
Nitpick comments:
In `@apps/e2e/tests/js/cross-domain-auth.test.ts`:
- Around line 189-191: Replace the boolean equality assertions on (clientApp as
any)._currentUrlLooksLikeStackOAuthCallback() with inline snapshots: change
expect((clientApp as any)._currentUrlLooksLikeStackOAuthCallback()).toBe(false)
to expect((clientApp as
any)._currentUrlLooksLikeStackOAuthCallback()).toMatchInlineSnapshot('false')
and the .toBe(true) case to .toMatchInlineSnapshot('true'); apply the same
change for the other identical assertions in this test file (the other
occurrences around the later assertions that check the same
_currentUrlLooksLikeStackOAuthCallback() result).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 413264f7-c689-455e-b9e9-659893a526ab
📒 Files selected for processing (4)
.claude/CLAUDE-KNOWLEDGE.mdapps/e2e/tests/js/cross-domain-auth.test.tspackages/stack-shared/src/utils/redirect-urls.tsxpackages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
✅ Files skipped from review due to trivial changes (1)
- .claude/CLAUDE-KNOWLEDGE.md
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dfb6d61. Configure here.
| protected _getLocalOAuthCallbackHandlerUrl(): string { | ||
| if (this._isOAuthCallbackUrlHosted()) { | ||
| return this._getOAuthCallbackRedirectUri(); | ||
| } |
There was a problem hiding this comment.
Server-side crash when hosted OAuth callback URL evaluated eagerly
Medium Severity
_getLocalOAuthCallbackHandlerUrl() is eagerly evaluated as an argument to planRedirectToHandler at line 2878, even in server-side contexts where currentUrl is null. When _isOAuthCallbackUrlHosted() returns true (e.g., with default: { type: "hosted" } as in the demo app change), it calls _getOAuthCallbackRedirectUri() which throws a StackAssertionError when window is undefined. Although planRedirectToHandler never uses localOAuthCallbackUrl when currentUrl is null (it returns early), JavaScript evaluates the argument before the call. This makes any server-side invocation of _redirectToHandler crash when the OAuth callback is configured as hosted.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dfb6d61. Configure here.
This was referenced May 22, 2026
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
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.
Note
Medium Risk
Changes core OAuth/cross-domain redirect and session-resolution behavior in the client SDK, which is security- and flow-critical and could cause login/redirect regressions if edge cases are missed. Risk is mitigated by expanded E2E/unit coverage and stricter trust checks.
Overview
Adds nested cross-domain auth handoffs for hosted flows by attaching source-session identifiers and callback URLs to cross-origin handler redirects, then completing a two-step bounce that mints a cross-domain code only when the source session and trusted redirect URLs match.
Hardens hosted OAuth callback behavior and session consistency: hosted callbacks now auto-run only when a matching
stack-oauth-outer-${state}cookie exists, the redirect URI for hosted callbacks is derived from the current page with stale OAuth params stripped, and startup auth transitions are tracked as pending promises that_getSession/react-like_useSessioncan await (with opt-out for re-entrant flows).Centralizes redirect URL validation by moving trusted-domain wildcard/port/localhost logic into
stack-shared(validateRedirectUrl,getTrustedParentDomain) and updating backend/client trust checks to use it; project client read schema now includesallow_localhost.Tightens handler URL configuration rules by preventing absolute
oauthCallbacktargets and avoiding inheriting absolutedefaulttargets foroauthCallback, plus updates the demo config to use hosted defaults and adds extensive new E2E/unit tests around these flows.Reviewed by Cursor Bugbot for commit dfb6d61. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation