fix(provider): preserve non-standard claims in generic OIDC parser#2504
Open
mastermanas805 wants to merge 1 commit intosupabase:masterfrom
Open
fix(provider): preserve non-standard claims in generic OIDC parser#2504mastermanas805 wants to merge 1 commit intosupabase:masterfrom
mastermanas805 wants to merge 1 commit intosupabase:masterfrom
Conversation
parseGenericIDToken unmarshalled ID tokens into the strongly-typed
Claims struct with no catch-all, silently dropping every claim that
was not declared as a struct field. Zitadel emits role data under
URN-namespaced keys ("urn:zitadel:iam:org:project:roles") which
disappeared before reaching the user's session, the symptom reported
in supabase#2494. The same path also dropped standard OIDC claims like amr,
auth_time, azp, sid, and at_hash for any custom OIDC provider whose
issuer was not one of the named cases.
Mirror the two-pass pattern parseAzureIDToken already uses: parse the
token into the typed Claims struct, then re-parse into a
map[string]interface{}, strip the registered JWT claims and the OIDC
standard claims that already have explicit fields on Claims, and
retain whatever remains in CustomClaims. The kept values flow through
the existing structs.Map serialization into raw_user_meta_data.custom_claims
and from there into the JWT access token, matching the pre-existing
Apple and Azure behaviors.
Adds an in-memory OIDC provider regression test that signs a
Zitadel-shaped ID token (both project-scoped and unscoped URN role
claims) and asserts the URN keys survive in CustomClaims while the
standard claims live only in their typed fields.
Fixes supabase#2494
Signed-off-by: Manas Srivastava <mastermanas805@gmail.com>
Author
|
@fadymak requesting your review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes #2494.
parseGenericIDToken(internal/api/provider/oidc.go) unmarshals the ID token into the strongly-typedClaimsstruct, which has no catch-all field. JSON unmarshal silently drops every key that is not declared as a struct field. For Zitadel, that means the role claims emitted under URN-namespaced keys (urn:zitadel:iam:org:project:roles,urn:zitadel:iam:org:project:<projectId>:roles) never reachdata.Metadata.CustomClaims, never get serialized intoraw_user_meta_data, and never appear in the session returned bysupabase.auth.getSession().The same path also drops standard OIDC claims that custom providers commonly emit (
amr,auth_time,azp,sid,at_hash) for any issuer that does not match one of the named cases inParseIDToken.How
Mirror the two-pass pattern
parseAzureIDTokenalready uses (oidc.go:329-365):Claimsstruct so OIDC standard fields land in their typed homes.map[string]interface{}to capture every key the JWT carries.Claims.data.Metadata.CustomClaims.This is exactly what Azure and Apple parsers do today; the generic path was missing it.
Empirical evidence (Zitadel ID token before vs after)
Reproduced locally with
supabase/authrunning against a fresh Zitadel instance and a custom OIDC provider configured for it.Raw claims Zitadel emits (18 keys):
data.Metadataafter the strongly-typed unmarshal — before this fix:6 standard OIDC claims and both URN role claims dropped.
data.Metadata.CustomClaimsafter this fix:{ "urn:zitadel:iam:org:project:roles": {"admin": {"<orgId>": "<orgDomain>"}}, "urn:zitadel:iam:org:project:<projectId>:roles": {"admin": {"<orgId>": "<orgDomain>"}} }After running the OAuth flow end to end,
auth.users.raw_user_meta_data.custom_claimsnow contains both URN keys, which is whatgetSession()returns to the client.Tests
Adds
internal/api/provider/oidc_zitadel_test.gowithTestParseGenericIDTokenPreservesURNNamespacedClaims. The test:httptest.Serverexposing/.well-known/openid-configurationand/jwkssooidc.NewProvidercan perform real discovery.ParseIDToken, asserts both URN claims are preserved inCustomClaims, and asserts standard claims (iss,sub,email, etc.) do not leak intoCustomClaims.This is a self-contained alternative to the captured real tokens used elsewhere in
oidc_test.go, which avoids the key-rotation flakiness already flagged in that file.make testpasses locally;gofmt -lis clean on touched files;go vet ./...is clean.Notes for review
CustomOIDCProviderpath.user_metadata.custom_claimsand into the JWT access token viatokens/service.go. This is consistent with the existing Apple and Azure behaviors, not a new failure surface.parseGoogleIDToken,parseLinkedinIDToken,parseKakaoIDToken,parseFacebookIDToken,parseSnapchatIDToken,parseVercelMarketplaceIDToken) drop unknown claims the same way. The same fix pattern would apply, but each is a separate semantic change for that provider's user base. Happy to follow up in separate PRs if you'd like.