Reject redirect URIs containing pre-loaded OIDC response parameters#49959
Conversation
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 17) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 21) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 17) |
1ca4f4f to
9efc7c9
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 17) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 17) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 21) |
eeb3fec to
ca8502c
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 17) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 17) org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClientKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 21) |
ca8502c to
621dc51
Compare
mposolda
left a comment
There was a problem hiding this comment.
@jimmychakkalakal Thanks for this PR!
Added some comments inline. Can you please check?
|
Hi @jimmychakkalakal, I'm sharing this from Niro, a PR pentesting tool we're building to help OSS maintainers catch security regressions before merge. I ran it against this PR and it found the following issue. A user initiating OIDC login can preload session_state in the redirect_uri, causing a valid code response with both user-chosen and Keycloak-issued session_state values; first-value parsers will track the wrong session state for that login. Reproduction StepsPrecondition: use CR_003 (standard user in realm niro).
Remediation GuidanceIn services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java, add "session_state" to FORBIDDEN_OIDC_PARAMS so verifyRedirectUri rejects redirect URIs that preload Keycloak’s OIDC response parameter before auth begins. Extend RedirectUtilsTest with a case for https://example.com/callback?session_state=attack, alongside the existing code/state/iss coverage. |
6fc2be0 to
4b30a25
Compare
mposolda
left a comment
There was a problem hiding this comment.
@jimmychakkalakal Thanks! The first 2 commits of your PR makes sense and I would approve such a PR.
But I don't understand why there is a need of the changes from your 3rd commit 4b30a25 . I don't see any relationship of this commit to the addressed GH issue. Did you added this commit by accident? Or is that any issue with adminEvents in the testsuite? In that case, it might be good to address it rather in the separate PR than this one.
Thanks @mposolda no, it wasn't an accident. The tests were failing, and I found out it has to do with uncleared admin events. I will then remove the adminevent releated changes from the PR |
Prevent HTTP Parameter Pollution by validating that incoming redirect URIs do not already contain reserved OIDC query parameters (e.g., code, state, iss) before matching. Closes keycloak#49430 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
- Replace hardcoded strings with OAuth2Constants and Constants - Add missing parameters: session_state, response, kc_action, kc_action_status - Update OIDCRedirectUriBuilder to use OAuth2Constants.RESPONSE - Add tests for new parameters Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
4b30a25 to
d613424
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds protection against HTTP parameter pollution in OIDC redirect URIs by rejecting redirect URIs that already contain reserved OIDC response parameters, and updates/extends tests accordingly.
Changes:
- Added a forbidden-parameter check to
RedirectUtils.verifyRedirectUri(...)to reject redirect URIs containing reserved OIDC/OAuth parameters in the query string. - Added new unit/integration tests for polluted redirect URIs and stabilized some admin-event assertions by clearing
adminEventsbefore tests. - Replaced hard-coded
"response"usage withOAuth2Constants.RESPONSEinOIDCRedirectUriBuilder.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/AbstractOIDCResponseTypeTest.java | Removes a test that relied on now-forbidden session_state in the redirect URI. |
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java | Adds integration tests to ensure polluted redirect URIs are rejected; adds a positive test for custom query params. |
| tests/base/src/test/java/org/keycloak/tests/admin/realm/AbstractRealmRolesTest.java | Clears adminEvents before each test to reduce cross-test interference. |
| tests/base/src/test/java/org/keycloak/tests/admin/identityprovider/IdentityProviderSamlTest.java | Uses unique IDP alias per run to avoid collisions. |
| tests/base/src/test/java/org/keycloak/tests/admin/identityprovider/AbstractIdentityProviderTest.java | Clears adminEvents before each test to reduce cross-test interference. |
| tests/base/src/test/java/org/keycloak/tests/admin/group/AbstractGroupTest.java | Clears adminEvents before each test to reduce cross-test interference. |
| tests/base/src/test/java/org/keycloak/tests/admin/client/AbstractClientScopeTest.java | Clears adminEvents before each test to reduce cross-test interference. |
| tests/base/src/test/java/org/keycloak/tests/admin/authentication/AbstractAuthenticationTest.java | Clears adminEvents in setup to reduce cross-test interference. |
| tests/base/src/test/java/org/keycloak/tests/admin/AttackDetectionResourceTest.java | Clears adminEvents mid-test before asserting subsequent admin events. |
| services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java | Adds unit tests verifying polluted redirect URIs are rejected. |
| services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java | Implements the forbidden-parameter rejection logic for redirect URIs. |
| services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java | Uses OAuth2Constants.RESPONSE instead of hard-coded parameter name. |
d613424 to
076f43c
Compare
Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
c7b92e7 to
e059fe6
Compare
|
@mposolda I have removed all admin event-related code from the tests. Unlike my previous commits, the pipeline is green this time. I assume it has something to do with the rebase. Could you please review the PR? Thanks in advance! |
mposolda
left a comment
There was a problem hiding this comment.
@jimmychakkalakal Thanks!
Prevent HTTP Parameter Pollution by validating that incoming redirect URIs do not already contain reserved OIDC query parameters (e.g., code, state, iss) before matching.
Closes #49430