From 8afe2a7cea4d08d88ecdb10f585c058a9ae22d2f Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Sat, 28 Mar 2026 20:05:29 +0100 Subject: [PATCH] fix(oauth-provider): return JSON redirects from post-login OAuth continuation (#8815) --- packages/oauth-provider/src/authorize.ts | 33 +- packages/oauth-provider/src/continue.ts | 1 + packages/oauth-provider/src/oauth.test.ts | 568 +++++++++++++++++++++- packages/oauth-provider/src/oauth.ts | 13 + 4 files changed, 577 insertions(+), 38 deletions(-) diff --git a/packages/oauth-provider/src/authorize.ts b/packages/oauth-provider/src/authorize.ts index a6e1d896ff..ec97a07ae2 100644 --- a/packages/oauth-provider/src/authorize.ts +++ b/packages/oauth-provider/src/authorize.ts @@ -175,13 +175,15 @@ export async function authorizeEndpoint( }); if (!query.client_id) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL(ctx, "invalid_client", "client_id is required"), ); } if (!query.response_type) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL(ctx, "invalid_request", "response_type is required"), ); } @@ -191,7 +193,8 @@ export async function authorizeEndpoint( : undefined; const promptNone = promptSet?.has("none") ?? false; if (promptSet?.has("select_account") && !opts.selectAccount?.page) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL( ctx, `unsupported_prompt_select_account`, @@ -201,7 +204,8 @@ export async function authorizeEndpoint( } if (!(query.response_type === "code")) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL( ctx, "unsupported_response_type", @@ -213,12 +217,14 @@ export async function authorizeEndpoint( // Check client const client = await getClient(ctx, opts, query.client_id); if (!client) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL(ctx, "invalid_client", "client_id is required"), ); } if (client.disabled) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL(ctx, "client_disabled", "client is disabled"), ); } @@ -227,7 +233,8 @@ export async function authorizeEndpoint( (url) => url === query.redirect_uri, ); if (!redirectUri || !query.redirect_uri) { - throw ctx.redirect( + return handleRedirect( + ctx, getErrorURL(ctx, "invalid_redirect", "invalid redirect uri"), ); } @@ -240,7 +247,8 @@ export async function authorizeEndpoint( return !validScopes?.has(scope); }); if (invalidScopes.length) { - throw ctx.redirect( + return handleRedirect( + ctx, formatErrorURL( query.redirect_uri, "invalid_scope", @@ -263,7 +271,8 @@ export async function authorizeEndpoint( // Validate PKCE parameters if required if (pkceRequired) { if (!query.code_challenge || !query.code_challenge_method) { - throw ctx.redirect( + return handleRedirect( + ctx, formatErrorURL( query.redirect_uri, "invalid_request", @@ -279,7 +288,8 @@ export async function authorizeEndpoint( if (query.code_challenge || query.code_challenge_method) { // Both parameters must be provided together if (!query.code_challenge || !query.code_challenge_method) { - throw ctx.redirect( + return handleRedirect( + ctx, formatErrorURL( query.redirect_uri, "invalid_request", @@ -293,7 +303,8 @@ export async function authorizeEndpoint( // Check code challenge method is supported (only S256) const codeChallengesSupported = ["S256"]; if (!codeChallengesSupported.includes(query.code_challenge_method)) { - throw ctx.redirect( + return handleRedirect( + ctx, formatErrorURL( query.redirect_uri, "invalid_request", diff --git a/packages/oauth-provider/src/continue.ts b/packages/oauth-provider/src/continue.ts index e221363c5e..c6c2e7b9c0 100644 --- a/packages/oauth-provider/src/continue.ts +++ b/packages/oauth-provider/src/continue.ts @@ -57,6 +57,7 @@ async function created( }); } const query = new URLSearchParams(_query); + ctx.headers?.set("accept", "application/json"); ctx.query = deleteFromPrompt(query, "create"); const { url } = await authorizeEndpoint(ctx, opts); return { diff --git a/packages/oauth-provider/src/oauth.test.ts b/packages/oauth-provider/src/oauth.test.ts index f600fa4b25..1da9dee590 100644 --- a/packages/oauth-provider/src/oauth.test.ts +++ b/packages/oauth-provider/src/oauth.test.ts @@ -30,6 +30,19 @@ import { oauthProviderResourceClient } from "./client-resource"; import { oauthProvider } from "./oauth"; import type { OAuthClient } from "./types/oauth"; +function isRedirectResult( + value: unknown, +): value is { redirect: boolean; url: string } { + return ( + typeof value === "object" && + value !== null && + "redirect" in value && + typeof value.redirect === "boolean" && + "url" in value && + typeof value.url === "string" + ); +} + describe("oauth - init", () => { it("should fail without the jwt plugin", async () => { await expect( @@ -304,22 +317,20 @@ describe("oauth", async () => { vi.unstubAllGlobals(); }); - let signInEmailRedirectUri = ""; - await authClient.signIn.email( + const signInResponse = await authClient.signIn.email( { email: testUser.email, password: testUser.password, }, { - onResponse(ctx) { - signInEmailRedirectUri = ctx.response.headers.get("Location") || ""; - }, + throw: true, }, ); - expect(signInEmailRedirectUri).toContain(rpBaseUrl); + expect(signInResponse.redirect).toBe(true); + expect(signInResponse.url).toContain(rpBaseUrl); let callbackUrl = ""; - await client.$fetch(signInEmailRedirectUri!, { + await client.$fetch(signInResponse.url, { method: "GET", headers, onError(context) { @@ -401,6 +412,410 @@ describe("oauth", async () => { expect(signInResponse.url).not.toContain(`${authServerBaseUrl}/login`); }); + /** + * @see https://github.com/better-auth/better-auth/issues/7041 + */ + it("should return JSON redirect after sign-in without Sec-Fetch-Mode header", async ({ + onTestFinished, + }) => { + if (!oauthClient?.client_id || !oauthClient?.client_secret) { + throw Error("beforeAll not run properly"); + } + + const { customFetchImpl: customFetchImplRP } = await createTestInstance(); + + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: rpBaseUrl, + fetchOptions: { + customFetchImpl: customFetchImplRP, + }, + }); + const headers = new Headers(); + const data = await client.signIn.oauth2( + { + providerId, + callbackURL: "/success", + }, + { + throw: true, + onSuccess: cookieSetter(headers), + }, + ); + + let loginRedirectUri = ""; + await authClient.$fetch(data.url, { + method: "GET", + onError(ctx) { + loginRedirectUri = ctx.response.headers.get("Location") || ""; + }, + }); + expect(loginRedirectUri).toContain("/login"); + + vi.stubGlobal("window", { + location: { + href: "", + search: new URL(loginRedirectUri, authServerBaseUrl).search, + }, + }); + onTestFinished(() => { + vi.unstubAllGlobals(); + }); + + // Sign in WITHOUT Sec-Fetch-Mode: cors header. + // The after-hook must still return JSON (not a 302 redirect) + // to avoid CORS errors when fetch follows cross-origin redirects. + let signInLocationHeader = ""; + const signInResponse = await authClient.signIn.email( + { + email: testUser.email, + password: testUser.password, + }, + { + throw: true, + onResponse(ctx) { + signInLocationHeader = ctx.response.headers.get("Location") || ""; + }, + }, + ); + + expect(signInLocationHeader).toBe(""); + expect(signInResponse.redirect).toBe(true); + expect(signInResponse.url).toContain( + `${rpBaseUrl}/api/auth/oauth2/callback/${providerId}`, + ); + }); + + /** + * @see https://github.com/better-auth/better-auth/issues/7041 + */ + it("should still return 302 redirect for navigate-mode requests (form submissions)", async ({ + onTestFinished, + }) => { + if (!oauthClient?.client_id || !oauthClient?.client_secret) { + throw Error("beforeAll not run properly"); + } + + const { customFetchImpl: customFetchImplRP } = await createTestInstance(); + + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: rpBaseUrl, + fetchOptions: { + customFetchImpl: customFetchImplRP, + }, + }); + const headers = new Headers(); + const data = await client.signIn.oauth2( + { + providerId, + callbackURL: "/success", + }, + { + throw: true, + onSuccess: cookieSetter(headers), + }, + ); + + let loginRedirectUri = ""; + await authClient.$fetch(data.url, { + method: "GET", + onError(ctx) { + loginRedirectUri = ctx.response.headers.get("Location") || ""; + }, + }); + expect(loginRedirectUri).toContain("/login"); + + vi.stubGlobal("window", { + location: { + href: "", + search: new URL(loginRedirectUri, authServerBaseUrl).search, + }, + }); + onTestFinished(() => { + vi.unstubAllGlobals(); + }); + + // Sign in with Sec-Fetch-Mode: navigate (browser form submission). + // The after-hook must NOT override Accept, so handleRedirect + // throws a 302 that the browser follows naturally. + let signInLocationHeader = ""; + await authClient.signIn.email( + { + email: testUser.email, + password: testUser.password, + }, + { + headers: { + "Sec-Fetch-Mode": "navigate", + }, + onResponse(ctx) { + signInLocationHeader = ctx.response.headers.get("Location") || ""; + }, + }, + ); + + expect(signInLocationHeader).toContain(rpBaseUrl); + }); + + /** + * @see https://github.com/better-auth/better-auth/issues/7041 + */ + it("should still return 302 redirect for html accept requests without Sec-Fetch-Mode", async ({ + onTestFinished, + }) => { + if (!oauthClient?.client_id || !oauthClient?.client_secret) { + throw Error("beforeAll not run properly"); + } + + const { customFetchImpl: customFetchImplRP } = await createTestInstance(); + + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: rpBaseUrl, + fetchOptions: { + customFetchImpl: customFetchImplRP, + }, + }); + const headers = new Headers(); + const data = await client.signIn.oauth2( + { + providerId, + callbackURL: "/success", + }, + { + throw: true, + onSuccess: cookieSetter(headers), + }, + ); + + let loginRedirectUri = ""; + await authClient.$fetch(data.url, { + method: "GET", + onError(ctx) { + loginRedirectUri = ctx.response.headers.get("Location") || ""; + }, + }); + expect(loginRedirectUri).toContain("/login"); + + vi.stubGlobal("window", { + location: { + href: "", + search: new URL(loginRedirectUri, authServerBaseUrl).search, + }, + }); + onTestFinished(() => { + vi.unstubAllGlobals(); + }); + + let signInLocationHeader = ""; + await authClient.signIn.email( + { + email: testUser.email, + password: testUser.password, + }, + { + headers: { + accept: "text/html,application/xhtml+xml", + }, + onResponse(ctx) { + signInLocationHeader = ctx.response.headers.get("Location") || ""; + }, + }, + ); + + expect(signInLocationHeader).toContain(rpBaseUrl); + }); + + /** + * @see https://github.com/better-auth/better-auth/issues/7041 + */ + it("should return JSON error redirect when client is deleted during OAuth flow", async ({ + onTestFinished, + }) => { + // Create a separate client that we'll delete mid-flow + const { headers: adminHeaders } = await signInWithTestUser(); + const tempClient = await authorizationServer.api.adminCreateOAuthClient({ + headers: adminHeaders, + body: { + redirect_uris: [redirectUri], + skip_consent: true, + }, + }); + expect(tempClient?.client_id).toBeDefined(); + + const { customFetchImpl: customFetchImplRP } = await getTestInstance({ + account: { + accountLinking: { trustedProviders: [providerId] }, + }, + plugins: [ + genericOAuth({ + config: [ + { + scopes: ["openid", "profile", "email"], + providerId, + redirectURI: redirectUri, + authorizationUrl: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + tokenUrl: `${authServerBaseUrl}/api/auth/oauth2/token`, + userInfoUrl: `${authServerBaseUrl}/api/auth/oauth2/userinfo`, + clientId: tempClient!.client_id, + clientSecret: tempClient!.client_secret!, + pkce: true, + }, + ], + }), + ], + }); + + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: rpBaseUrl, + fetchOptions: { customFetchImpl: customFetchImplRP }, + }); + const rpHeaders = new Headers(); + const data = await client.signIn.oauth2( + { providerId, callbackURL: "/success" }, + { throw: true, onSuccess: cookieSetter(rpHeaders) }, + ); + + let loginRedirectUri = ""; + await authClient.$fetch(data.url, { + method: "GET", + onError(ctx) { + loginRedirectUri = ctx.response.headers.get("Location") || ""; + }, + }); + expect(loginRedirectUri).toContain("/login"); + + vi.stubGlobal("window", { + location: { + href: "", + search: new URL(loginRedirectUri, authServerBaseUrl).search, + }, + }); + onTestFinished(() => { + vi.unstubAllGlobals(); + }); + + // Delete the client AFTER the authorize redirect but BEFORE sign-in + await authorizationServer.api.deleteOAuthClient({ + headers: adminHeaders, + body: { client_id: tempClient!.client_id }, + }); + + // Sign in — the after hook calls authorizeEndpoint which finds the + // client is gone. This must return a JSON error redirect, not a 302. + const signInResponse = await authClient.signIn.email( + { + email: testUser.email, + password: testUser.password, + }, + { + throw: true, + }, + ); + + expect(signInResponse.redirect).toBe(true); + expect(signInResponse.url).toContain("error=invalid_client"); + }); + + /** + * @see https://github.com/better-auth/better-auth/issues/7041 + */ + it("should return JSON error redirect when client is disabled during OAuth flow", async ({ + onTestFinished, + }) => { + const { headers: adminHeaders } = await signInWithTestUser(); + const tempClient = await authorizationServer.api.adminCreateOAuthClient({ + headers: adminHeaders, + body: { + redirect_uris: [redirectUri], + skip_consent: true, + }, + }); + expect(tempClient?.client_id).toBeDefined(); + + const { customFetchImpl: customFetchImplRP } = await getTestInstance({ + account: { + accountLinking: { trustedProviders: [providerId] }, + }, + plugins: [ + genericOAuth({ + config: [ + { + scopes: ["openid", "profile", "email"], + providerId, + redirectURI: redirectUri, + authorizationUrl: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + tokenUrl: `${authServerBaseUrl}/api/auth/oauth2/token`, + userInfoUrl: `${authServerBaseUrl}/api/auth/oauth2/userinfo`, + clientId: tempClient!.client_id, + clientSecret: tempClient!.client_secret!, + pkce: true, + }, + ], + }), + ], + }); + + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: rpBaseUrl, + fetchOptions: { customFetchImpl: customFetchImplRP }, + }); + const rpHeaders = new Headers(); + const data = await client.signIn.oauth2( + { providerId, callbackURL: "/success" }, + { throw: true, onSuccess: cookieSetter(rpHeaders) }, + ); + + let loginRedirectUri = ""; + await authClient.$fetch(data.url, { + method: "GET", + onError(ctx) { + loginRedirectUri = ctx.response.headers.get("Location") || ""; + }, + }); + expect(loginRedirectUri).toContain("/login"); + + vi.stubGlobal("window", { + location: { + href: "", + search: new URL(loginRedirectUri, authServerBaseUrl).search, + }, + }); + onTestFinished(() => { + vi.unstubAllGlobals(); + }); + + const context = await authorizationServer.$context; + await context.adapter.update({ + model: "oauthClient", + where: [ + { + field: "clientId", + value: tempClient!.client_id, + }, + ], + update: { + disabled: true, + }, + }); + + const signInResponse = await authClient.signIn.email( + { + email: testUser.email, + password: testUser.password, + }, + { + throw: true, + }, + ); + + expect(signInResponse.redirect).toBe(true); + expect(signInResponse.url).toContain("error=client_disabled"); + }); + it("should sign in using generic oauth discovery", async ({ onTestFinished, }) => { @@ -458,22 +873,20 @@ describe("oauth", async () => { vi.unstubAllGlobals(); }); - let signInEmailRedirectUri = ""; - await authClient.signIn.email( + const signInResponse = await authClient.signIn.email( { email: testUser.email, password: testUser.password, }, { - onResponse(ctx) { - signInEmailRedirectUri = ctx.response.headers.get("Location") || ""; - }, + throw: true, }, ); - expect(signInEmailRedirectUri).toContain(rpBaseUrl); + expect(signInResponse.redirect).toBe(true); + expect(signInResponse.url).toContain(rpBaseUrl); let callbackURL = ""; - await client.$fetch(signInEmailRedirectUri!, { + await client.$fetch(signInResponse.url, { method: "GET", headers, onError(ctx) { @@ -823,6 +1236,106 @@ describe("oauth - prompt", async () => { isUserRegistered = true; }); + it("create - should return JSON redirect when continuing from setup", async ({ + onTestFinished, + }) => { + isUserRegistered = false; + onTestFinished(() => { + isUserRegistered = true; + vi.unstubAllGlobals(); + }); + + const tempClient = await authorizationServer.api.adminCreateOAuthClient({ + headers, + body: { + redirect_uris: [redirectUri], + }, + }); + if (!tempClient?.client_id || !tempClient.client_secret) { + expect.unreachable(); + } + + const { customFetchImpl: customFetchImplRP, cookieSetter } = + await getTestInstance({ + account: { + accountLinking: { trustedProviders: [providerId] }, + }, + plugins: [ + genericOAuth({ + config: [ + { + scopes: ["openid", "profile", "email"], + providerId, + redirectURI: redirectUri, + authorizationUrl: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + tokenUrl: `${authServerBaseUrl}/api/auth/oauth2/token`, + userInfoUrl: `${authServerBaseUrl}/api/auth/oauth2/userinfo`, + clientId: tempClient.client_id, + clientSecret: tempClient.client_secret, + pkce: true, + }, + ], + }), + ], + }); + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: rpBaseUrl, + fetchOptions: { + customFetchImpl: customFetchImplRP, + }, + }); + + const oauthHeaders = new Headers(); + const data = await client.signIn.oauth2( + { + providerId, + callbackURL: "/success", + }, + { + throw: true, + onSuccess: cookieSetter(oauthHeaders), + }, + ); + + let setupRedirectUri = ""; + await serverClient.$fetch(data.url, { + method: "GET", + headers, + onError(context) { + setupRedirectUri = context.response.headers.get("Location") || ""; + }, + }); + expect(setupRedirectUri).toContain("/setup"); + + vi.stubGlobal("window", { + location: { + search: new URL(setupRedirectUri, authServerBaseUrl).search, + }, + }); + isUserRegistered = true; + + let continueLocationHeader = ""; + const continueRes = await serverClient.oauth2.continue( + { + created: true, + }, + { + headers, + throw: true, + onResponse(context) { + continueLocationHeader = + context.response.headers.get("Location") || ""; + }, + }, + ); + + expect(continueLocationHeader).toBe(""); + expect(continueRes.redirect).toBe(true); + expect(continueRes.url).toContain("/consent"); + expect(continueRes.url).toContain(`client_id=${tempClient.client_id}`); + }); + it("consent - should sign in", async ({ onTestFinished }) => { if (!oauthClient?.client_id || !oauthClient?.client_secret) { throw Error("beforeAll not run properly"); @@ -1374,20 +1887,18 @@ describe("oauth - prompt", async () => { }); // Check for redirection to /consent after login - let signInEmailRedirectUri = ""; - await serverClient.signIn.email( + const signInResponse = await serverClient.signIn.email( { email: testUser.email, password: testUser.password, }, { - onResponse(ctx) { - signInEmailRedirectUri = ctx.response.headers.get("Location") || ""; - }, + throw: true, }, ); - expect(signInEmailRedirectUri).toContain("/consent"); - expect(signInEmailRedirectUri).toContain("prompt=consent"); + expect(signInResponse.redirect).toBe(true); + expect(signInResponse.url).toContain("/consent"); + expect(signInResponse.url).toContain("prompt=consent"); }); it("select_account+consent - should always redirect to select_account and force consent (notice consent previously given)", async ({ @@ -1612,21 +2123,24 @@ describe("oauth - prompt", async () => { vi.unstubAllGlobals(); }); - let consentRedirectUri = ""; // Select Account and continue auth flow - await serverClient.organization.setActive( + const setActiveResponse = await serverClient.organization.setActive( { organizationId: org.id, organizationSlug: org.slug, }, { headers, - onResponse(context) { - consentRedirectUri = context.response.headers.get("Location") || ""; - cookieSetter(headers)(context); - }, + throw: true, + onSuccess: cookieSetter(headers), }, ); + expect(isRedirectResult(setActiveResponse)).toBe(true); + if (!isRedirectResult(setActiveResponse)) { + expect.unreachable(); + } + expect(setActiveResponse.redirect).toBe(true); + const consentRedirectUri = setActiveResponse.url; expect(consentRedirectUri).toContain(`/consent`); expect(consentRedirectUri).toContain(`client_id=${oauthClient.client_id}`); expect(consentRedirectUri).toContain(`scope=`); diff --git a/packages/oauth-provider/src/oauth.ts b/packages/oauth-provider/src/oauth.ts index c056a393a0..b18d9459c6 100644 --- a/packages/oauth-provider/src/oauth.ts +++ b/packages/oauth-provider/src/oauth.ts @@ -283,6 +283,19 @@ export const oauthProvider = >(options: O) => { if (!session) return; ctx.context.session = session; + const secFetchMode = ctx.request?.headers + ?.get("sec-fetch-mode") + ?.toLowerCase(); + const acceptHeader = + ctx.request?.headers?.get("accept")?.toLowerCase() ?? ""; + const isNavigationRequest = + secFetchMode === "navigate" || + (!secFetchMode && + (acceptHeader.includes("text/html") || + acceptHeader.includes("application/xhtml+xml"))); + if (!isNavigationRequest) { + ctx.headers?.set("accept", "application/json"); + } ctx.query = deleteFromPrompt(query, "login"); return await authorizeEndpoint(ctx, opts); }),