[MOSIP-44771] SECURITY: id_token_hint validation does not verify JWT signature#1873
[MOSIP-44771] SECURITY: id_token_hint validation does not verify JWT signature#1873Md-Humair-KK wants to merge 2 commits into
Conversation
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
WalkthroughA single method, ChangesID Token Hint Validation Security Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java`:
- Around line 459-467: The code reduces the JWT 'aud' to a single entry via
getFirst() which fails when the expected signupIDTokenAudience appears
elsewhere; update AuthorizationHelperService to read the full audience list from
jwt.getJWTClaimsSet().getAudience(), treat null/empty as missing, and then check
membership (e.g., audiences != null &&
audiences.contains(signupIDTokenAudience)); keep the clientId check
(signupIDTokenAudience.equals(clientId)) and throw
EsignetException(ErrorConstants.INVALID_ID_TOKEN_HINT) when the expected
audience is not present or clientId doesn't match.
🪄 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 UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 251d1ee5-7256-4d9b-b4a4-fed6037f91fc
📒 Files selected for processing (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java
| String audience = jwt.getJWTClaimsSet().getAudience() != null && !jwt.getJWTClaimsSet().getAudience().isEmpty() | ||
| ? jwt.getJWTClaimsSet().getAudience().getFirst() | ||
| : null; | ||
|
|
||
| // Defence-in-depth: re-check audience binding and that the caller's clientId is | ||
| // the expected signup audience. verifyIdToken above already validates 'aud', this | ||
| // extra check guards against any future divergence. | ||
| if (!signupIDTokenAudience.equals(audience) || !signupIDTokenAudience.equals(clientId)) { | ||
| throw new EsignetException(ErrorConstants.INVALID_ID_TOKEN_HINT); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JWT / OpenID Connect, can the aud claim contain multiple audiences, and should application code validate that the expected audience is contained in the list instead of comparing only the first entry? Please use RFC 7519, OpenID Connect Core, and Nimbus JOSE+JWT documentation.
💡 Result:
Yes, the aud (audience) claim in a JWT can contain multiple audiences [1][2][3]. According to RFC 7519, the aud value is generally an array of case-sensitive strings, though it may be a single string if there is only one audience [1][2]. Application code must validate that the expected audience is contained within the list, rather than comparing only the first entry [4][5]. RFC 7519 specifies that each principal intended to process the JWT must identify itself with a value in the audience claim; if the principal does not find its identifier in the claim, the JWT must be rejected [1][2]. OpenID Connect Core 1.0 reinforces this for ID Tokens, stating that the Client must validate that the aud claim contains its registered client_id [3][6]. When using the Nimbus JOSE+JWT library, you should avoid using exact-match claim validation for the audience if you need to support multiple potential values [7]. Instead, use the DefaultJWTClaimsVerifier constructor that accepts a Set of acceptedAudience values [8][9][7]. This ensures the library correctly checks if the token's audience list contains the expected identifier, rather than requiring an exact match of the entire array [7]. Top results: [1], [2], [3], [6], [8], [7]
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc7519.txt
- 2: https://datatracker.ietf.org/doc/html/rfc7519
- 3: https://openid.net/specs/openid-connect-core-1_0-errata2.html
- 4: https://workos.com/blog/how-to-validate-the-jwt-aud-claim-and-why-it-matters
- 5: https://mojoauth.com/blog/jwt-audience-claim-complete-developer-guide-with-examples
- 6: https://openid.net/specs/openid-connect-core-1_0.html
- 7: [Improvement] Support JWT tokens with multiple audiences in aud claim apache/gravitino#9733
- 8: https://javadoc.io/static/com.nimbusds/nimbus-jose-jwt/10.0/com/nimbusds/jwt/proc/DefaultJWTClaimsVerifier.html
- 9: https://stackoverflow.com/questions/71263642/nimbus-jose-jwt-expected-audience-claim-to-be-any-of-a-multiple
Check audience membership against the full aud list, not just the first entry.
Per RFC 7519 and OpenID Connect Core, the aud claim can contain multiple audiences. Reducing it to getAudience().getFirst() will reject a token whose aud list legitimately contains the expected audience at any position other than first, even if verifyIdToken(idTokenHint, signupIDTokenAudience) already validated it. The code must check membership across the entire list.
Suggested fix
- String audience = jwt.getJWTClaimsSet().getAudience() != null && !jwt.getJWTClaimsSet().getAudience().isEmpty()
- ? jwt.getJWTClaimsSet().getAudience().getFirst()
- : null;
+ List<String> audience = jwt.getJWTClaimsSet().getAudience();
// Defence-in-depth: re-check audience binding and that the caller's clientId is
// the expected signup audience. verifyIdToken above already validates 'aud', this
// extra check guards against any future divergence.
- if (!signupIDTokenAudience.equals(audience) || !signupIDTokenAudience.equals(clientId)) {
+ if (CollectionUtils.isEmpty(audience)
+ || !audience.contains(signupIDTokenAudience)
+ || !signupIDTokenAudience.equals(clientId)) {
throw new EsignetException(ErrorConstants.INVALID_ID_TOKEN_HINT);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String audience = jwt.getJWTClaimsSet().getAudience() != null && !jwt.getJWTClaimsSet().getAudience().isEmpty() | |
| ? jwt.getJWTClaimsSet().getAudience().getFirst() | |
| : null; | |
| // Defence-in-depth: re-check audience binding and that the caller's clientId is | |
| // the expected signup audience. verifyIdToken above already validates 'aud', this | |
| // extra check guards against any future divergence. | |
| if (!signupIDTokenAudience.equals(audience) || !signupIDTokenAudience.equals(clientId)) { | |
| throw new EsignetException(ErrorConstants.INVALID_ID_TOKEN_HINT); | |
| List<String> audience = jwt.getJWTClaimsSet().getAudience(); | |
| // Defence-in-depth: re-check audience binding and that the caller's clientId is | |
| // the expected signup audience. verifyIdToken above already validates 'aud', this | |
| // extra check guards against any future divergence. | |
| if (CollectionUtils.isEmpty(audience) | |
| || !audience.contains(signupIDTokenAudience) | |
| || !signupIDTokenAudience.equals(clientId)) { | |
| throw new EsignetException(ErrorConstants.INVALID_ID_TOKEN_HINT); | |
| } |
🤖 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
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java`
around lines 459 - 467, The code reduces the JWT 'aud' to a single entry via
getFirst() which fails when the expected signupIDTokenAudience appears
elsewhere; update AuthorizationHelperService to read the full audience list from
jwt.getJWTClaimsSet().getAudience(), treat null/empty as missing, and then check
membership (e.g., audiences != null &&
audiences.contains(signupIDTokenAudience)); keep the clientId check
(signupIDTokenAudience.equals(clientId)) and throw
EsignetException(ErrorConstants.INVALID_ID_TOKEN_HINT) when the expected
audience is not present or clientId doesn't match.
1 participant
Summary by CodeRabbit
id_token_hint, including improved audience verification and rejection of malformed or incomplete tokens.