fix: handle Microsoft multi-tenant issuer mismatch#305
Conversation
When using MicrosoftEntraId with the "common" (multi-tenant) endpoint,
OIDC discovery returns issuer `.../common/v2.0` but ID tokens carry
tenant-specific `iss` claims (`.../{tenantId}/v2.0`). This causes
oauth4webapi's issuer validation to reject the token.
Use oauth4webapi's `_expectedIssuer` extension point to accept the
token's own `iss` claim for microsoft-entra-id providers, matching
the approach used by @auth/core (Auth.js).
Fixes get-convex#304
|
@moshemarciano is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces an Changes
Sequence Diagram(s)sequenceDiagram
participant AppServer as App Server
participant Discovery as OIDC Discovery
participant AuthServer as Microsoft Auth Server
participant OAuthLib as oauth4webapi
AppServer->>Discovery: fetch .well-known/openid-configuration
Discovery-->>AppServer: metadata (issuer = .../common/v2.0)
AppServer->>AppServer: applyMultiTenantEntraFix -> set _expectedIssuer = (result) => result.claims.iss
AppServer->>AuthServer: redirect user / authorization code flow
AuthServer-->>AppServer: token response (id_token with tenant-specific iss)
AppServer->>OAuthLib: processAuthorizationCodeResponse (validate token)
OAuthLib-->>AppServer: accepts token using result.claims.iss as expected issuer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/oauth/convexAuth.ts`:
- Around line 120-124: The override of the oauth4webapi internal _expectedIssuer
must only be applied for Microsoft Entra multi-tenant aliases, not all
microsoft-entra-id configs; change the condition around the _expectedIssuer
override so it additionally checks that the config represents a multi-tenant
alias (e.g., config.tenant is one of "common", "organizations", or "consumers",
or config.issuer/authority contains "/common/", "/organizations/" or
"/consumers/"), and only then set (discoveredAs as any)[expectedIssuer] =
(result:any) => result.claims.iss; keep the existing symbol access
(_expectedIssuer) and discoveredAs usage but scope it via this extra
multi-tenant check (or a small helper isMultiTenantEntra(config)) so
tenant-specific endpoints are not altered.
🪄 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: 5baccec3-223d-450b-ba54-c527410982a6
📒 Files selected for processing (1)
src/server/oauth/convexAuth.ts
Only apply _expectedIssuer override when the discovered issuer is a multi-tenant alias (common, organizations, consumers). Tenant-specific endpoints return exact issuer matches and don't need this workaround.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/oauth/convexAuth.ts`:
- Around line 123-127: There is a duplicate declaration of issuer which causes a
compile error; remove the second `const issuer = ...` and instead assign to the
existing `issuer` variable (e.g., `issuer = discoveredAs.issuer ?? ""`) or
rename the new variable if a separate value is needed, then keep the
`isMultiTenantEntra` logic using `issuer` (references: discoveredAs,
isMultiTenantEntra, config.type, config.id).
🪄 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: 0a60a38a-075b-4b16-9c3e-985004fbc0a3
📒 Files selected for processing (1)
src/server/oauth/convexAuth.ts
The function already declares `const issuer = new URL(config.issuer)` at the top. Rename the discovered issuer variable to `discoveredIssuer` to avoid a TS2451 duplicate declaration error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/oauth/convexAuth.ts`:
- Around line 123-133: The multi-tenant Microsoft Entra issuer override is only
applied to discoveredAs but not to the explicit-endpoints branch, so when
callers pass explicit endpoints the constructed as lacks the _expectedIssuer
override and fails validation in o.processAuthorizationCodeResponse; extract the
override logic that computes expectedIssuer = (o as any)._expectedIssuer and
sets (target as any)[expectedIssuer] = (result:any) => result.claims.iss into a
small helper function (e.g., applyMultiTenantIssuerOverride(target, o, config))
and call it both where discoveredAs is mutated and where the explicit-endpoints
`as` object is constructed so both paths accept tenant-specific iss claims.
🪄 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: ce40a582-8265-4cc7-8296-2a4984dafc50
📒 Files selected for processing (1)
src/server/oauth/convexAuth.ts
Extract the _expectedIssuer override into a shared helper and call it in both the OIDC discovery branch and the explicit-endpoints branch. Previously only the discovery path was covered — callers providing all three endpoints explicitly with a multi-tenant issuer would still fail.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/server/oauth/convexAuth.ts (1)
101-101: Simplify redundant issuer access.The expression
(as as any).issuer ?? as.issueris redundant—both access the sameissuerproperty. The cast toanydoesn't change the property being accessed.Suggested simplification
- const issuer = (as as any).issuer ?? as.issuer ?? ""; + const issuer = as.issuer ?? "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/oauth/convexAuth.ts` at line 101, The assignment to issuer uses a redundant cast: replace the expression "const issuer = (as as any).issuer ?? as.issuer ?? "";" with a single access that reads the issuer once (e.g., use as.issuer ?? "") to remove the unnecessary (as as any) cast and simplify the code; update the line where issuer is declared in convexAuth.ts (the variable name issuer and the object as) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/oauth/convexAuth.ts`:
- Around line 107-110: The code currently reads the internal symbol
_expectedIssuer from oauth4webapi (const expectedIssuer = (o as
any)._expectedIssuer) and assigns a function to (as as any)[expectedIssuer] to
pull result.claims.iss; replace this fragile internal-symbol approach by using
oauth4webapi's documented processDiscoveryResponse API: call
processDiscoveryResponse with the expected issuer identifier and the discovery
response (instead of touching _expectedIssuer) and use the returned/validated
issuer value for issuer validation and any assignment previously done via (as as
any)[expectedIssuer]; update references involving o, as, expectedIssuer, and
result.claims.iss to use the processDiscoveryResponse call and its output for
issuer validation and assignment.
---
Nitpick comments:
In `@src/server/oauth/convexAuth.ts`:
- Line 101: The assignment to issuer uses a redundant cast: replace the
expression "const issuer = (as as any).issuer ?? as.issuer ?? "";" with a single
access that reads the issuer once (e.g., use as.issuer ?? "") to remove the
unnecessary (as as any) cast and simplify the code; update the line where issuer
is declared in convexAuth.ts (the variable name issuer and the object as)
accordingly.
🪄 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: 0ce52b15-76c8-4cb6-ba0b-c832ef8e927a
📒 Files selected for processing (1)
src/server/oauth/convexAuth.ts
| // _expectedIssuer is a runtime-exported Symbol from oauth4webapi, | ||
| // not yet in the type declarations. | ||
| const expectedIssuer = (o as any)._expectedIssuer as symbol; | ||
| (as as any)[expectedIssuer] = (result: any) => result.claims.iss; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does oauth4webapi version 3.1.2 export _expectedIssuer symbol for custom issuer validation?
💡 Result:
No, oauth4webapi version 3.1.2 does not export a symbol named _expectedIssuer for custom issuer validation. The package exports tree-shakeable ESM with documented public APIs such as processDiscoveryResponse(expectedIssuerIdentifier: URL, response: Response), which handles issuer validation using the provided expectedIssuerIdentifier parameter passed by the caller. No sources mention an exported _expectedIssuer symbol; the underscore prefix suggests it would be private if it exists internally. Official docs, API reference, changelog for 3.1.2 (refactor: shake supported functions, types update), and examples confirm custom validation is done via public functions like processDiscoveryResponse, not a special exported symbol.
Citations:
- 1: https://github.com/panva/oauth4webapi
- 2: https://www.npmjs.com/package/oauth4webapi
- 3: https://jsr.io/@panva/oauth4webapi/doc
- 4: https://github.com/panva/oauth4webapi/blob/main/docs/functions/processDiscoveryResponse.md
- 5: chore(deps): bump oauth4webapi from 3.1.1 to 3.1.3 nextauthjs/next-auth#12261
- 6: https://github.com/panva/oauth4webapi/blob/main/docs/README.md
- 7: Allow custom discovery location? panva/oauth4webapi#63
Replace reliance on undocumented internal _expectedIssuer symbol with oauth4webapi's public API.
The code accesses _expectedIssuer from oauth4webapi 3.1.2, which is not documented or exported as part of the library's public API. The underscore prefix and absence of any mention in oauth4webapi's official documentation or type declarations confirms this is internal implementation detail. oauth4webapi instead provides documented public APIs like processDiscoveryResponse(expectedIssuerIdentifier, response) for handling issuer validation. Using undocumented internal symbols creates a fragile dependency that can silently break with any library update, and should be replaced with the documented public functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/oauth/convexAuth.ts` around lines 107 - 110, The code currently
reads the internal symbol _expectedIssuer from oauth4webapi (const
expectedIssuer = (o as any)._expectedIssuer) and assigns a function to (as as
any)[expectedIssuer] to pull result.claims.iss; replace this fragile
internal-symbol approach by using oauth4webapi's documented
processDiscoveryResponse API: call processDiscoveryResponse with the expected
issuer identifier and the discovery response (instead of touching
_expectedIssuer) and use the returned/validated issuer value for issuer
validation and any assignment previously done via (as as any)[expectedIssuer];
update references involving o, as, expectedIssuer, and result.claims.iss to use
the processDiscoveryResponse call and its output for issuer validation and
assignment.
Explain why _expectedIssuer is the correct approach: it's a runtime-exported extension point from oauth4webapi, not a private internal — and it's the same method @auth/core uses for Microsoft multi-tenant issuer validation.
|
We hit the same issue while integrating Microsoft Entra ID with Convex Auth for multi-tenant sign-in ( After Microsoft consent, the callback failed with:
We applied this patch locally via Thanks for putting this together — would love to see it land in a release. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
Problem
When using
MicrosoftEntraIdwith the default multi-tenant ("common") endpoint, the OAuth callback fails. OIDC discovery returnsissuer: .../common/v2.0, but ID tokens carry tenant-specificissclaims (.../{tenantId}/v2.0).oauth4webapi's issuer validation rejects these tokens.Fix
Use
oauth4webapi's exported_expectedIssuerSymbol extension point on the discoveredAuthorizationServerformicrosoft-entra-idproviders. This accepts the token's ownissclaim while still validating the JWT signature — the same approach@auth/core(Auth.js) uses internally for Microsoft.The fix only applies in the discovery code path (when
authorization,token, oruserinfoendpoints are not all explicitly provided), which is the common configuration for Microsoft Entra ID.Testing
tenantId: "...") continues to work — discovery returns the tenant-specific issuer which matches tokensconfig.id === "microsoft-entra-id"Closes #304
Summary by CodeRabbit
New Features
Bug Fixes