fix: preserve ID token claims over UserInfo response claims#19
fix: preserve ID token claims over UserInfo response claims#19
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request strengthens the security posture of OpenID Connect authentication by modifying how user claims are processed. It ensures that claims obtained from the cryptographically signed ID token are never overwritten by claims from the UserInfo endpoint, thereby preventing potential identity spoofing if the UserInfo endpoint were to be compromised. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughUpdated OpenID Connect provider claim merging: getUserInfo now preserves existing ID token claims and only adds UserInfo claims for keys not present in the ID token. A unit test was added to verify this preservation and augmentation behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability by ensuring that cryptographically signed ID token claims are not overwritten by claims from the UserInfo endpoint. The implementation correctly prioritizes the ID token claims. A new test is added to verify this behavior. The changes are well-implemented, and I have one suggestion to improve the robustness of the new test case.
| provider, _ := NewCustomisedURL( | ||
| "client_id", "client_secret", "http://localhost/callback", | ||
| "https://example.com/auth", "https://example.com/token", | ||
| "https://example.com", userInfoServer.URL, "", | ||
| ) |
There was a problem hiding this comment.
It's good practice to check for errors returned from functions, even in tests, to make them more robust. While NewCustomisedURL currently doesn't return an error, its signature allows for it, and this could change in the future. It would be safer to handle the potential error.
| provider, _ := NewCustomisedURL( | |
| "client_id", "client_secret", "http://localhost/callback", | |
| "https://example.com/auth", "https://example.com/token", | |
| "https://example.com", userInfoServer.URL, "", | |
| ) | |
| provider, err := NewCustomisedURL( | |
| "client_id", "client_secret", "http://localhost/callback", | |
| "https://example.com/auth", "https://example.com/token", | |
| "https://example.com", userInfoServer.URL, "", | |
| ) | |
| a.NoError(err) |
References
- Use the
testifypackage for assertions in tests to maintain consistency with the existing codebase.
There was a problem hiding this comment.
Fixed in latest push — now checks a.NoError(err) on NewCustomisedURL.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/openidConnect/openidConnect_test.go (1)
224-229: Assert provider construction error in test setup.Line [224] currently discards
NewCustomisedURLerror. Please assert it (a.NoError(err)) so setup failures fail fast and don’t cascade into misleading assertion errors.Suggested patch
- provider, _ := NewCustomisedURL( + provider, err := NewCustomisedURL( "client_id", "client_secret", "http://localhost/callback", "https://example.com/auth", "https://example.com/token", "https://example.com", userInfoServer.URL, "", ) + a.NoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/openidConnect/openidConnect_test.go` around lines 224 - 229, The test currently discards the error returned by NewCustomisedURL; change the call to capture the error (e.g., provider, err := NewCustomisedURL(...)) and assert it with a.NoError(err) immediately after construction so setup failures fail fast and don't produce cascading test errors; reference the NewCustomisedURL call and the provider variable in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@providers/openidConnect/openidConnect_test.go`:
- Around line 224-229: The test currently discards the error returned by
NewCustomisedURL; change the call to capture the error (e.g., provider, err :=
NewCustomisedURL(...)) and assert it with a.NoError(err) immediately after
construction so setup failures fail fast and don't produce cascading test
errors; reference the NewCustomisedURL call and the provider variable in the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e10a5b7-e8cf-4a4e-bb26-1da33e127d96
📒 Files selected for processing (2)
providers/openidConnect/openidConnect.goproviders/openidConnect/openidConnect_test.go
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability by ensuring that claims from a signed ID token are not overwritten by claims from an unsigned UserInfo response. The change modifies the claim merging logic to only add new claims from the UserInfo endpoint if they don't already exist. A new test case has been added to verify this behavior, confirming that existing claims are preserved and new ones are added correctly. The changes are logical, well-implemented, and properly tested.
tphakala
force-pushed
the
fix/oidc-userinfo-claim-priority
branch
from
22d0905 to
d6e8310
Compare
Previously, UserInfo response claims overwrote ID token claims. Since ID token claims are cryptographically signed and UserInfo claims are not, the signed claims should take precedence. UserInfo claims are now only added when not already present in the ID token.
tphakala
force-pushed
the
fix/oidc-userinfo-claim-priority
branch
from
d6e8310 to
c26cb25
Compare
1 participant
Summary
Security impact: Previously, a compromised UserInfo endpoint could overwrite signed ID token claims (e.g., email), allowing identity spoofing while the
subclaim still matched.Test plan
go test ./providers/openidConnect/... -vpassesTest_GetUserInfo_PreservesIDTokenClaimsverifies ID token email is preserved when UserInfo returns a different email, and new claims from UserInfo are still addedSummary by CodeRabbit
Bug Fixes
Tests