Skip to content

Commit e2dc5f5

Browse files
mantrakp04mantrakp04
authored
[codex] fix OAuth redirect contract (#1393)
## Summary - Route browser OAuth redirects through the configured `redirectMethod` instead of hardcoded `window.location` calls. - Keep OAuth redirect APIs pending after navigation starts, including custom redirect methods. - Add `cliAuthConfirm` handler URL metadata and custom-page prompt coverage. - Update SDK spec text for browser OAuth callback and `returnTo` behavior. ## Root Cause OAuth helpers previously combined URL construction with direct browser navigation. That bypassed configured redirect methods and made it too easy for public redirect APIs to resolve after navigation started. ## Impact Browser SDK consumers get consistent redirect behavior across built-in and custom navigation methods. `returnTo` is handled as the post-callback destination while the OAuth callback URL remains fixed to the configured handler route. ## Validation - `pnpm test run packages/template/src/lib/auth.test.ts` - `pnpm test run apps/e2e/tests/js/oauth.test.ts` - `pnpm -C packages/template lint` - `pnpm -C apps/e2e lint` - `pnpm -C packages/template typecheck` - `pnpm -C apps/e2e typecheck` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added CLI authorization confirmation page/flow for terminal-based auth. * Added optional returnTo parameter for OAuth to control post-auth redirects. * Exposed configurable redirect behavior so apps follow the chosen redirect method. * **Bug Fixes** * OAuth callback now uses app navigation/queued redirects and shows a fallback link instead of forcing location.assign. * **Tests** * Added unit and e2e tests covering OAuth URL generation, scope handling, and CLI auth confirmation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 5e5cfde commit e2dc5f5

8 files changed

Lines changed: 197 additions & 26 deletions

File tree

‎apps/e2e/tests/js/oauth.test.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ it("adds provider_scope from oauthScopesOnSignIn for authenticate flow", async (
1717
},
1818
{
1919
client: {
20+
redirectMethod: "window",
2021
oauthScopesOnSignIn: {
2122
github: ["repo"],
2223
},
@@ -52,4 +53,56 @@ it("adds provider_scope from oauthScopesOnSignIn for authenticate flow", async (
5253
expect(scope).toBe("user:email repo");
5354
}, { timeout: 40_000 });
5455

56+
it("does not resolve signInWithOAuth after a custom redirectMethod starts navigation", async ({ expect }) => {
57+
const navigatedUrls: string[] = [];
58+
const { clientApp } = await createApp(
59+
{
60+
config: {
61+
oauthProviders: [
62+
{
63+
id: "github",
64+
type: "standard",
65+
clientId: "test_client_id",
66+
clientSecret: "test_client_secret",
67+
},
68+
],
69+
},
70+
},
71+
{
72+
client: {
73+
redirectMethod: {
74+
useNavigate: () => (url) => {
75+
navigatedUrls.push(url);
76+
},
77+
navigate: (url) => {
78+
navigatedUrls.push(url);
79+
},
80+
},
81+
},
82+
}
83+
);
84+
85+
const previousWindow = globalThis.window;
86+
const previousDocument = globalThis.document;
87+
globalThis.document = { cookie: "", createElement: () => ({}) } as any;
88+
globalThis.window = {
89+
location: {
90+
href: localRedirectUrl,
91+
},
92+
} as any;
5593

94+
try {
95+
const redirectResult = clientApp.signInWithOAuth("github").then(() => "resolved");
96+
const result = await Promise.race([
97+
redirectResult,
98+
new Promise<string>((resolve) => setTimeout(() => resolve("pending"), 5000)),
99+
]);
100+
101+
expect(navigatedUrls).toHaveLength(1);
102+
expect(new URL(navigatedUrls[0]).pathname).toBe("/login/oauth/authorize");
103+
expect(result).toBe("pending");
104+
} finally {
105+
globalThis.window = previousWindow;
106+
globalThis.document = previousDocument;
107+
}
108+
}, { timeout: 40_000 });

‎packages/template/src/components-page/oauth-callback.tsx‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,27 @@ import { useEffect, useRef, useState } from "react";
88
import { useStackApp } from "..";
99
import { MaybeFullPage } from "../components/elements/maybe-full-page";
1010
import { StyledLink } from "../components/link";
11+
import { stackAppInternalsSymbol } from "../lib/stack-app";
1112
import { useTranslation } from "../lib/translations";
1213

1314
export function OAuthCallback({ fullPage }: { fullPage?: boolean }) {
1415
const { t } = useTranslation();
1516
const app = useStackApp();
1617
const called = useRef(false);
1718
const [showRedirectLink, setShowRedirectLink] = useState(false);
19+
const [redirectUrl, setRedirectUrl] = useState<string | null>(null);
1820

1921
useEffect(() => runAsynchronously(async () => {
2022
if (called.current) return;
2123
called.current = true;
24+
const redirectToError = async (url: URL) => {
25+
const urlString = url.toString();
26+
if (app[stackAppInternalsSymbol].getRedirectMethod() === "none") {
27+
setRedirectUrl(urlString);
28+
return;
29+
}
30+
await app[stackAppInternalsSymbol].redirectToUrl(urlString, { replace: true });
31+
};
2232
try {
2333
const hasRedirected = await app.callOAuthCallback();
2434
if (!hasRedirected) {
@@ -30,13 +40,13 @@ export function OAuthCallback({ fullPage }: { fullPage?: boolean }) {
3040
errorUrl.searchParams.set("errorCode", e.errorCode);
3141
errorUrl.searchParams.set("message", e.message);
3242
errorUrl.searchParams.set("details", JSON.stringify(e.details ?? {}));
33-
window.location.replace(errorUrl.toString());
43+
await redirectToError(errorUrl);
3444
return;
3545
}
3646
captureError("<OAuthCallback />", e);
37-
window.location.replace(new URL(app.urls.error, window.location.href).toString());
47+
await redirectToError(new URL(app.urls.error, window.location.href));
3848
}
39-
}), []);
49+
}), [app]);
4050

4151
useEffect(() => {
4252
setTimeout(() => setShowRedirectLink(true), 3000);
@@ -56,7 +66,7 @@ export function OAuthCallback({ fullPage }: { fullPage?: boolean }) {
5666
<div className="flex flex-col justify-center items-center gap-4">
5767
<Spinner size={20} />
5868
</div>
59-
{showRedirectLink ? <p>{t('If you are not redirected automatically, ')}<StyledLink className="whitespace-nowrap" href={app.urls.home}>{t("click here")}</StyledLink></p> : null}
69+
{showRedirectLink || redirectUrl != null ? <p>{t('If you are not redirected automatically, ')}<StyledLink className="whitespace-nowrap" href={redirectUrl ?? app.urls.home}>{t("click here")}</StyledLink></p> : null}
6070
</div>
6171
</MaybeFullPage>
6272
);

‎packages/template/src/components-page/stack-handler-client.tsx‎

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ import { FilterUndefined, filterUndefined } from "@stackframe/stack-shared/dist/
55
import { getRelativePart } from "@stackframe/stack-shared/dist/utils/urls";
66
import { notFound, redirect, RedirectType, usePathname, useSearchParams } from 'next/navigation'; // THIS_LINE_PLATFORM next
77
import { useMemo } from 'react';
8+
/* IF_PLATFORM react
9+
import { useEffect, useRef } from 'react';
10+
// END_PLATFORM */
811
import { SignIn, SignUp, StackServerApp } from "..";
912
import { useStackApp } from "../lib/hooks";
1013
import { HandlerUrls, StackClientApp, stackAppInternalsSymbol } from "../lib/stack-app";
@@ -230,8 +233,12 @@ export function StackHandlerClient(props: BaseHandlerProps & Partial<RouteProps>
230233
const currentLocation = pathname;
231234
const searchParamsSource = searchParamsFromHook;
232235
/* ELSE_IF_PLATFORM react
236+
const navigate = stackApp.useNavigate();
237+
const navigateRef = useRef(navigate);
238+
navigateRef.current = navigate;
233239
const currentLocation = props.location ?? window.location.pathname;
234240
const searchParamsSource = new URLSearchParams(window.location.search);
241+
const redirectTargets: (string | undefined)[] = [];
235242
END_PLATFORM */
236243

237244
const { path, searchParams, handlerPath } = useMemo(() => {
@@ -278,7 +285,7 @@ export function StackHandlerClient(props: BaseHandlerProps & Partial<RouteProps>
278285
// IF_PLATFORM next
279286
redirect(toAbsoluteOrRelativeRedirectTarget(urlObj), RedirectType.replace);
280287
/* ELSE_IF_PLATFORM react
281-
window.location.href = toAbsoluteOrRelativeRedirectTarget(urlObj);
288+
redirectTargets.push(toAbsoluteOrRelativeRedirectTarget(urlObj));
282289
END_PLATFORM */
283290
};
284291

@@ -312,11 +319,38 @@ export function StackHandlerClient(props: BaseHandlerProps & Partial<RouteProps>
312319
// IF_PLATFORM next
313320
redirect(result.redirect, RedirectType.replace);
314321
/* ELSE_IF_PLATFORM react
315-
window.location.href = result.redirect;
316-
return null;
322+
redirectTargets.push(result.redirect);
317323
END_PLATFORM */
318324
}
319325

326+
/* IF_PLATFORM react
327+
const redirectTarget = redirectTargets[0];
328+
const shouldRenderRedirectFallback = redirectTarget != null && stackApp[stackAppInternalsSymbol].getRedirectMethod() === "none";
329+
useEffect(() => {
330+
if (redirectTarget == null || shouldRenderRedirectFallback) {
331+
return;
332+
}
333+
navigateRef.current(redirectTarget);
334+
}, [redirectTarget, shouldRenderRedirectFallback]);
335+
336+
if (redirectTarget != null && shouldRenderRedirectFallback) {
337+
return (
338+
<MessageCard
339+
title="Continue"
340+
fullPage={props.fullPage}
341+
primaryButtonText="Continue"
342+
primaryAction={() => window.location.assign(redirectTarget)}
343+
>
344+
Continue to the next page.
345+
</MessageCard>
346+
);
347+
}
348+
349+
if (redirectTarget != null) {
350+
return null;
351+
}
352+
END_PLATFORM */
353+
320354
return result;
321355
}
322356

‎packages/template/src/lib/auth.test.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
// @vitest-environment jsdom
2+
3+
import { StackClientInterface } from "@stackframe/stack-shared";
4+
import { describe, expect, it, vi } from "vitest";
5+
import { getNewOAuthProviderOrScopeUrl } from "./auth";
6+
7+
vi.mock("./cookie", async (importOriginal) => {
8+
const actual = await importOriginal<typeof import("./cookie")>();
9+
return {
10+
...actual,
11+
saveVerifierAndState: async () => ({
12+
codeChallenge: "<stripped code challenge>",
13+
state: "<stripped state>",
14+
}),
15+
};
16+
});
17+
18+
describe("getNewOAuthProviderOrScopeUrl", () => {
19+
it("returns the OAuth URL without performing navigation", async () => {
20+
window.history.replaceState({}, "", "/account?after_auth_return_to=%2Fsettings");
21+
22+
const iface = new StackClientInterface({
23+
clientVersion: "test",
24+
getBaseUrl: () => "https://api.example.com",
25+
getApiUrls: () => ["https://api.example.com"],
26+
extraRequestHeaders: {},
27+
projectId: "00000000-0000-4000-8000-000000000000",
28+
publishableClientKey: "pck_test",
29+
});
30+
const session = iface.createSession({ refreshToken: null, accessToken: null });
31+
32+
const location = await getNewOAuthProviderOrScopeUrl(
33+
iface,
34+
{
35+
provider: "github",
36+
redirectUrl: "/handler/oauth-callback",
37+
errorRedirectUrl: "/handler/error",
38+
providerScope: "repo user",
39+
},
40+
session,
41+
);
42+
43+
const url = new URL(location);
44+
expect(`${url.origin}${url.pathname}`).toBe("https://api.example.com/api/v1/auth/oauth/authorize/github");
45+
expect(Object.fromEntries(url.searchParams.entries())).toMatchInlineSnapshot(`
46+
{
47+
"after_callback_redirect_url": "http://localhost:3000/account?after_auth_return_to=%2Fsettings",
48+
"client_id": "00000000-0000-4000-8000-000000000000",
49+
"client_secret": "pck_test",
50+
"code_challenge": "<stripped code challenge>",
51+
"code_challenge_method": "S256",
52+
"error_redirect_url": "http://localhost:3000/handler/error?after_auth_return_to=%2Fsettings",
53+
"grant_type": "authorization_code",
54+
"provider_scope": "repo user",
55+
"redirect_uri": "http://localhost:3000/handler/oauth-callback?after_auth_return_to=%2Fsettings",
56+
"response_type": "code",
57+
"scope": "legacy",
58+
"state": "<stripped state>",
59+
"type": "link",
60+
}
61+
`);
62+
});
63+
});

‎packages/template/src/lib/auth.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
11
import { KnownError, StackClientInterface } from "@stackframe/stack-shared";
22
import { InternalSession } from "@stackframe/stack-shared/dist/sessions";
33
import { StackAssertionError, throwErr } from "@stackframe/stack-shared/dist/utils/errors";
4-
import { neverResolve } from "@stackframe/stack-shared/dist/utils/promises";
54
import { Result } from "@stackframe/stack-shared/dist/utils/results";
65
import { deindent } from "@stackframe/stack-shared/dist/utils/strings";
76
import { constructRedirectUrl } from "../utils/url";
87
import { consumeVerifierAndStateCookie, saveVerifierAndState } from "./cookie";
9-
export async function addNewOAuthProviderOrScope(
8+
export async function getNewOAuthProviderOrScopeUrl(
109
iface: StackClientInterface,
1110
options: {
1211
provider: string,
@@ -15,9 +14,9 @@ export async function addNewOAuthProviderOrScope(
1514
providerScope?: string,
1615
},
1716
session: InternalSession,
18-
) {
17+
): Promise<string> {
1918
const { codeChallenge, state } = await saveVerifierAndState();
20-
const location = await iface.getOAuthUrl({
19+
return await iface.getOAuthUrl({
2120
provider: options.provider,
2221
redirectUrl: constructRedirectUrl(options.redirectUrl, "redirectUrl"),
2322
errorRedirectUrl: constructRedirectUrl(options.errorRedirectUrl, "errorRedirectUrl"),
@@ -28,8 +27,6 @@ export async function addNewOAuthProviderOrScope(
2827
session,
2928
providerScope: options.providerScope,
3029
});
31-
window.location.assign(location);
32-
await neverResolve();
3330
}
3431

3532
/**

‎packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ import * as NextNavigationUnscrambled from "next/navigation"; // import the enti
4141
import React, { useCallback, useMemo } from "react"; // THIS_LINE_PLATFORM react-like
4242
import type * as yup from "yup";
4343
import { constructRedirectUrl } from "../../../../utils/url";
44-
import { addNewOAuthProviderOrScope, callOAuthCallback } from "../../../auth";
44+
import { getNewOAuthProviderOrScopeUrl, callOAuthCallback } from "../../../auth";
4545
import { CookieHelper, createBrowserCookieHelper, createCookieHelper, createPlaceholderCookieHelper, deleteCookie, deleteCookieClient, isSecure as isSecureCookieContext, saveVerifierAndState, setOrDeleteCookie, setOrDeleteCookieClient } from "../../../cookie";
4646
import { envVars } from "../../../env";
4747
import { ApiKey, ApiKeyCreationOptions, ApiKeyUpdateOptions, apiKeyCreationOptionsToCrud } from "../../api-keys";
@@ -220,7 +220,7 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
220220
}
221221
}
222222

223-
await addNewOAuthProviderOrScope(
223+
const location = await getNewOAuthProviderOrScopeUrl(
224224
this._interface,
225225
{
226226
provider,
@@ -230,6 +230,7 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
230230
},
231231
session,
232232
);
233+
await this._redirectTo({ url: location });
233234
return await neverResolve();
234235
}
235236
);
@@ -423,7 +424,7 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
423424
Often, you can solve this by calling this function in the browser instead, or by removing the 'or: redirect' option and dealing with the case where the user doesn't have enough permissions.
424425
`);
425426
}
426-
await addNewOAuthProviderOrScope(
427+
const location = await getNewOAuthProviderOrScopeUrl(
427428
this._interface,
428429
{
429430
provider: options.providerId,
@@ -433,6 +434,7 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
433434
},
434435
options.session,
435436
);
437+
await this._redirectTo({ url: location });
436438
return await neverResolve();
437439
} else if (!hasConnection) {
438440
return null;
@@ -1677,7 +1679,7 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
16771679
// END_PLATFORM
16781680
async linkConnectedAccount(provider: string, options?: { scopes?: string[] }) {
16791681
const scopeString = options?.scopes?.join(" ") ?? "";
1680-
await addNewOAuthProviderOrScope(
1682+
const location = await getNewOAuthProviderOrScopeUrl(
16811683
app._interface,
16821684
{
16831685
provider,
@@ -1687,8 +1689,8 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
16871689
},
16881690
session,
16891691
);
1690-
// This won't actually be reached since addNewOAuthProviderOrScope redirects
1691-
await neverResolve();
1692+
await app._redirectTo({ url: location });
1693+
return await neverResolve();
16921694
},
16931695
async getOrLinkConnectedAccount(provider: string, options?: { scopes?: string[] }) {
16941696
const connectedAccounts = Result.orThrow(await app._currentUserConnectedAccountsCache.getOrWait([session], "write-only"));
@@ -2796,16 +2798,20 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
27962798
this._ensurePersistentTokenStore();
27972799
const session = await this._getSession();
27982800
const currentUrl = new URL(window.location.href);
2799-
const afterCallbackRedirectUrl = currentUrl.searchParams.has("after_auth_return_to")
2800-
? currentUrl.toString()
2801-
: undefined;
2801+
const afterCallbackRedirectUrl = options?.returnTo != null
2802+
? constructRedirectUrl(options.returnTo, "returnTo")
2803+
: (
2804+
currentUrl.searchParams.has("after_auth_return_to")
2805+
? currentUrl.toString()
2806+
: undefined
2807+
);
28022808
const siteKeys = this._getBotChallengeSiteKeys();
28032809
const { codeChallenge, state } = await saveVerifierAndState();
28042810

28052811
const executeOAuth = async (challenge: { token?: string, phase?: "invisible" | "visible", unavailable?: true }) => {
28062812
return await this._interface.authorizeOAuth({
28072813
provider,
2808-
redirectUrl: constructRedirectUrl(options?.returnTo ?? this.urls.oauthCallback, "redirectUrl"),
2814+
redirectUrl: constructRedirectUrl(this.urls.oauthCallback, "redirectUrl"),
28092815
errorRedirectUrl: constructRedirectUrl(this.urls.error, "errorRedirectUrl"),
28102816
afterCallbackRedirectUrl,
28112817
type: "authenticate",
@@ -2844,7 +2850,7 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
28442850
}
28452851

28462852
const location = Result.orThrow(authorizeResult);
2847-
window.location.assign(location);
2853+
await this._redirectTo({ url: location });
28482854
await neverResolve();
28492855
}
28502856

@@ -3475,6 +3481,10 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
34753481
) => {
34763482
return await this._interface.sendClientRequest(path, requestOptions, await this._getSession(), requestType);
34773483
},
3484+
getRedirectMethod: () => this._redirectMethod ?? throwErr("Redirect method should have been initialized in the Stack client app constructor"),
3485+
redirectToUrl: async (url: string | URL, options?: { replace?: boolean }) => {
3486+
await this._redirectTo({ url, ...options });
3487+
},
34783488
refreshOwnedProjects: async () => {
34793489
await this._refreshOwnedProjects(await this._getSession());
34803490
},

0 commit comments

Comments
 (0)