Merged
Conversation
Other minor redirect URL changes: - app.urls.* for auth is now aware of after_auth_return_to - redirectToSignOut now sets and preserves after_auth_return_to - OAuth sign-in after_auth_return_to now carries callback-return context
|
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:
📝 WalkthroughWalkthroughAdds cross-domain OAuth PKCE handoff: new backend cross-domain authorize endpoint, DB column linking granted refresh tokens, client-side URL/redirect planning and hosted-handler resolution, centralized env access, tests, demo UI, and event-tracker runtime guards. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Handler as Hosted Auth Handler
participant Backend as Backend OAuth Handler
participant DB as Database
participant User as User Agent
Browser->>Handler: POST /auth/oauth/cross-domain/authorize (auth headers, body)
Handler->>Backend: validate session, refresh-token ownership, publishable key
Backend->>DB: verify refresh token record (projectUserId, id)
DB-->>Backend: token record
Backend->>Backend: generate/validate PKCE (state, code_challenge)
Backend->>Backend: oauthServer.authorize(auth handler includes user + afterCallbackRedirectUrl)
Backend->>DB: create AuthorizationCode (grantedRefreshTokenId = user.refreshTokenId)
Backend-->>Handler: return JSON { redirect_url } (from oauthResponse.headers.location)
Handler-->>Browser: JSON { redirect_url }
Browser->>User: browser navigates to redirect_url (OAuth provider)
User->>Backend: provider -> callback with code,state
Browser->>Backend: POST /token (exchange code + code_verifier)
Backend->>DB: lookup code -> retrieve grantedRefreshTokenId
DB-->>Backend: code, grantedRefreshTokenId
Backend-->>Browser: tokens (access_token, refresh_token)
Browser->>Browser: store tokens and redirect to after_callback_redirect_url
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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
Greptile SummaryThis PR introduces a Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App (cross-domain)
participant SDK as StackClientApp SDK
participant Hosted as Hosted Handler
participant Backend as Stack Backend
participant Provider as OAuth Provider
App->>SDK: redirectToSignIn()
SDK->>SDK: planRedirectToHandler() detects cross-domain target
SDK->>Backend: POST /auth/oauth/cross-domain/authorize with PKCE params and refresh-token header
Backend->>Backend: Validate redirect_uri and after_callback_redirect_url against trusted domains
Backend->>Backend: Issue authorization code, store grantedRefreshTokenId
Backend-->>SDK: Returns redirect_url pointing to hosted handler callback
SDK->>Hosted: Redirect to hosted sign-in with after_auth_return_to set to local callback URL
Hosted->>Provider: OAuth sign-in redirect
Provider-->>Hosted: Returns to hosted oauth-callback with code and state
Hosted->>Backend: Exchange OAuth code for session
Hosted->>App: Redirect to after_auth_return_to which is local handler with stack_cross_domain_auth marker
App->>SDK: callOAuthCallback() reconstructs redirect_uri from current URL
SDK->>Backend: POST /auth/oauth/token to exchange cross-domain auth code with PKCE verifier
Backend->>Backend: Verify PKCE and reuse grantedRefreshTokenId as refresh token
Backend-->>SDK: Returns access token and reused refresh token with after_callback_redirect_url
SDK->>App: Redirect to original page via after_callback_redirect_url
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class support for hosted (cross-domain) handler URLs in the JS SDK/template, including a secure cross-domain OAuth handoff flow that reuses an existing refresh-token session, plus a centralized env-var access layer.
Changes:
- Introduce URL target resolution (
hostedvshandler-component) and hosted handler URL templating for all handler endpoints. - Implement cross-domain auth handoff plumbing (redirect URL planning, authorize endpoint, refresh-token reuse via authorization codes) with accompanying unit/e2e coverage.
- Centralize env-var reads via
envVarsand enforce via eslint; update template components to use it.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/template/src/lib/stack-app/url-targets.ts | Resolves handler URL targets (hosted vs local component) into concrete URLs. |
| packages/template/src/lib/stack-app/url-targets.test.ts | Unit tests for URL target resolution + hosted template handling. |
| packages/template/src/lib/stack-app/index.ts | Re-export new URL target/option/resolved types. |
| packages/template/src/lib/stack-app/common.ts | Introduce HandlerUrlTarget/HandlerUrlOptions/ResolvedHandlerUrls types. |
| packages/template/src/lib/stack-app/apps/interfaces/client-app.ts | Update client app options + urls typing to use resolved URLs. |
| packages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts | Central redirect planning + cross-domain handoff URL construction utilities. |
| packages/template/src/lib/stack-app/apps/implementations/common.ts | Switch URL building to resolveHandlerUrls; move env reads to envVars. |
| packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts | Wire hosted URL trust, dynamic redirect-ready app.urls, and cross-domain authorize execution. |
| packages/template/src/lib/env.ts | Add centralized, bundler-friendly env-var getters (envVars). |
| packages/template/src/lib/auth.ts | Plumb afterCallbackRedirectUrl through OAuth URL creation. |
| packages/template/src/components-page/stack-handler-client.tsx | Support redirecting handler pages to hosted targets + unknown path fallback routing. |
| packages/template/src/components-page/oauth-callback.tsx | Replace direct process.env reads with envVars. |
| packages/template/src/components-page/account-settings/payments/payments-panel.tsx | Replace direct process.env reads with envVars. |
| packages/template/.eslintrc.cjs | Enforce envVars usage by banning process.env reads (except env.ts). |
| examples/demo/src/stack.tsx | Demo app configured to use hosted handler defaults. |
| examples/demo/src/app/cross-domain-handoff/page.tsx | New manual cross-domain handoff verification page. |
| examples/demo/.env.development | Add hosted handler URL template for local dev. |
| claude/CLAUDE-KNOWLEDGE.md | Record cross-domain handoff + env-var conventions/notes. |
| apps/e2e/tests/js/cross-domain-auth.test.ts | New JS e2e coverage for hosted redirect + cross-domain handoff behavior. |
| apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/cross-domain-authorize.test.ts | New backend e2e coverage for cross-domain authorize + PKCE exchange + validation. |
| apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts | Add coverage for invalid/untrusted after_callback_redirect_url. |
| apps/e2e/.env.development | Configure local hosted handler domain suffix for e2e env. |
| apps/dashboard/.env.development | Configure local hosted handler domain suffix for dashboard dev env. |
| apps/backend/src/oauth/model.tsx | Add refresh-token reuse via authorization codes (grantedRefreshTokenId). |
| apps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsx | New endpoint to mint cross-domain authorize redirects gated by refresh-token-bound session. |
| apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx | Validate + whitelist after_callback_redirect_url. |
| apps/backend/prisma/schema.prisma | Add grantedRefreshTokenId to ProjectUserAuthorizationCode. |
| apps/backend/prisma/migrations/20260310150000_add_oauth_authorization_code_refresh_token_id/tests/preserve-null-and-allow-uuid.ts | Migration test ensuring null preservation + UUID acceptance. |
| apps/backend/prisma/migrations/20260310150000_add_oauth_authorization_code_refresh_token_id/migration.sql | Migration adding grantedRefreshTokenId column. |
| apps/backend/.env.development | Configure local hosted handler domain suffix for backend dev env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
apps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsx
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
![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: 6
🧹 Nitpick comments (3)
apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts (1)
202-255: Add a positive test for accepted native-app redirect URLs.These additions cover invalid and untrusted values well; please also add a success case for
after_callback_redirect_urlusing the accepted native-app scheme so the allowlist path is locked by tests.Suggested test shape
+it("should allow accepted native-app after_callback_redirect_url", async ({ expect }) => { + const response = await niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", { + redirect: "manual", + query: { + ...await Auth.OAuth.getAuthorizeQuery(), + after_callback_redirect_url: "stack-auth-mobile-oauth-url://post-auth", + }, + }); + expect(response.status).toBe(307); +});As per coding guidelines,
**/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/backend/endpoints/api/v1/auth/oauth/authorize.test.ts` around lines 202 - 255, Add a positive E2E test in the same file to verify the allowlist accepts native-app redirect schemes: create an it block (e.g., it("should accept native-app after_callback_redirect_url", ...)) that calls niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", { redirect: "manual", query: { ...await Auth.OAuth.getAuthorizeQuery(), after_callback_redirect_url: "com.stack.app://post-auth" } }) and assert the response is a successful redirect (e.g., status 302) and does not return a REDIRECT_URL_NOT_WHITELISTED/SCHEMA_ERROR; use the existing helpers Auth.OAuth.getAuthorizeQuery and niceBackendFetch so the new test mirrors the invalid/untrusted tests and locks the allowlist path.apps/e2e/tests/js/cross-domain-auth.test.ts (1)
51-60: Replaceas anytype casts withvi.stubGlobal()and remove private method coupling.The mocks bypass type guarantees without justification and lack required explanatory comments. Use
vi.stubGlobal()instead, which is the established pattern inapps/e2e/tests/js/cookies.test.ts. Additionally, spying on_createCrossDomainAuthRedirectUrl(a private method) creates brittle test-to-implementation coupling; instead, mock the request boundary.Affects lines: 51-60, 92-99, 132-138, 177-179, 181-190, 233-235, 237-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/js/cross-domain-auth.test.ts` around lines 51 - 60, Replace unsafe "as any" global assignments and the private-method spy with stable stubs: use vi.stubGlobal() to mock document and window (matching the pattern in apps/e2e/tests/js/cookies.test.ts) instead of casting to any, and remove the spy on the private method _createCrossDomainAuthRedirectUrl; instead, mock the external request boundary (the function or module that makes the HTTP/auth request) so tests inspect the resulting behavior (redirect assignment/exception) without coupling to a private implementation detail. Ensure comments explain why vi.stubGlobal is used and which external boundary is being mocked.examples/demo/src/app/cross-domain-handoff/page.tsx (1)
68-90: Pass the async handlers straight toButton.These wrappers duplicate the button’s built-in async handling and make the page harder to read. Letting
Buttonreceive the async function directly keeps the loading/error behavior intact without the extra indirection.Based on learnings: In the stack-auth codebase, Button components from stackframe/stack-ui can accept async functions directly and automatically handle loading states with indicators. No need to wrap these with `runAsynchronouslyWithAlert`.♻️ Minimal cleanup
- <Button - onClick={() => runAsynchronouslyWithAlert(async () => { - await app.redirectToSignIn(); - })} - > + <Button + onClick={async () => { + await app.redirectToSignIn(); + }} + >Apply the same pattern to the other
Buttonhandlers on this page.Also applies to: 108-129, 141-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/demo/src/app/cross-domain-handoff/page.tsx` around lines 68 - 90, The Button handlers are wrapped unnecessarily with runAsynchronouslyWithAlert, duplicating the Button component’s built-in async handling; replace each onClick={() => runAsynchronouslyWithAlert(async () => { ... })} with a direct async handler reference so Button receives the async function directly (e.g., pass async () => await user?.signOut({ redirectUrl: "/cross-domain-handoff" }), async () => await app.redirectToSignIn(), and async () => await app.redirectToSignUp()) so the Button component can manage loading/error UI itself; update the other Button usages that currently wrap runAsynchronouslyWithAlert (including the similar handlers elsewhere on the page) to the same direct-async pattern.
🤖 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/backend/src/oauth/model.tsx`:
- Around line 126-131: Replace the throw of StackAssertionError in the
refresh-token ownership check with the deterministic OAuth/auth error type used
by the OAuth routes (so the branch maps to a 401/invalid auth response instead
of an assertion). Specifically, in the block that inspects refreshTokenObj and
compares refreshTokenObj.projectUserId to user.id, remove the
StackAssertionError throw and throw or return the project's mapped auth error
(the same class or factory used elsewhere for invalid credentials/invalid_grant
responses in OAuth handling) so callers receive a consistent auth failure
instead of an unhandled server assertion.
In `@claude/CLAUDE-KNOWLEDGE.md`:
- Around line 82-110: The knowledge entry currently contradicts itself about
carrying refreshTokenId in the URL vs DB; update the text to keep only the
DB-column approach: remove references that encode `refreshTokenId` into
`afterCallbackRedirectUrl` and any mention of decoding it in
`OAuthModel._getOrCreateRefreshTokenObj` or sanitizing
`after_callback_redirect_url` in `saveToken`, and instead state that
refresh-token linkage is stored in
`ProjectUserAuthorizationCode.grantedRefreshTokenId` and returned via
`getAuthorizationCode` so token issuance can reuse it; keep mentions of
`afterCallbackRedirectUrl` as URL-only and references to
`planRedirectToHandler`/`redirect-page-urls.ts` behaviors as-is.
In `@packages/template/src/components-page/oauth-callback.tsx`:
- Around line 30-31: The stale closure `error` is being used in the redirect
check inside the OAuth callback, causing failed callbacks to still redirect;
update the logic in the handler that calls app.redirectToSignIn to read the
current/local error value from this execution (e.g., capture the callback result
into a local variable like `currentError` or `callbackError`) and use an
explicit null/undefined check (use `== null` rather than `!error`) together with
`hasRedirected` and the NODE_ENV condition before calling
`app.redirectToSignIn({ noRedirectBack: true })`; locate and update the redirect
condition that references `error` and `hasRedirected` (the block around the
OAuth callback/redirect logic) and ensure the local variable is used so the
redirect happens only when there is no actual callback error.
In `@packages/template/src/lib/auth.ts`:
- Line 26: Normalize and validate options.afterCallbackRedirectUrl before
passing it into getOAuthUrl the same way redirectUrl and errorRedirectUrl are
handled: locate where getOAuthUrl is invoked with afterCallbackRedirectUrl
(symbol: afterCallbackRedirectUrl) in auth.ts and run it through the existing
normalization/validation routine used for redirectUrl/errorRedirectUrl (or call
the same helper/validator), and if validation fails throw an error immediately
so the function fails fast rather than passing a raw value to getOAuthUrl.
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 2207-2215: The helper _getLocalOAuthCallbackHandlerUrl currently
only overrides the default target but lets callers' urls.oauthCallback
(including hosted or absolute values) pass through, causing cross-origin
callback resolution errors; fix by forcing urls.oauthCallback to a local handler
value before calling resolveHandlerUrls (e.g. set urls.oauthCallback = { type:
"handler-component" } while still spreading this._urlOptions for other keys), so
resolveHandlerUrls(...) always returns a same-origin oauthCallback; update the
call in _getLocalOAuthCallbackHandlerUrl to merge this forced oauthCallback
override and keep projectId unchanged.
- Around line 2223-2245: The code currently always parses the response into the
success shape and throws a StackAssertionError on unexpected payloads, which
hides real HTTP errors; modify the block after calling
this._interface.sendClientRequest to first check response.ok and, if false,
route the response through the normal client error path (e.g., call your
existing error handler on the interface or rethrow a parsed client error using
the response and session) instead of continuing to parse and potentially
throwing StackAssertionError; only parse JSON and validate redirect_url when
response.ok is true.
---
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts`:
- Around line 202-255: Add a positive E2E test in the same file to verify the
allowlist accepts native-app redirect schemes: create an it block (e.g.,
it("should accept native-app after_callback_redirect_url", ...)) that calls
niceBackendFetch("/api/v1/auth/oauth/authorize/spotify", { redirect: "manual",
query: { ...await Auth.OAuth.getAuthorizeQuery(), after_callback_redirect_url:
"com.stack.app://post-auth" } }) and assert the response is a successful
redirect (e.g., status 302) and does not return a
REDIRECT_URL_NOT_WHITELISTED/SCHEMA_ERROR; use the existing helpers
Auth.OAuth.getAuthorizeQuery and niceBackendFetch so the new test mirrors the
invalid/untrusted tests and locks the allowlist path.
In `@apps/e2e/tests/js/cross-domain-auth.test.ts`:
- Around line 51-60: Replace unsafe "as any" global assignments and the
private-method spy with stable stubs: use vi.stubGlobal() to mock document and
window (matching the pattern in apps/e2e/tests/js/cookies.test.ts) instead of
casting to any, and remove the spy on the private method
_createCrossDomainAuthRedirectUrl; instead, mock the external request boundary
(the function or module that makes the HTTP/auth request) so tests inspect the
resulting behavior (redirect assignment/exception) without coupling to a private
implementation detail. Ensure comments explain why vi.stubGlobal is used and
which external boundary is being mocked.
In `@examples/demo/src/app/cross-domain-handoff/page.tsx`:
- Around line 68-90: The Button handlers are wrapped unnecessarily with
runAsynchronouslyWithAlert, duplicating the Button component’s built-in async
handling; replace each onClick={() => runAsynchronouslyWithAlert(async () => {
... })} with a direct async handler reference so Button receives the async
function directly (e.g., pass async () => await user?.signOut({ redirectUrl:
"/cross-domain-handoff" }), async () => await app.redirectToSignIn(), and async
() => await app.redirectToSignUp()) so the Button component can manage
loading/error UI itself; update the other Button usages that currently wrap
runAsynchronouslyWithAlert (including the similar handlers elsewhere on the
page) to the same direct-async pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82048621-079a-4807-871c-ecbeae81c2f7
📒 Files selected for processing (30)
apps/backend/.env.developmentapps/backend/prisma/migrations/20260310150000_add_oauth_authorization_code_refresh_token_id/migration.sqlapps/backend/prisma/migrations/20260310150000_add_oauth_authorization_code_refresh_token_id/tests/preserve-null-and-allow-uuid.tsapps/backend/prisma/schema.prismaapps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsxapps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsxapps/backend/src/oauth/model.tsxapps/dashboard/.env.developmentapps/e2e/.env.developmentapps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.tsapps/e2e/tests/backend/endpoints/api/v1/auth/oauth/cross-domain-authorize.test.tsapps/e2e/tests/js/cross-domain-auth.test.tsclaude/CLAUDE-KNOWLEDGE.mdexamples/demo/.env.developmentexamples/demo/src/app/cross-domain-handoff/page.tsxexamples/demo/src/stack.tsxpackages/template/.eslintrc.cjspackages/template/src/components-page/account-settings/payments/payments-panel.tsxpackages/template/src/components-page/oauth-callback.tsxpackages/template/src/components-page/stack-handler-client.tsxpackages/template/src/lib/auth.tspackages/template/src/lib/env.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/implementations/common.tspackages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.tspackages/template/src/lib/stack-app/apps/interfaces/client-app.tspackages/template/src/lib/stack-app/common.tspackages/template/src/lib/stack-app/index.tspackages/template/src/lib/stack-app/url-targets.test.tspackages/template/src/lib/stack-app/url-targets.ts
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
2238-2260:⚠️ Potential issue | 🟠 MajorSurface
/auth/oauth/cross-domain/authorizefailures as client errors, not assertions.A 400/401 from this endpoint is still converted into
StackAssertionError, so callers lose the originalKnownErrors/status information and the redirect flow only sees a generic failure. Please route non-2xx responses through the normal interface error path before validatingredirect_url.🤖 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 2238 - 2260, The code currently throws a StackAssertionError when response.ok is false; instead route non-2xx responses through the interface's normal client-error path before validating the payload. Replace the current !response.ok branch in the sendClientRequest response handling so that you call the interface's error conversion/propagation helper (e.g. a method on this._interface such as createClientErrorFromResponse/convertResponseToError/throwIfResponseError) with the response and session and rethrow that result, rather than constructing a StackAssertionError; keep the subsequent JSON parsing and redirect_url validation only for successful responses. Ensure you remove the StackAssertionError throw in that branch and use the interface method so KnownErrors/status information is preserved.
🧹 Nitpick comments (1)
.cursor/commands/pr-comments-review.md (1)
1-1: Consider more professional language for documentation.The phrase "mostly bullshit" is informal and may not be appropriate for command documentation, even for internal tooling. Consider replacing it with more professional alternatives like "not actionable," "invalid," or "low-priority."
📝 Suggested rewording
-Please review the PR comments with the `gh` CLI and fix those issues that are valid and relevant. Resolve the comments when you fix them. Also resolve all those comments that no longer exist or have already been resolved. Leave those comments that are mostly bullshit unresolved. Also, create or modify tests to make sure the fixed behavior works as expected. Report the result to me in detail (for the most important issues, whether resolved or unresolved, mark them with a ‼️ emoji). Do NOT automatically commit or stage the changes back to the PR! +Please review the PR comments with the `gh` CLI and address those issues that are valid and relevant. Resolve the comments when you fix them. Also resolve all those comments that no longer exist or have already been resolved. Leave comments that are not actionable or low-priority unresolved. Also, create or modify tests to make sure the fixed behavior works as expected. Report the result to me in detail (for the most important issues, whether resolved or unresolved, mark them with a ‼️ emoji). Do NOT automatically commit or stage the changes back to the PR!Note: Also incorporated the static analysis suggestion to use "address" instead of "fix" for a more formal tone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cursor/commands/pr-comments-review.md at line 1, Replace the informal phrase "mostly bullshit" in the documentation for the pr-comments-review command with a more professional alternative (e.g., "not actionable", "invalid", or "low-priority") and change wording from "fix" to "address" where suggested; locate the text by searching for the exact string "mostly bullshit" in .cursor/commands/pr-comments-review.md and update surrounding sentences to keep tone consistent, then update any doc/help output tests that assert the command's messaging (tests referencing pr-comments-review help text or the exact phrase) so they assert the new phrasing; ensure to resolve the PR comment after making these documentation and test updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 535-536: The constructor currently calls
_prefetchCrossDomainHandoffParamsIfNeeded() unconditionally which causes
saveVerifierAndState() to generate and store a fresh verifier/state even on
OAuth callback pages; update _prefetchCrossDomainHandoffParamsIfNeeded() (or the
constructor call) to first detect if the current URL contains OAuth callback
query parameters (both "code" and "state") and if so, return immediately (no
saveVerifierAndState call). Ensure this change touches the places mentioned (the
constructor invocation at the top-level and the code paths around
callOAuthCallback(), and the helper logic where saveVerifierAndState() is
invoked) so prefetch is skipped when code+state are present to avoid creating
unrelated verifier/state pairs.
In `@packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts`:
- Around line 45-55: The null/undefined checks around reflective reads are
insufficient: update the guard in event-tracker.ts to explicitly verify
screenObject != null before calling Reflect.get(screenObject,
"width")/("height") (replace the typeof screenObject !== "object" check) and
likewise ensure historyObject != null before calling Reflect.get(historyObject,
"pushState") so reflective reads never run on null/undefined; adjust the
conditions around the existing Reflect.get calls (referencing screenObject,
historyObject, and Reflect.get) to short-circuit earlier when either object is
null or undefined.
---
Duplicate comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 2238-2260: The code currently throws a StackAssertionError when
response.ok is false; instead route non-2xx responses through the interface's
normal client-error path before validating the payload. Replace the current
!response.ok branch in the sendClientRequest response handling so that you call
the interface's error conversion/propagation helper (e.g. a method on
this._interface such as
createClientErrorFromResponse/convertResponseToError/throwIfResponseError) with
the response and session and rethrow that result, rather than constructing a
StackAssertionError; keep the subsequent JSON parsing and redirect_url
validation only for successful responses. Ensure you remove the
StackAssertionError throw in that branch and use the interface method so
KnownErrors/status information is preserved.
---
Nitpick comments:
In @.cursor/commands/pr-comments-review.md:
- Line 1: Replace the informal phrase "mostly bullshit" in the documentation for
the pr-comments-review command with a more professional alternative (e.g., "not
actionable", "invalid", or "low-priority") and change wording from "fix" to
"address" where suggested; locate the text by searching for the exact string
"mostly bullshit" in .cursor/commands/pr-comments-review.md and update
surrounding sentences to keep tone consistent, then update any doc/help output
tests that assert the command's messaging (tests referencing pr-comments-review
help text or the exact phrase) so they assert the new phrasing; ensure to
resolve the PR comment after making these documentation and test updates.
🪄 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: 02fac0d2-9b1a-45f1-98e7-757f6c1028fc
📒 Files selected for processing (9)
.cursor/commands/pr-comments-review.mdapps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsxapps/backend/src/oauth/model.tsxclaude/CLAUDE-KNOWLEDGE.mdpackages/template/src/components-page/oauth-callback.tsxpackages/template/src/lib/auth.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/implementations/event-tracker.tspackages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/template/src/lib/auth.ts
- apps/backend/src/oauth/model.tsx
- claude/CLAUDE-KNOWLEDGE.md
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts
Outdated
Show resolved
Hide resolved
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (2)
536-536: Consider skipping prefetch on OAuth callback pages.The constructor unconditionally calls
_prefetchCrossDomainHandoffParamsIfNeeded(). On OAuth callback URLs (containingcodeandstatequery parameters), this creates unnecessary verifier/state cookies that won't be used, ascallOAuthCallback()consumes the verifier keyed by the state from the URL.While this doesn't break functionality (each state has its own cookie key), it's wasteful and could be avoided:
♻️ Optional: Skip prefetch on callback URLs
protected _prefetchCrossDomainHandoffParamsIfNeeded() { const canWriteOauthVerifierCookie = this._tokenStoreInit === "cookie" || this._tokenStoreInit === "nextjs-cookie"; if ( !isBrowserLike() || !canWriteOauthVerifierCookie || this._isPrefetchingCrossDomainHandoffParams || this._getFreshPrefetchedCrossDomainHandoffParams() != null ) { return; } + // Skip prefetch on OAuth callback pages - the existing verifier will be consumed + const currentUrl = new URL(window.location.href); + if (currentUrl.searchParams.has("code") && currentUrl.searchParams.has("state")) { + return; + } this._isPrefetchingCrossDomainHandoffParams = true;🤖 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` at line 536, The constructor currently always calls _prefetchCrossDomainHandoffParamsIfNeeded(), which needlessly creates verifier/state cookies on OAuth callback pages; update the logic to detect OAuth callback URLs (presence of both "code" and "state" query parameters) and skip calling _prefetchCrossDomainHandoffParamsIfNeeded() in that case (or add an early return inside _prefetchCrossDomainHandoffParamsIfNeeded() that checks for code+state). Ensure callOAuthCallback() continues to read the verifier keyed by the state from the URL and that no other behavior is changed.
2352-2354: API failures are now surfaced, but consider exposing the actual error.The code now checks
response.okbefore parsing, addressing the previous concern. The error message includes status and response text, which is helpful for debugging.However, the backend may return structured error responses (e.g.,
KnownErrors) that are currently collapsed into a genericStackAssertionError. Consider parsing and rethrowing the actual error type for better error handling upstream.🤖 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 2352 - 2354, The current check throws a generic StackAssertionError using response.status and text which loses structured backend errors; update the error path where response.ok is false (around the response handling that throws StackAssertionError) to attempt parsing the response body as JSON, detect known error shapes (e.g., fields like name, code, message or the backend's KnownErrors shape), and rethrow an appropriate typed error (or reconstruct the backend error) instead of always throwing StackAssertionError; if parsing fails or the payload doesn't match a known shape, then fall back to throwing StackAssertionError including status and raw text. Ensure this logic is applied where response is inspected and StackAssertionError is thrown so upstream code can handle specific error types.
🤖 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`:
- Line 536: The constructor currently always calls
_prefetchCrossDomainHandoffParamsIfNeeded(), which needlessly creates
verifier/state cookies on OAuth callback pages; update the logic to detect OAuth
callback URLs (presence of both "code" and "state" query parameters) and skip
calling _prefetchCrossDomainHandoffParamsIfNeeded() in that case (or add an
early return inside _prefetchCrossDomainHandoffParamsIfNeeded() that checks for
code+state). Ensure callOAuthCallback() continues to read the verifier keyed by
the state from the URL and that no other behavior is changed.
- Around line 2352-2354: The current check throws a generic StackAssertionError
using response.status and text which loses structured backend errors; update the
error path where response.ok is false (around the response handling that throws
StackAssertionError) to attempt parsing the response body as JSON, detect known
error shapes (e.g., fields like name, code, message or the backend's KnownErrors
shape), and rethrow an appropriate typed error (or reconstruct the backend
error) instead of always throwing StackAssertionError; if parsing fails or the
payload doesn't match a known shape, then fall back to throwing
StackAssertionError including status and raw text. Ensure this logic is applied
where response is inspected and StackAssertionError is thrown so upstream code
can handle specific error types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cd5db2e-da6e-42af-81ee-f92efcbcd03d
📒 Files selected for processing (10)
apps/backend/.env.developmentapps/backend/prisma/schema.prismaapps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsxapps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.tsclaude/CLAUDE-KNOWLEDGE.mdexamples/demo/.env.developmentexamples/demo/src/stack.tsxpackages/template/src/lib/auth.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/interfaces/client-app.ts
💤 Files with no reviewable changes (1)
- packages/template/src/lib/auth.ts
✅ Files skipped from review due to trivial changes (1)
- examples/demo/src/stack.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/e2e/tests/backend/endpoints/api/v1/auth/oauth/authorize.test.ts
- apps/backend/src/app/api/latest/auth/oauth/authorize/[provider_id]/route.tsx
- apps/backend/prisma/schema.prisma
- claude/CLAUDE-KNOWLEDGE.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
2263-2285:⚠️ Potential issue | 🟡 MinorSkip handoff prefetch on OAuth callback pages.
This still calls
saveVerifierAndState()on pages that already havecodeandstatein the URL. On those pages,callOAuthCallback()can only consume the verifier keyed by the callback state, so the extra pair is dead state at best and makes this auth path harder to reason about. Return early when the current URL already contains both callback params.🤖 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 2263 - 2285, In _prefetchCrossDomainHandoffParamsIfNeeded, avoid calling saveVerifierAndState on OAuth callback pages by returning early if the current browser URL already contains both "code" and "state" query params; add a check (after isBrowserLike() and before setting _isPrefetchingCrossDomainHandoffParams / invoking runAsynchronously) that inspects window.location.search via URLSearchParams for "code" and "state" and returns if both are present so callOAuthCallback can consume the existing verifier without creating a stray verifier/state pair.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 2246-2260: The urls getter currently returns plain resolved URLs
during SSR or while async prefetch hasn't produced PKCE/handoff state, which
lets initial <a href> links miss required cross-domain params; update the urls
getter (method: urls) so that on the client (when typeof window !== "undefined")
it guarantees handoff params synchronously before calling
resolveAppUrlsForCurrentPage: call
_getCrossDomainHandoffParamsForUrlsGetter(currentUrl) and if it returns null,
synchronously generate the minimal PKCE/handoff state (reuse the same logic used
by redirect flows) or fallback to a deterministic local handoff token, then call
resolveAppUrlsForCurrentPage with that non-null crossDomainHandoffParams and
_getLocalOAuthCallbackHandlerUrl so signIn/signUp/onboarding/signOut hrefs
always include handoff params on first render.
- Around line 2356-2360: The code uses the 'in' operator on result without
ensuring result is an object, which can throw if response.json() returned null,
a primitive, an array, or if JSON parsing failed; wrap the response.json() call
in a try/catch to convert JSON parse errors into a StackAssertionError, then
validate that result is a non-null plain object (typeof result === "object" &&
result !== null && !Array.isArray(result)) before checking ("redirect_url" in
result) and typeof result.redirect_url === "string"; throw a StackAssertionError
with the original result or caught error details when validation fails
(referencing response.json(), the local variable result, and
StackAssertionError).
---
Duplicate comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 2263-2285: In _prefetchCrossDomainHandoffParamsIfNeeded, avoid
calling saveVerifierAndState on OAuth callback pages by returning early if the
current browser URL already contains both "code" and "state" query params; add a
check (after isBrowserLike() and before setting
_isPrefetchingCrossDomainHandoffParams / invoking runAsynchronously) that
inspects window.location.search via URLSearchParams for "code" and "state" and
returns if both are present so callOAuthCallback can consume the existing
verifier without creating a stray verifier/state pair.
🪄 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: 9fa1c981-5c63-41b1-8c53-882fd987518a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
claude/CLAUDE-KNOWLEDGE.mdpackages/template/src/lib/env.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/implementations/event-tracker.ts
✅ Files skipped from review due to trivial changes (1)
- packages/template/src/lib/env.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts
- claude/CLAUDE-KNOWLEDGE.md
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Outdated
Show resolved
Hide resolved
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/template/src/lib/stack-app/url-targets.ts (1)
209-216: Build the hosted URL with encoded URL parts instead of raw replacement.
pagePathis inserted into the final URL verbatim here. That makes reserved characters alter the URL structure instead of staying inside the path, and it also bypasses the repo’s URL-construction convention. Please assemble this withurlString/encodeURIComponent()orURLpath setters rather than raw template replacement.As per coding guidelines: use
urlStringorencodeURIComponent()instead of normal string interpolation for URLs for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/url-targets.ts` around lines 209 - 216, The getHostedHandlerUrl function currently injects options.pagePath directly into templateFilled causing reserved characters to break URL structure; update getHostedHandlerUrl to encode URL parts instead of raw replacement: construct hostedPath from options.pagePath but pass its segments through encodeURIComponent (or use a URL instance and set pathname/path segments) and replace hostedHandlerPathPlaceholder and hostedHandlerProjectIdPlaceholder with encoded values so the final new URL(templateFilled) receives a valid, encoded URL string; ensure leading slashes are normalized before encoding so path segments remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/template/src/lib/stack-app/url-targets.ts`:
- Around line 178-190: The hosted URL template accepted by
getHostedHandlerUrlTemplate currently only checks for the presence of
placeholders hostedHandlerProjectIdPlaceholder and hostedHandlerPathPlaceholder
(via NEXT_PUBLIC_STACK_HOSTED_HANDLER_URL_TEMPLATE) but later
isHostedHandlerUrlForProject trusts only origin + pathname prefix; fix by
restricting/validating the template shape to a form the trust check can verify
(e.g., enforce a scheme + host + path pattern like
https://{projectId}.domain/{hostedPath} or ensure no query parameter placement
of placeholders), or alternatively change isHostedHandlerUrlForProject to
compare against the fully expanded template including query parameters; update
getHostedHandlerUrlTemplate to reject templates that place placeholders in query
strings or other unsupported locations and add clear error hint referencing
NEXT_PUBLIC_STACK_HOSTED_HANDLER_URL_TEMPLATE, hostedHandlerProjectIdPlaceholder
and hostedHandlerPathPlaceholder so only templates that match the
origin+pathname heuristic are allowed.
- Around line 192-206: resolveCustomTargetUrl currently rejects targets whose
handlerName isn't present in customPagePrompts before honoring the legacy
fast-path for version 0, which makes { handler: { type: "custom", url, version:
0 } } fail; update resolveCustomTargetUrl to short-circuit and return
options.target.url when options.target.version === 0 (or specifically when
handlerName === "handler" && options.target.version === 0) before checking
customPagePrompts, otherwise keep the existing behavior: if handlerName exists
in customPagePrompts validate the version against customPagePrompt.versions and
throw the existing unsupported-version error, and if not present (and not the v0
legacy case) throw the "cannot be a custom page" error.
---
Nitpick comments:
In `@packages/template/src/lib/stack-app/url-targets.ts`:
- Around line 209-216: The getHostedHandlerUrl function currently injects
options.pagePath directly into templateFilled causing reserved characters to
break URL structure; update getHostedHandlerUrl to encode URL parts instead of
raw replacement: construct hostedPath from options.pagePath but pass its
segments through encodeURIComponent (or use a URL instance and set pathname/path
segments) and replace hostedHandlerPathPlaceholder and
hostedHandlerProjectIdPlaceholder with encoded values so the final new
URL(templateFilled) receives a valid, encoded URL string; ensure leading slashes
are normalized before encoding so path segments remain correct.
🪄 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: 477fb853-8ad5-4238-9c31-bf8b39cc5daf
📒 Files selected for processing (5)
claude/CLAUDE-KNOWLEDGE.mdpackages/template/src/lib/stack-app/common.tspackages/template/src/lib/stack-app/index.tspackages/template/src/lib/stack-app/url-targets.test.tspackages/template/src/lib/stack-app/url-targets.ts
✅ Files skipped from review due to trivial changes (1)
- packages/template/src/lib/stack-app/url-targets.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/template/src/lib/stack-app/index.ts
- packages/template/src/lib/stack-app/common.ts
packages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts
Outdated
Show resolved
Hide resolved
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Show resolved
Hide resolved
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.
|
|
||
| this._prefetchCrossDomainHandoffParamsIfNeeded(); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Unused method left behind after static urls refactor
Low Severity
_getCrossDomainHandoffParamsForUrlsGetter is defined but never called. The urls getter was made static (confirmed by CLAUDE-KNOWLEDGE.md: "app.urls is now static and no longer injects runtime params"), so this method — which combines query-param parsing with prefetch fallback logic for the old dynamic urls getter — is dead code. The redirect flow uses _getCrossDomainHandoffParamsForRedirect instead.
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.
Other minor redirect URL changes:
Note
High Risk
Introduces new cross-domain OAuth authorization-code issuance and refresh-token reuse plumbing, plus a schema migration, touching security-sensitive auth and redirect validation paths.
Overview
Adds hosted handler URL targets to the JS/Template SDK (
urls.default: { type: "hosted" }), including env-driven hosted domain/template resolution, unknown/handler/*fallback routing, and updated examples to useredirectTo*()instead of relying on dynamicapp.urls.Implements cross-domain auth handoff: a new backend endpoint
POST /auth/oauth/cross-domain/authorizeissues a one-time PKCE authorization-code redirect only for authenticated, refresh-token-bound sessions (verifies provided refresh token ownership/validity), and the SDK plans/executes these redirects while preservingafter_auth_return_toand cross-domain query params.Tightens OAuth redirect handling by validating
after_callback_redirect_urlas a URL and whitelist-checking it, and persists refresh-token linkage by addingProjectUserAuthorizationCode.grantedRefreshTokenIdso token exchange can reuse the intended refresh-token session. Includes new migration tests, unit tests for URL targeting, and E2E coverage for cross-domain authorize and redirect URL validation.Written by Cursor Bugbot for commit d09c7c9. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests