From 4f3fa24dfe90bf25ffd42fac420a978de48bf3e9 Mon Sep 17 00:00:00 2001 From: Konstantin Wohlwend Date: Wed, 27 May 2026 17:14:39 -0700 Subject: [PATCH] Include the scope parameter in certain Microsoft OAuth operations --- .claude/CLAUDE-KNOWLEDGE.md | 3 +++ AGENTS.md | 1 + .../oauth/callback/[provider_id]/route.tsx | 1 + apps/backend/src/oauth/providers/base.test.ts | 24 +++++++++++++++++- apps/backend/src/oauth/providers/base.tsx | 25 +++++++++++++++++++ .../backend/src/oauth/providers/microsoft.tsx | 5 ++++ 6 files changed, 58 insertions(+), 1 deletion(-) diff --git a/.claude/CLAUDE-KNOWLEDGE.md b/.claude/CLAUDE-KNOWLEDGE.md index d88b86b20..f01d96f85 100644 --- a/.claude/CLAUDE-KNOWLEDGE.md +++ b/.claude/CLAUDE-KNOWLEDGE.md @@ -562,3 +562,6 @@ A: Project config overrides only support the hosted `sourceOfTruth` shape. Legac ## Q: How should managed email onboarding e2e tests wait for mock verification? A: Do not rely on a fixed `wait(1500)` after setup. The mock onboarding path flips the domain to `verified` asynchronously through `runAsynchronously`, so tests should poll the managed-onboarding check endpoint until the expected status appears. + +## Q: How should Microsoft OAuth callback token exchange include scopes? +A: Microsoft Entra ID's v2 token endpoint can reject authorization-code exchanges with `AADSTS70011` if the token request omits `scope`. Keep scope emission opt-in at the provider layer (`includeScopeInCallbackTokenExchange`) and pass the same merged base/provider scopes to `openid-client` via the callback `extras.exchangeBody.scope` parameter. The callback route must forward stored `providerScope` from the outer OAuth info so custom Microsoft provider scopes are included in the token exchange. diff --git a/AGENTS.md b/AGENTS.md index 77626849c..750017f5b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -118,6 +118,7 @@ To see all development ports, refer to the index.html of `apps/dev-launchpad/pub - NEVER EVER return a server error with an internal server error that may contain information that the user shouldn't see. For example, never return the error message on a public API from an upstream provider without properly filtering it first. Most of the time, for internal server errors, you should just use StackAssertionError (which won't pass the message to the user), not StatusError (you almost never want to instantiate a StatusError with status 5xx). - When adding code to the `private` part of the backend, put the actual implementation into `implementation` (if the submodule is checked out), and implement a simple fallback in `implementation-fallback` for self-hosters. `implementation.generated.ts` will automatically be generated, which you can then import from `index.ts`. (See the existing code as an example.) If the submodule isn't checked out, but you need to add code to the `private` part of the backend, let the user know. - Security-sensitive code on the backend that shouldn't be public should be in the `private` part of the backend. +- When you fix some obscure bug, or otherwise make a small change that is the result of a complex thought, add a concise comment explaining the thought in detail. Your mental model should be that we want to keep track of all the tiny decisions that are not clearly visible in the code, such that when/if we rewrite the code in the future, we don't have to re-learn all the tiny decisions that were made iteratively. ### Code-related - Use ES6 maps instead of records wherever you can. diff --git a/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx b/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx index f0ea7d9e6..a5870a0f0 100644 --- a/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx +++ b/apps/backend/src/app/api/latest/auth/oauth/callback/[provider_id]/route.tsx @@ -170,6 +170,7 @@ const handler = createSmartRouteHandler({ callbackResult = await providerObj.getCallback({ codeVerifier: innerCodeVerifier, state: innerState, + extraScope: providerScope, callbackParams: { ...query, ...body, diff --git a/apps/backend/src/oauth/providers/base.test.ts b/apps/backend/src/oauth/providers/base.test.ts index f732f6d46..18cfdd414 100644 --- a/apps/backend/src/oauth/providers/base.test.ts +++ b/apps/backend/src/oauth/providers/base.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { getOAuthAccessTokenRefreshError, getOAuthAccessTokenRefreshErrorDisposition, isRetryableOAuthUserInfoError, resolveOAuthAccessTokenExpiredAt } from "./base"; +import { getOAuthAccessTokenRefreshError, getOAuthAccessTokenRefreshErrorDisposition, getOAuthCallbackExtraParams, isRetryableOAuthUserInfoError, resolveOAuthAccessTokenExpiredAt } from "./base"; describe("isRetryableOAuthUserInfoError", () => { it("returns true for openid-client timeout errors", () => { @@ -102,6 +102,28 @@ describe("getOAuthAccessTokenRefreshError", () => { }); }); +describe("getOAuthCallbackExtraParams", () => { + it("omits token exchange params unless the provider opts in", () => { + expect(getOAuthCallbackExtraParams({ + includeScopeInTokenExchange: false, + baseScope: "openid", + extraScope: "User.Read", + })).toBeUndefined(); + }); + + it("includes merged scopes for providers that require scope on callback exchange", () => { + expect(getOAuthCallbackExtraParams({ + includeScopeInTokenExchange: true, + baseScope: "User.Read openid", + extraScope: "Mail.Read", + })).toEqual({ + exchangeBody: { + scope: "User.Read openid Mail.Read", + }, + }); + }); +}); + describe("resolveOAuthAccessTokenExpiredAt", () => { it("uses finite provider expires_in values", () => { expect(resolveOAuthAccessTokenExpiredAt({ diff --git a/apps/backend/src/oauth/providers/base.tsx b/apps/backend/src/oauth/providers/base.tsx index a75fb67fb..fca7fb8be 100644 --- a/apps/backend/src/oauth/providers/base.tsx +++ b/apps/backend/src/oauth/providers/base.tsx @@ -219,6 +219,22 @@ export function getOAuthAccessTokenRefreshError(error: unknown, options: { type DefaultAccessTokenExpiresInMillis = number | null | ((tokenSet: OIDCTokenSet) => number | null | undefined); +export function getOAuthCallbackExtraParams(options: { + includeScopeInTokenExchange: boolean | undefined, + baseScope: string, + extraScope: string | undefined, +}) { + if (!options.includeScopeInTokenExchange) { + return undefined; + } + + return { + exchangeBody: { + scope: mergeScopeStrings(options.baseScope, options.extraScope ?? ""), + }, + }; +} + function getFiniteNumber(value: unknown): number | undefined { return typeof value === "number" && Number.isFinite(value) ? value : undefined; } @@ -299,6 +315,7 @@ export abstract class OAuthBaseProvider { public readonly noPKCE?: boolean, public readonly openid?: boolean, public readonly alternativeIssuers?: string[], + public readonly includeScopeInCallbackTokenExchange?: boolean, ) {} protected static async createConstructorArgs(options: @@ -312,6 +329,7 @@ export abstract class OAuthBaseProvider { tokenEndpointAuthMethod?: "client_secret_post" | "client_secret_basic", noPKCE?: boolean, alternativeIssuers?: string[], + includeScopeInCallbackTokenExchange?: boolean, } & ( | ({ @@ -360,6 +378,7 @@ export abstract class OAuthBaseProvider { options.noPKCE, options.openid, options.alternativeIssuers, + options.includeScopeInCallbackTokenExchange, ] as const; } @@ -386,6 +405,7 @@ export abstract class OAuthBaseProvider { callbackParams: CallbackParamsType, codeVerifier: string, state: string, + extraScope?: string, }): Promise<{ userInfo: OAuthUserInfo, tokenSet: TokenSet }> { let tokenSet; const callbackParams = { ...options.callbackParams }; @@ -408,6 +428,11 @@ export abstract class OAuthBaseProvider { code_verifier: this.noPKCE ? undefined : options.codeVerifier, state: options.state, }, + getOAuthCallbackExtraParams({ + includeScopeInTokenExchange: this.includeScopeInCallbackTokenExchange, + baseScope: this.scope, + extraScope: options.extraScope, + }), ] as const; try { diff --git a/apps/backend/src/oauth/providers/microsoft.tsx b/apps/backend/src/oauth/providers/microsoft.tsx index e87cd1d82..a127d3b1e 100644 --- a/apps/backend/src/oauth/providers/microsoft.tsx +++ b/apps/backend/src/oauth/providers/microsoft.tsx @@ -26,6 +26,11 @@ export class MicrosoftProvider extends OAuthBaseProvider { baseScope: "User.Read openid", openid: true, jwksUri: `https://login.microsoftonline.com/${tenantId}/discovery/v2.0/keys`, + // Microsoft treats the token-request scope as a resource selector during + // authorization-code redemption. Plain sign-in can work without restating + // it, but connected-account flows with extra provider_scope values have + // returned AADSTS70011 unless we include the originally requested scopes. + includeScopeInCallbackTokenExchange: true, ...rest, })); }