From ea158be9ed99cc1d9b21393136cb5aa607d516d7 Mon Sep 17 00:00:00 2001 From: Konstantin Wohlwend Date: Fri, 27 Mar 2026 10:50:15 -0700 Subject: [PATCH] Various PR changes --- .cursor/commands/pr-comments-review.md | 2 +- .../oauth/cross-domain/authorize/route.tsx | 12 +++++- apps/backend/src/oauth/model.tsx | 9 ++--- claude/CLAUDE-KNOWLEDGE.md | 8 +++- .../src/components-page/oauth-callback.tsx | 4 +- packages/template/src/lib/auth.ts | 4 +- .../apps/implementations/client-app-impl.ts | 37 ++++++++++++++++-- .../apps/implementations/event-tracker.ts | 39 ++++++++++++++----- .../implementations/redirect-page-urls.ts | 17 ++++---- 9 files changed, 98 insertions(+), 34 deletions(-) diff --git a/.cursor/commands/pr-comments-review.md b/.cursor/commands/pr-comments-review.md index bc9d8784f..b8e4bcc58 100644 --- a/.cursor/commands/pr-comments-review.md +++ b/.cursor/commands/pr-comments-review.md @@ -1 +1 @@ -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. Report the result to me in detail. Do NOT automatically commit or stage the changes back to the PR! +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! diff --git a/apps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsx b/apps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsx index bca56f71f..972bd5235 100644 --- a/apps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsx +++ b/apps/backend/src/app/api/latest/auth/oauth/cross-domain/authorize/route.tsx @@ -1,3 +1,4 @@ +import { checkApiKeySet, throwCheckApiKeySetError } from "@/lib/internal-api-keys"; import { isAcceptedNativeAppUrl, validateRedirectUrl } from "@/lib/redirect-urls"; import { Tenancy } from "@/lib/tenancies"; import { isRefreshTokenValid } from "@/lib/tokens"; @@ -156,10 +157,19 @@ export const POST = createSmartRouteHandler({ id: user.id, refreshTokenId: refreshTokenObj.id, }; + const publishableClientKey = headers["x-stack-publishable-client-key"]?.[0] ?? publishableClientKeyNotNecessarySentinel; + const keyCheck = await checkApiKeySet(tenancy.project.id, { publishableClientKey }); + if (keyCheck.status === "error") { + throwCheckApiKeySetError( + keyCheck.error, + tenancy.project.id, + new KnownErrors.InvalidPublishableClientKey(tenancy.project.id), + ); + } const redirectUrl = await createCrossDomainAuthorizeRedirect({ tenancy, user: userWithSession, - publishableClientKey: headers["x-stack-publishable-client-key"]?.[0] ?? publishableClientKeyNotNecessarySentinel, + publishableClientKey, body, }); diff --git a/apps/backend/src/oauth/model.tsx b/apps/backend/src/oauth/model.tsx index 9e749796b..4953c2331 100644 --- a/apps/backend/src/oauth/model.tsx +++ b/apps/backend/src/oauth/model.tsx @@ -8,7 +8,7 @@ import { createRefreshTokenObj, decodeAccessToken, generateAccessTokenFromRefres import { getPrismaClientForTenancy, globalPrismaClient } from "@/prisma-client"; import { AuthorizationCode, AuthorizationCodeModel, Client, Falsey, RefreshToken, Token, User } from "@node-oauth/oauth2-server"; import { KnownErrors } from "@stackframe/stack-shared"; -import { StackAssertionError, captureError, throwErr } from "@stackframe/stack-shared/dist/utils/errors"; +import { StackAssertionError, StatusError, captureError, throwErr } from "@stackframe/stack-shared/dist/utils/errors"; import { getProjectBranchFromClientId } from "."; const PrismaClientKnownRequestError = Prisma.PrismaClientKnownRequestError; @@ -124,10 +124,7 @@ export class OAuthModel implements AuthorizationCodeModel { }, }); if (refreshTokenObj && refreshTokenObj.projectUserId !== user.id) { - throw new StackAssertionError("Cross-domain handoff refresh token does not belong to the authenticated user", { - refreshTokenProjectUserId: refreshTokenObj.projectUserId, - userId: user.id, - }); + throw new StatusError(401, "Cross-domain handoff refresh token does not belong to the authenticated user."); } if (refreshTokenObj && await isRefreshTokenValid({ tenancy, refreshTokenObj })) { return refreshTokenObj; @@ -151,7 +148,7 @@ export class OAuthModel implements AuthorizationCodeModel { } async saveToken(token: Token, client: Client, user: User): Promise { - const afterCallbackRedirectUrl = user.afterCallbackRedirectUrl; + const afterCallbackRedirectUrl = user.afterCallbackRedirectUrl ?? null; if (token.refreshToken) { const tenancy = await getSoleTenancyFromProjectBranch(...getProjectBranchFromClientId(client.id)); diff --git a/claude/CLAUDE-KNOWLEDGE.md b/claude/CLAUDE-KNOWLEDGE.md index 5b4587304..11fb33666 100644 --- a/claude/CLAUDE-KNOWLEDGE.md +++ b/claude/CLAUDE-KNOWLEDGE.md @@ -80,7 +80,7 @@ Q: How should unsubscribe-link e2e tests avoid breakage from email theme/layout A: In `apps/e2e/tests/backend/endpoints/api/v1/unsubscribe-link.test.ts`, avoid snapshotting the entire rendered HTML for transactional emails; assert stable behavior instead (email content present and `/api/v1/emails/unsubscribe-link` absent) so cosmetic wrapper/style changes do not fail the test. Q: How do cross-domain auth handoffs avoid creating extra refresh-token sessions? -A: The cross-domain authorize route must carry the current `refreshTokenId` through authorization-code exchange and OAuth token issuance must reuse that ID. We encode handoff metadata (`afterCallbackRedirectUrl`, `refreshTokenId`) into a prefixed payload stored in `afterCallbackRedirectUrl`, decode it in `OAuthModel._getOrCreateRefreshTokenObj`, and sanitize `after_callback_redirect_url` in `saveToken` so clients only receive the original redirect URL. +A: The cross-domain authorize route must carry the current `refreshTokenId` through authorization-code exchange and OAuth token issuance must reuse that ID. Keep `afterCallbackRedirectUrl` URL-only and persist refresh-token linkage in `ProjectUserAuthorizationCode.grantedRefreshTokenId`; then return that as `user.refreshTokenId` in `getAuthorizationCode` so token issuance can reuse the same refresh-token row with ownership checks. Q: Is there a manual demo page for cross-domain auth handoff verification? A: Yes — `examples/demo/src/app/cross-domain-handoff/page.tsx` provides one-click triggers for client sign-in/sign-up redirects, server protected-page redirects, and OAuth provider sign-in, plus runtime URL visibility for manual verification. @@ -97,6 +97,9 @@ A: Cross-domain handoff should still run when handoff params indicate a differen Q: How should `app.urls.signIn`/`signOut` behave for hosted cross-domain flows? A: In browser contexts, `app.urls` should return redirect-ready handler URLs for `signIn`, `signUp`, `onboarding`, and `signOut`: include `after_auth_return_to`, preserve existing cross-domain handoff params, and for hosted sign-in/up/onboarding populate cross-domain callback targets (`/handler/oauth-callback` with `stack_cross_domain_auth=1`) so plain `router.push(app.urls.signIn)` / `` keeps return-to-domain behavior. +Q: What should happen if hosted `after_auth_return_to` requires cross-domain handoff but URL params are missing? +A: In `planRedirectToHandler` (`redirect-page-urls.ts`), do not throw immediately. Generate missing PKCE handoff `state`/`codeChallenge` via `getCrossDomainHandoffParams(currentUrl)` and default `afterCallbackRedirectUrl` to `currentUrl.toString()`, then continue with cross-domain authorize planning. + Q: How do we avoid duplicating redirect-back URL logic between `app.urls` and `redirectTo*`? A: Keep redirect math and handler policy decisions in `redirect-page-urls.ts` (for example `resolveAppUrlsForCurrentPage`, `resolveRedirectBackAwareHandlerUrlForRedirect`, `getHandlerRedirectPolicy`) and keep `client-app-impl` as orchestration only (state/cookies/network/redirect side effects). Redirect execution should start from raw resolved URLs (not `this.urls`) so `noRedirectBack` still works as expected. @@ -118,6 +121,9 @@ A: E2E JS tests import `@stackframe/js` from built `dist`, so new helper files c Q: How should dashboard pages update project config values? A: Do not call `project.updateConfig(...)` directly from dashboard pages; lint enforces using `useUpdateConfig()` from `apps/dashboard/src/lib/config-update.tsx` so pushable-config confirmation flows are handled consistently. +Q: How should EventTracker behave in test environments with partial DOM mocks? +A: In `packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts`, gate `start()` behind runtime capability checks (DOM listener APIs and screen dimensions), and patch `window.history` instead of global `history`. This prevents crashes like `Cannot read properties of undefined (reading 'width')` in non-browser test stubs while keeping browser behavior unchanged. + Q: How can the dashboard find resumable onboarding state without SDK type changes? A: Query `/internal/projects` via `stackAppInternalsSymbol` and read each project's `onboarding_status`; this avoids relying on `AdminOwnedProject` fields that may lag until generated package copies are rebuilt. diff --git a/packages/template/src/components-page/oauth-callback.tsx b/packages/template/src/components-page/oauth-callback.tsx index b23c1c021..44694ee9f 100644 --- a/packages/template/src/components-page/oauth-callback.tsx +++ b/packages/template/src/components-page/oauth-callback.tsx @@ -21,13 +21,15 @@ export function OAuthCallback({ fullPage }: { fullPage?: boolean }) { if (called.current) return; called.current = true; let hasRedirected = false; + let callbackError: unknown = null; try { hasRedirected = await app.callOAuthCallback(); } catch (e) { + callbackError = e; captureError("", e); setError(e); } - if (!hasRedirected && (!error || envVars.NODE_ENV === 'production')) { + if (!hasRedirected && (callbackError == null || envVars.NODE_ENV === "production")) { await app.redirectToSignIn({ noRedirectBack: true }); } }), []); diff --git a/packages/template/src/lib/auth.ts b/packages/template/src/lib/auth.ts index 81644b495..d5d4e68cd 100644 --- a/packages/template/src/lib/auth.ts +++ b/packages/template/src/lib/auth.ts @@ -23,7 +23,9 @@ export async function signInWithOAuth( provider: options.provider, redirectUrl: constructRedirectUrl(options.redirectUrl, "redirectUrl"), errorRedirectUrl: constructRedirectUrl(options.errorRedirectUrl, "errorRedirectUrl"), - afterCallbackRedirectUrl: options.afterCallbackRedirectUrl, + afterCallbackRedirectUrl: options.afterCallbackRedirectUrl == null + ? undefined + : constructRedirectUrl(options.afterCallbackRedirectUrl, "afterCallbackRedirectUrl"), codeChallenge, state, type: "authenticate", diff --git a/packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts b/packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts index 4498c1aec..1bcf3d8d0 100644 --- a/packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts +++ b/packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts @@ -73,6 +73,8 @@ isReactServer = sc.isReactServer; const NextNavigation = scrambleDuringCompileTime(NextNavigationUnscrambled); // END_PLATFORM +const prefetchedCrossDomainHandoffTtlMs = 55 * 60 * 1000; + const allClientApps = new Map]>(); @@ -374,6 +376,7 @@ export class _StackClientAppImplIncomplete | null = null; private _prefetchedCrossDomainHandoffParams: CrossDomainHandoffParams | null = null; + private _prefetchedCrossDomainHandoffParamsFetchedAt = 0; private _isPrefetchingCrossDomainHandoffParams = false; protected async _createCookieHelper(overrideTokenStoreInit?: TokenStoreInit): Promise { @@ -2162,18 +2165,22 @@ export class _StackClientAppImplIncomplete { try { + if (!isBrowserLike()) { + return; + } const { state, codeChallenge } = await saveVerifierAndState(); this._prefetchedCrossDomainHandoffParams = { state, codeChallenge }; + this._prefetchedCrossDomainHandoffParamsFetchedAt = performance.now(); } finally { this._isPrefetchingCrossDomainHandoffParams = false; } @@ -2186,8 +2193,9 @@ export class _StackClientAppImplIncomplete prefetchedCrossDomainHandoffTtlMs) { + this._prefetchedCrossDomainHandoffParams = null; + this._prefetchedCrossDomainHandoffParamsFetchedAt = 0; + return null; + } + return this._prefetchedCrossDomainHandoffParams; + } + protected async _getCurrentUrl() { if (this._redirectMethod === "none") { return null; diff --git a/packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts b/packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts index f9d063753..6bd58b238 100644 --- a/packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts +++ b/packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts @@ -31,8 +31,8 @@ export class EventTracker { private readonly _sessionReplaySegmentId: string; private readonly _deps: EventTrackerDeps; - private _originalPushState: typeof history.pushState | null = null; - private _originalReplaceState: typeof history.replaceState | null = null; + private _originalPushState: History["pushState"] | null = null; + private _originalReplaceState: History["replaceState"] | null = null; constructor(deps: EventTrackerDeps) { this._deps = deps; @@ -42,6 +42,18 @@ export class EventTracker { start() { if (this._started) return; if (!isBrowserLike()) return; + const screenObject = Reflect.get(window, "screen"); + if ( + typeof window.addEventListener !== "function" + || typeof window.removeEventListener !== "function" + || typeof document.addEventListener !== "function" + || typeof document.removeEventListener !== "function" + || typeof screenObject !== "object" + || typeof Reflect.get(screenObject, "width") !== "number" + || typeof Reflect.get(screenObject, "height") !== "number" + ) { + return; + } this._started = true; this._setupPageViewCapture(); @@ -99,20 +111,26 @@ export class EventTracker { private _setupPageViewCapture() { // Fire initial page-view this._capturePageView("initial"); + const historyObject = window["history"]; + const originalPushState = Reflect.get(historyObject, "pushState"); + const originalReplaceState = Reflect.get(historyObject, "replaceState"); + if (typeof originalPushState !== "function" || typeof originalReplaceState !== "function") { + return; + } // Monkey-patch history.pushState - this._originalPushState = history.pushState.bind(history); - history.pushState = (...args: Parameters) => { + this._originalPushState = (...args: Parameters) => originalPushState.apply(historyObject, args); + Reflect.set(historyObject, "pushState", (...args: Parameters) => { this._originalPushState!(...args); this._capturePageView("push"); - }; + }); // Monkey-patch history.replaceState - this._originalReplaceState = history.replaceState.bind(history); - history.replaceState = (...args: Parameters) => { + this._originalReplaceState = (...args: Parameters) => originalReplaceState.apply(historyObject, args); + Reflect.set(historyObject, "replaceState", (...args: Parameters) => { this._originalReplaceState!(...args); this._capturePageView("replace"); - }; + }); // Listen for popstate (back/forward navigation) window.addEventListener("popstate", this._onPopState); @@ -205,12 +223,13 @@ export class EventTracker { } // Restore history methods + const historyObject = Reflect.get(window, "history"); if (this._originalPushState) { - history.pushState = this._originalPushState; + Reflect.set(historyObject, "pushState", this._originalPushState); this._originalPushState = null; } if (this._originalReplaceState) { - history.replaceState = this._originalReplaceState; + Reflect.set(historyObject, "replaceState", this._originalReplaceState); this._originalReplaceState = null; } diff --git a/packages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts b/packages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts index 3d6e2de73..3fbbd822c 100644 --- a/packages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts +++ b/packages/template/src/lib/stack-app/apps/implementations/redirect-page-urls.ts @@ -257,16 +257,15 @@ export async function planRedirectToHandler(options: { if (crossDomainHandoff == null) { return { type: "redirect", url: redirectBackUrl }; } - const state = crossDomainHandoff.handoffParams.state; - const codeChallenge = crossDomainHandoff.handoffParams.codeChallenge; - const afterCallbackRedirectUrl = crossDomainHandoff.handoffParams.afterCallbackRedirectUrl; - if (state == null || codeChallenge == null || afterCallbackRedirectUrl == null) { - throw new StackAssertionError("Cross-domain handoff policy was selected but required params were missing", { - state, - codeChallenge, - afterCallbackRedirectUrl, - }); + let state = crossDomainHandoff.handoffParams.state; + let codeChallenge = crossDomainHandoff.handoffParams.codeChallenge; + let afterCallbackRedirectUrl = crossDomainHandoff.handoffParams.afterCallbackRedirectUrl; + if (state == null || codeChallenge == null) { + const generatedHandoffParams = await options.getCrossDomainHandoffParams(options.currentUrl); + state ??= generatedHandoffParams.state; + codeChallenge ??= generatedHandoffParams.codeChallenge; } + afterCallbackRedirectUrl ??= options.currentUrl.toString(); return { type: "cross-domain-authorize", redirectUri: crossDomainHandoff.redirectBackTarget.toString(),