Re-run OIDC discovery after token exchange for Microsoft Entra ID#335
Re-run OIDC discovery after token exchange for Microsoft Entra ID#335guustgoossens wants to merge 2 commits into
Conversation
@convex-dev/auth rejected every Microsoft Entra ID sign-in whenever the Entra app was registered against a multi-tenant authority (/common, /organizations, /consumers). The callback failed inside processAuthorizationCodeResponse with an "unexpected JWT iss claim value" error because the authorization-server metadata still claimed .../common/v2.0 as the issuer while the id_token was signed with the per-tenant issuer (.../<real-tenant-guid>/v2.0). Port the post-token-exchange re-discovery that @auth/core's handleOAuth runs for providers carrying the conformInternal symbol (packages/core/src/lib/actions/callback/oauth/callback.ts). After the token exchange, decode the id_token, derive the per-tenant authority from its tid claim, re-fetch the discovery document for that authority and replace the AuthorizationServer metadata before processAuthorizationCodeResponse runs. Because @auth/core's conformInternal symbol is marked @internal and is not listed in the package's exports map, importing it from @auth/core/lib/symbols.js breaks under bundlers that enforce package exports (Vite/vitest). Detect the symbol structurally by description on the provider object instead, mirroring upstream's gate without a fragile deep import. The re-discovery uses an inline customFetch wrapper that rewrites the {tenantid} placeholder in the discovery document's issuer field with the real tid. We deliberately bypass provider[customFetch] here because the upstream Microsoft Entra ID provider's hook captures config.issuer at construction time (/common/v2.0) and would rewrite {tenantid} to "common", causing the same mismatch this PR fixes. Doing the rewrite locally makes the fix work against the @auth/core version currently pinned and independent of any upstream release. Add a test/convex/microsoftEntra.test.ts suite covering the re-discovery happy path and a spoofed-issuer rejection, and register the Microsoft Entra ID provider in test/convex/auth.ts with a no-op profile callback so tests don't hit graph.microsoft.com.
|
@guustgoossens is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: get-convex/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements Microsoft Entra ID multi-tenant support by adding tenant-specific issuer re-discovery to the OAuth callback handler. After token exchange, the callback detects providers with an internal conformance marker, decodes the id_token to extract the tenant ID ( Sequence DiagramsequenceDiagram
participant Client
participant OAuth Callback Handler
participant Token Service
participant Discovery Service
participant JWKS Service
Client->>OAuth Callback Handler: POST callback with code
OAuth Callback Handler->>Token Service: exchange code for id_token
Token Service-->>OAuth Callback Handler: id_token response
OAuth Callback Handler->>OAuth Callback Handler: decode id_token to extract tid
OAuth Callback Handler->>Discovery Service: fetch discovery for tenant authority
Discovery Service-->>OAuth Callback Handler: discovery metadata
OAuth Callback Handler->>OAuth Callback Handler: rewrite issuer with tid
OAuth Callback Handler->>OAuth Callback Handler: re-process discovery
OAuth Callback Handler->>JWKS Service: fetch JWKS with tenant issuer
JWKS Service-->>OAuth Callback Handler: JWKS
OAuth Callback Handler-->>Client: redirect with code or error
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/convex/microsoftEntra.test.ts (1)
288-347: ⚡ Quick winAdd a single-tenant authority regression test.
Current coverage is centered on
/common. Add one case where initial discovery is already/{tenant-guid}/v2.0and callback still succeeds; this guards against tenant-segment replacement regressions in re-discovery logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/convex/microsoftEntra.test.ts` around lines 288 - 347, Add a new test in the "microsoft entra id multi-tenant issuer re-discovery" suite that covers the single-tenant authority flow: call setupEnv(), create t via convexTest(schema) and ctx via driveSignInUpTo(t), then invoke driveCallback(t, ctx, ...) with idTokenIssuerTenantId and idTokenTidClaim both set to TENANT_ID but ensure the initial discovery URL used by the mocked fetch is already the tenant-specific authority (i.e., `/${TENANT_ID}/v2.0/.well-known/openid-configuration`); assert the fetchMock requestedUrls contains that tenant-specific discovery URL and that the returned callbackResponse.status is 302 and the Location header contains a non-null code query param (similar assertions to the existing first test but with initial discovery set to the tenant GUID instead of /common).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/oauth/callback.ts`:
- Around line 223-226: The regex and replace logic for extracting and swapping
the tenant id can break GUIDs with hyphens and corrupt the issuer; update
tenantRe to match any non-slash segment (e.g.
/microsoftonline\.com\/([^\/]+)\/v2\.0/) and then build the new issuer
deterministically instead of blind string replacement—either use
as.issuer.replace(tenantRe, `microsoftonline.com/${tid}/v2.0`) or parse new
URL(as.issuer) and set its pathname to `/[tid]/v2.0`; adjust references to
tenantRe, tenantId, issuer, as.issuer and tid accordingly.
---
Nitpick comments:
In `@test/convex/microsoftEntra.test.ts`:
- Around line 288-347: Add a new test in the "microsoft entra id multi-tenant
issuer re-discovery" suite that covers the single-tenant authority flow: call
setupEnv(), create t via convexTest(schema) and ctx via driveSignInUpTo(t), then
invoke driveCallback(t, ctx, ...) with idTokenIssuerTenantId and idTokenTidClaim
both set to TENANT_ID but ensure the initial discovery URL used by the mocked
fetch is already the tenant-specific authority (i.e.,
`/${TENANT_ID}/v2.0/.well-known/openid-configuration`); assert the fetchMock
requestedUrls contains that tenant-specific discovery URL and that the returned
callbackResponse.status is 302 and the Location header contains a non-null code
query param (similar assertions to the existing first test but with initial
discovery set to the tenant GUID instead of /common).
🪄 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: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed3bf8fc-805a-44e6-b597-35385af59a99
📒 Files selected for processing (3)
src/server/oauth/callback.tstest/convex/auth.tstest/convex/microsoftEntra.test.ts
The previous re-discovery used `(\w+)` to extract the tenant segment from `as.issuer`, which doesn't match hyphenated tenant GUIDs like `8a2a7b4d-1234-5678-9abc-def012345678` — so when a caller configured the provider with a single-tenant authority, the match silently fell back to `"common"` and `.replace(tenantId, tid)` was a no-op. Switch to `[^/]+` so any URL path segment matches (authority names and GUIDs alike), and rewrite via the segment-anchored regex instead of a plain substring replace so we can't accidentally clobber an unrelated portion of the issuer URL. Matches the upstream fix in nextauthjs/next-auth#13440.
Fix Microsoft Entra ID multi-tenant issuer validation
Summary
@convex-dev/authrejects every Microsoft Entra ID sign-in whenever the Entraapp is registered against a multi-tenant authority (
/common,/organizations, or/consumers). The callback fails insideprocessAuthorizationCodeResponsewith anunexpected JWT "iss" (issuer) claim valueerror, because the authorization-server metadata still claimshttps://login.microsoftonline.com/common/v2.0as the issuer while theid_tokenMicrosoft returns is signed with the per-tenant issuer (e.g.https://login.microsoftonline.com/<real-tenant-guid>/v2.0).This PR ports the per-tenant re-discovery step that
@auth/coreruns afterthe token exchange (its
conformInternalblock inpackages/core/src/lib/actions/callback/oauth/callback.ts) into convex-auth'sown
handleOAuth, so the bug is fixed in convex-auth's callback flowindependently of next-auth's release schedule.
Root cause
Microsoft's discovery endpoint at
https://login.microsoftonline.com/{authority}/v2.0/.well-known/openid-configurationreturns the literal string
{tenantid}in theissuerfield for the sharedmulti-tenant authorities (see
MicrosoftDocs/azure-docs#113944).
The
@auth/coreMicrosoft Entra ID provider rewrites that placeholder via acustomFetchhook so that the initial discovery succeeds, but the realper-tenant issuer that ends up inside the signed
id_tokenis only knowableafter the token exchange, when we can read the
tidclaim.@auth/core'shandleOAuthhandles this by re-running OIDC discovery for theper-tenant authority once it has the
tid, before callingprocessAuthorizationCodeResponse.@convex-dev/auth'shandleOAuthwasported from a snapshot of
@auth/corethat pre-dates this block (the commentat the top of the file pins commit
5af1f30a), so it skipped that stepentirely — which is why issuer validation tripped here.
Changes
src/server/oauth/callback.tsas(the authorization server metadata used byoauth4webapi) to bereassigned later in
handleOAuth.conformance (i.e. it set the
@auth/coreinternalconformInternalsymbol),decode the returned
id_token, derive the per-tenant authority fromtid,re-fetch the discovery document for that authority, and replace
asbefore
processAuthorizationCodeResponseruns.@auth/core'sconformInternalsymbol is marked@internalandis not listed in the package's
exportsmap (so importing it from@auth/core/lib/symbols.jsbreaks under bundlers that enforce packageexports — including Vite/vitest), the new
hasConformInternalSymbolhelperdetects the symbol structurally by description (
Symbol("conform-internal"))on the provider object instead of importing it. This keeps the gate
functionally identical to upstream's symbol-based check without taking on
a fragile deep import.
customFetchwrapper that rewrites the{tenantid}placeholder in the discovery document'sissuerfield with thereal
tidfrom the ID token. Bypassingprovider[customFetch]here isdeliberate: the upstream provider's hook captures
config.issueratconstruction time (
/common/v2.0), so it would rewrite{tenantid}tocommonand we'd still hit an issuer mismatch. By doing the rewritelocally with the real
tid, this fix works against the currently shipped@auth/core@0.37.xand does not require waiting for the parallelnextauthjs/next-authfix at fix(providers): Microsoft Entra ID multi-tenant issuer validation nextauthjs/next-auth#13440 to release.test/convex/auth.tsprofilecallback sothe tests don't try to hit
https://graph.microsoft.com/...for the user'sprofile photo.
test/convex/microsoftEntra.test.ts(new)Two tests exercise the new code path through the real Convex HTTP callback
flow, with
fetchstubbed to simulate Microsoft's discovery + token endpointsand a signed
id_tokenproduced from the same RSA key pair already used bythe test suite:
accepts an id_token whose issuer matches the tid claim— drives a fullmulti-tenant sign-in through
/api/auth/signin/microsoft-entra-idand thecallback handler; asserts the re-discovery endpoint for the real tenant
GUID is hit and the callback redirects back to the app with an auth code.
rejects an id_token whose issuer does not match the tid claim—same flow, but signs the
id_tokenwith a different (spoofed) issuer thanthe
tidclaim; asserts the callback does not succeed (oauth4webapi'sissuer validation rejects the token, the catch in
signInredirects withouta
code).The existing GitHub
oauth.test.tssuite continues to pass, exercising thenon-Microsoft path unchanged.
Before / after
Before:
After: multi-tenant sign-ins complete successfully; spoofed-issuer tokens
remain rejected by
processAuthorizationCodeResponse.Related
@auth/corefix that updates the provider'scustomFetchto lookat the request URL before falling back to
config.issuer:fix(providers): Microsoft Entra ID multi-tenant issuer validation nextauthjs/next-auth#13440
conformInternalblock this is ported from:packages/core/src/lib/actions/callback/oauth/callback.ts#L188-L224microsoftonline.com/{tenantid}/v2.0Test plan
cd test && npm run test:once— 14 files / 42 tests pass (including 2new Microsoft Entra ID tests).
npx tsc --project tsconfig.server.json --noEmit— clean.npx eslint src— clean.cd test && npm run lint— clean.oauth.test.tscontinues to pass (non-Microsoftregression coverage).