oidc: extract id_token claims, enforce email_verified, harden sub val… by babs · Pull Request #590 · root-gg/plik

babs · 2026-02-16T12:29:45Z

…idation

Parse id_token JWT from token endpoint response and merge claims with userinfo (userinfo takes precedence per OIDC Core spec)
Handle email_verified as bool, string, or numeric to support non-compliant IdPs (AWS Cognito, Entra)
Add OIDCRequireVerifiedEmail config option to reject unverified emails
Enforce OIDC Core 5.3.4 sub mismatch check between id_token and userinfo (logged server-side, generic error to client)
Synthesize display name from given_name + family_name when name is absent
Log OIDC discovery background refresh failures
Add InitOIDCDiscovery logger parameter for background goroutine logging

…idation - Parse id_token JWT from token endpoint response and merge claims with userinfo (userinfo takes precedence per OIDC Core spec) - Handle email_verified as bool, string, or numeric to support non-compliant IdPs (AWS Cognito, Entra) - Add OIDCRequireVerifiedEmail config option to reject unverified emails - Enforce OIDC Core 5.3.4 sub mismatch check between id_token and userinfo (logged server-side, generic error to client) - Synthesize display name from given_name + family_name when name is absent - Log OIDC discovery background refresh failures - Add InitOIDCDiscovery logger parameter for background goroutine logging

camathieu

Hey @babs, thanks for this excellent contribution! The code quality and test coverage are really impressive — you've handled a lot of real-world IdP quirks (AWS Cognito, Entra) that would've bitten users in production.

Overall Assessment

Architecture & Logic: ✅ Solid

The id_token → userinfo claims merging follows OIDC Core spec correctly
Sub mismatch check (5.3.4) with server-side logging + generic client error is good security practice
email_verified polymorphism handling (bool, string, float64) is exactly what's needed for interop
Deep-copy semantics in mergeClaims prevent subtle aliasing bugs — nice attention to detail
Background refresh failure logging fills an important observability gap

Test Coverage: ✅ Outstanding (+584 lines)

Unit tests for all new functions (parseIDTokenClaims, mergeClaims, UnmarshalJSON)
Integration tests for all new callback paths (verified email, sub mismatch, name synthesis)
Edge case coverage (nil inputs, pointer aliasing, malformed JWTs, all email_verified variants)

Required Changes

🔴 go fmt failure — CI is blocking on formatting issues in server/handlers/oidc.go and server/handlers/oidc_test.go. The root cause is struct field alignment: PreferredUsername is longer than adjacent fields, which gofmt wants to re-align. Same issue in config.go with OIDCRequireVerifiedEmail.

Fix:

gofmt -w server/handlers/oidc.go server/handlers/oidc_test.go server/common/config.go

That should be the only blocker — happy to approve once the formatting is fixed! 🎉

camathieu · 2026-02-16T19:43:47Z

server/handlers/oidc.go


+type oidcClaims struct {
+	Sub              string `json:"sub"`
+	Email            string `json:"email"`
+	EmailVerified    *bool  `json:"-"` // Populated exclusively by custom UnmarshalJSON to handle bool/string/numeric variants
+	Name             string `json:"name"`
+	GivenName        string `json:"given_name"`
+	FamilyName       string `json:"family_name"`
+	PreferredUsername string `json:"preferred_username"`
+	Picture          string `json:"picture"`
+	Locale           string `json:"locale"`


🔴 CI failure: go fmt — The PreferredUsername field name is longer than FamilyName, which breaks Go's tab-alignment for the struct fields. go fmt wants consistent alignment across the entire struct group.

Run gofmt -w server/handlers/oidc.go server/handlers/oidc_test.go to fix. The struct will likely end up looking like:

type oidcClaims struct { Sub string `json:"sub"` Email string `json:"email"` EmailVerified *bool `json:"-"` Name string `json:"name"` GivenName string `json:"given_name"` FamilyName string `json:"family_name"` PreferredUsername string `json:"preferred_username"` Picture string `json:"picture"` Locale string `json:"locale"` }

(Note the extra space before each type to align with PreferredUsername.)

camathieu · 2026-02-16T19:43:50Z

server/common/config.go

+	OIDCAuthentication       bool     `json:"oidcAuthentication"`
+	OIDCClientID             string   `json:"-"`
+	OIDCClientSecret         string   `json:"-"`
+	OIDCProviderURL          string   `json:"-"`
+	OIDCProviderName         string   `json:"oidcProviderName"`
+	OIDCValidDomains         []string `json:"-"`
+	OIDCRequireVerifiedEmail bool     `json:"-"`

 	MetadataBackendConfig map[string]interface{} `json:"-"`



Same go fmt issue: OIDCRequireVerifiedEmail is longer than OIDCValidDomains, so the alignment of the entire struct field group needs to be adjusted. Running gofmt -w on the file should fix this automatically.

camathieu · 2026-02-16T19:43:54Z

server/handlers/oidc.go

+		return nil, fmt.Errorf("unable to marshal id_token claims: %s", err)
+	}
+
+	var claims oidcClaims


Nice — using jwt.WithoutClaimsValidation() + ParseUnverified is correct here since the token was obtained directly from the token endpoint over TLS. The comment explaining this is appreciated. 👍

camathieu · 2026-02-16T19:44:02Z

server/handlers/oidc.go

 		}
+		return
+	}
+	oidcDiscoveryCache = &oidcDiscoveryEntry{
+		discovery:   discovery,
+		providerURL: providerURL,
+		fetchedAt:   time.Now(),
 	}


Good improvement — the original code silently swallowed refresh failures. Logging them as warnings is the right severity, and guarding with oidcLog != nil keeps it safe for tests where the logger isn't set.

camathieu · 2026-02-16T19:44:05Z

server/handlers/oidc.go

 		return
 	}

-	var userInfo oidcUserInfo
-	if err := json.NewDecoder(io.LimitReader(userinfoResp.Body, oidcMaxResponseSize)).Decode(&userInfo); err != nil {
+	var userinfoClaims oidcClaims
+	if err := json.NewDecoder(io.LimitReader(userinfoResp.Body, oidcMaxResponseSize)).Decode(&userinfoClaims); err != nil {
 		ctx.InternalServerError("unable to parse OIDC userinfo", err)
 		return
 	}


✅ OIDC Core 5.3.4 compliance — spec says: "the sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token". Logging the actual values server-side while showing a generic error to the client is the correct security posture.

Minor suggestion: you could consider using ctx.GetLogger().Warningf(...) consistently, but since this aligns with the existing error handling pattern in this file, it looks fine as-is.

camathieu · 2026-02-16T19:44:07Z

server/handlers/oidc_test.go

+			name:         "unexpected type (array)",
+			json:         `{"sub":"s","email_verified":[]}`,
+			wantVerified: nil,
+			wantSub:      "s",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var claims oidcClaims
+			err := json.Unmarshal([]byte(tt.json), &claims)
+			require.NoError(t, err)
+			require.Equal(t, tt.wantSub, claims.Sub)
+			if tt.wantVerified == nil {
+				require.Nil(t, claims.EmailVerified)
+			} else {
+				require.NotNil(t, claims.EmailVerified)
+				require.Equal(t, *tt.wantVerified, *claims.EmailVerified)
+			}
+		})
+	}
+}
+
+func TestOIDCCallbackEmailVerifiedFromUserinfo(t *testing.T) {
+	ResetOIDCDiscoveryCache()
+	ctx := newTestingContext(common.NewConfiguration())
+	setupOIDCConfig(ctx.GetConfig())
+	ctx.GetConfig().OIDCRequireVerifiedEmail = true
+
+	// id_token has NO email_verified
+	idTokenClaims := oidcClaims{
+		Sub:   "userinfo-ev-user",
+		Email: "ev@root.gg",
+		Name:  "EV User",
+	}
+
+	// email_verified comes exclusively from userinfo
+	userinfoClaims := oidcClaims{
+		Sub:           "userinfo-ev-user",
+		EmailVerified: boolPtr(true),
+	}
+
+	_, shutdown, err := common.StartAPIMockServerCustomPort(common.APIMockServerDefaultPort, oidcMockHandler(oidcMockOptions{
+		userinfo: userinfoClaims,
+		idToken:  &idTokenClaims,
+	}))
+	defer shutdown()
+	require.NoError(t, err, "unable to start mock server")
+
+	req := oidcCallbackRequest(t, oidcTestState(t, ctx.GetConfig().OIDCClientSecret))
+	rr := ctx.NewRecorder(req)
+	OIDCCallback(ctx, rr, req)
+
+	require.Equal(t, 301, rr.Code, "login should succeed when email_verified comes from userinfo")
+
+	user, err := ctx.GetMetadataBackend().GetUser("oidc:userinfo-ev-user")
+	require.NoError(t, err)
+	require.NotNil(t, user)
+	require.Equal(t, "ev@root.gg", user.Email)
+}
+
 func TestDiscoverOIDCCache(t *testing.T) {
 	ResetOIDCDiscoveryCache()



Excellent test coverage for the email_verified polymorphism — covers bool, string (case-insensitive), float64, null, absent, and unexpected type. This is exactly the kind of edge-case testing needed for interoperability with non-compliant IdPs.

camathieu · 2026-02-16T19:48:44Z

AI Review went too far and posted response directly on GH without even asking for confirmation, sorry about that.
LGTM :)

babs and others added 2 commits February 16, 2026 13:29

Merge branch 'master' into feature/oidc-id-token-claims

2284976

camathieu requested changes Feb 16, 2026

View reviewed changes

fix: go fmt struct field alignment

0af86ed

camathieu merged commit 6e656b5 into root-gg:master Feb 16, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oidc: extract id_token claims, enforce email_verified, harden sub val…#590

oidc: extract id_token claims, enforce email_verified, harden sub val…#590
camathieu merged 3 commits intoroot-gg:masterfrom
babs:feature/oidc-id-token-claims

babs commented Feb 16, 2026

Uh oh!

camathieu left a comment

Uh oh!

camathieu Feb 16, 2026

Uh oh!

camathieu Feb 16, 2026

Uh oh!

camathieu Feb 16, 2026

Uh oh!

camathieu Feb 16, 2026

Uh oh!

camathieu Feb 16, 2026

Uh oh!

camathieu Feb 16, 2026

Uh oh!

camathieu commented Feb 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

babs commented Feb 16, 2026

Uh oh!

camathieu left a comment

Choose a reason for hiding this comment

Overall Assessment

Required Changes

Uh oh!

camathieu Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

camathieu Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

camathieu Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

camathieu Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

camathieu Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

camathieu Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

camathieu commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

camathieu commented Feb 16, 2026 •

edited

Loading