Always validate post-logout redirect URIs in SecureRedirectUrisEnforcerExecutor by jimmychakkalakal · Pull Request #53038 · keycloak/keycloak

jimmychakkalakal · 2026-09-22T09:23:06Z

The secure-redirect-uris-enforcer executor skipped post-logout redirect URI validation when both standard flow and implicit flow were disabled, and read post-logout URIs only from the proposed representation. Together this allowed a two-step bypass: store an HTTP post-logout URI with flows off, then re-enable flows via a partial update that omits the attributes map. The stored HTTP URI survived and was used at RP-initiated logout.

What changed

verifyPostLogoutRedirectUriUpdate now runs on every CREATE/UPDATE, regardless of flow state.
When the proposed representation omits post.logout.redirect.uris, the executor revalidates the value stored on the client instead of silently accepting the update.

Upgrade impact

Two classes of client are now rejected on CREATE/UPDATE:

Both flows disabled with HTTP redirect URIs, and either no post.logout.redirect.uris attribute or the
+placeholder — both inherit the redirect URIs.
An explicit HTTP post.logout.redirect.uris stored in attributes, whether set before the policy was active or retained
through the bypass above. Partial updates that omit the attributes map now revalidate it, so even a rename is
rejected.
Remediation: set post.logo 8000 ut.redirect.uris to - to opt out of enforcement, or migrate to HTTPS. Both work before and after upgrading. Documented in the 26.8.0 upgrading guide.

Closes #50982

PR Description Generated with Claude Code

Copilot

Copilot review overview

🟡 Changes recommended

The reported partial-update security bypass remains possible, and the regression test does not exercise a true partial update.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Post-logout validation falls back to safe redirects, bypassing insecure URIs · New
Test uses a full client representation instead of a partial update · New

What changed in this PR

Updates secure redirect enforcement to validate post-logout URIs regardless of enabled authentication flows.

Changes:

Moves post-logout validation outside flow checks.
Adds stored-client fallback and integration tests.

File	Description
`SecureRedirectUrisEnforcerExecutor.java`	Adds unconditional post-logout URI validation.
`SecureRedirectUrisEnforcerExecutorTest.java`	Adds regression scenarios for URI validation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟡 Changes recommended

Stored URI fallback remains bypassable, relative URIs can use the wrong root URL, and the tests do not exercise true partial updates.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 · 1 · 1

Open (4)

Fall back to stored rootUrl when retaining logout URIs · New
Post-logout validation falls back to safe redirects, bypassing insecure URIs
Test partial update without resubmitting stored logout URIs · New
Test uses a full client representation instead of a partial update

Copilot

Copilot review overview

🟡 Changes recommended

Partial-update state merging remains incorrect, leaving a policy bypass and blocking explicit URI removal.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 · 1

Open (3)

Resolve stored redirect URIs against the updated root URL · New
Post-logout validation falls back to safe redirects, bypassing insecure URIs
Test uses a full client representation instead of a partial update

Resolved since last review (2)

Fall back to stored rootUrl when retaining logout URIs
Test partial update without resubmitting stored logout URIs

Previously missed (1)

In code that hasn't changed since last review

Honor explicit '-' to disable post-logout redirects

services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRedirectUrisEnforcerExecutor.java:245

An explicitly supplied "-" is converted to an empty list, so this condition replaces the intended disabled configuration with the stored URI list. A client with a pre-existing invalid URI is therefore unable to remediate it by disabling post-logout redirects; distinguish an omitted attribute from the explicit "-" sentinel before falling back.

Copilot

Copilot review overview

🟡 Changes recommended

Partial updates containing safe redirect URIs can still bypass validation of a retained insecure post-logout URI.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 · 1

Open (3)

Test safe redirectUris with omitted attributes to expose bypass · New
Post-logout validation falls back to safe redirects, bypassing insecure URIs
Test uses a full client representation instead of a partial update

Resolved since last review (1)

Resolve stored redirect URIs against the updated root URL

Copilot

Copilot review overview

🟡 Changes recommended

The security-sensitive root URL fallback lacks a test where root selection affects validation.

Review effort: Balanced
Findings: 2

Open (2)

Add rootUrl fallback tests with relative post-logout URI · New
Test uses a full client representation instead of a partial update

Resolved since last review (2)

Test safe redirectUris with omitted attributes to expose bypass
Post-logout validation falls back to safe redirects, bypassing insecure URIs

abstractj · 2026-10-09T18:08:50Z

Hi @jimmychakkalakal — I validated this PR against the two-step bypass scenario from #50982 and all 14 tests pass. The fix is solid: moving the stored ClientModel into verifyPostLogoutRedirectUriUpdate correctly closes both the flow-disabled seed path and the partial-update omission path.

While reviewing the executor layer for the same pattern, I noticed two other executors with a related hardening gap worth flagging:

SecureClientUrisExecutor
On UPDATE, confirmSecureUris(proposed) is called with no fallback to the stored model. A partial update that omits the attributes map skips revalidation of already-stored URI fields — backchannelLogoutUrl, frontChannelLogoutUrl, jwksUri, requestUris, cibaClientNotificationEndpoint, postLogoutRedirectUris, and redirectUris. Unlike #50982 there is no conditional guard that allows seeding a bad value at creation time, so this is not an exploitable bypass — but it means a pre-existing non-compliant URI (e.g. registered before the policy was active) will not be caught when the client is later updated.

SecureClientUrisPatternExecutor
Same gap — validateClientUris(proposed) on UPDATE with no stored-model fallback. Any URI field in the configured clientUriFields absent from a partial update payload is silently skipped.

Neither rises to the level of a vulnerability given that creation-time validation is not bypassable in these executors, but both are worth a follow-up hardening issue to bring them in line with the pattern this PR establishes for SecureRedirectUrisEnforcerExecutor.

For reference, RejectResourceOwnerPasswordCredentialsGrantExecutor and SamlSecureClientUrisExecutor already handle the partial-update case correctly.

Those are no deal breakers but weaknesses that might worth to consider cc @ahus1

jimmychakkalakal · 2026-10-12T09:13:17Z

Thanks @abstractj for validating the fix and reviewing the related executors.

I’m happy to open a separate hardening issue for partial-update handling in SecureClientUrisExecutor and SecureClientUrisPatternExecutor and follow up with a separate Issue/PR, keeping this PR focused on #50982.

@ahus1, would this approach work for you?

abstractj · 2026-10-13T15:40:42Z

@jimmychakkalakal please ensure to create those follow up issues linking to this issue, before merging the changes.

jimmychakkalakal · 2026-10-14T08:59:57Z

@abstractj Created follow-up hardening issue #53952 for SecureClientUrisExecutor and SecureClientUrisPatternExecutor, linked to #50982 and this PR.

jimmychakkalakal · 2026-10-14T15:33:03Z

Hi @mposolda , Bruno just approved this PR. Whenever you have a moment, could you take a look and merge it if everything looks good on your end? Thanks!

mposolda

@jimmychakkalakal @abstractj Thanks for the PR and review!

AFAIK the correct upgrading file for Keycloak upstream main branch should be changes-27_0_0.adoc . Otherwise it is OK to me.

jimmychakkalakal · 2026-10-20T10:16:04Z

@jimmychakkalakal @abstractj Thanks for the PR and review!

AFAIK the correct upgrading file for Keycloak upstream main branch should be changes-27_0_0.adoc . Otherwise it is OK to me.

@mposolda Thanks for the comment. I have moved documentation to changes-27_0_0.adoc

Copilot

🔵 Needs a closer look

Security-sensitive validation and compatibility changes need human confirmation supported by integration-test results.

1 open issue

Distinguish retained blank attributes from removed blanks during validation

_{Review effort: Balanced}

Copilot

🟢 Approval recommended

Static review identified no unresolved blocking issues within the agreed scope; integration tests were not executed.

1 open issue

Distinguish retained blank attributes from removed blanks during validation

_{Review effort: Balanced}

mposolda

@jimmychakkalakal Thanks! LGTM, but there is conflict. Could you rebase please?

jimmychakkalakal · 2026-10-27T12:54:35Z

@jimmychakkalakal Thanks! LGTM, but there is conflict. Could you rebase please?

Thanks @mposolda I have rebased the branch with latest main

Copilot

🟡 Changes recommended

Filtering empty entries from explicit post-logout attributes leaves an HTTP redirect policy bypass.

2 open issues

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Filtering empty post-logout redirect entries skips validation even though they remain persisted, allowing an empty logout redirect to resolve to an attacker-controlled HTTP root URL.

2 open issues

_{Review effort: Balanced}

mposolda

@jimmychakkalakal Thanks for the update! Adding inline comment related to copilot.

…erExecutor Validation was previously skipped when standard and implicit flows were disabled. This allowed a bypass where an insecure HTTP post-logout URI could be saved while flows were off, then kept active when flows were re-enabled via a partial update (which omitted the attributes map). Fixed by running verifyPostLogoutRedirectUriUpdate on every CREATE/UPDATE regardless of flow state. Partial updates missing the attributes map now fall back to stored ClientModel URIs to prevent retaining policy-violating values. Closes keycloak#50982 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>

Closes keycloak#50982 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>

…ents suggested by Bruno Closes keycloak#50982 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>

Clarified instructions for handling post-logout URI enforcement and updated information regarding OIDC Client CRs. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: jimmychakkalakal <47317106+jimmychakkalakal@users.noreply.github.com>

Closes keycloak#51275 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com> # Conflicts: # docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc

Copilot

🟢 Approval recommended

No active review issues are reported for the post-logout redirect URI validation changes.

1 open issue

Validate explicit post-logout redirect entries before filtering

1 resolved since last review

Distinguish retained blank attributes from removed blanks during validation

_{Review effort: Balanced}

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2026-11-05T15:57:33Z

Unreported flaky test detected

If 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.adapter.servlet.SAMLLoginResponseHandlingTest#testErrorHandlingUnsigned

Keycloak CI - Adapter IT

org.jboss.arquillian.graphene.enricher.exception.PageObjectInitializationException: 
Can not instantiate Page Object 
class org.keycloak.testsuite.adapter.page.Employee2Servlet
 declared in: 
org.keycloak.testsuite.adapter.servlet.SAMLLoginResponseHandlingTest
...

Conversation

Uh oh!

What changed

Upgrade impact

Uh oh!

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.adapter.servlet.SAMLLoginResponseHandlingTest#testErrorHandlingUnsigned

org.keycloak.testsuite.adapter.servlet.SAMLLoginResponseHandlingTest#testErrorHandlingSigned

org.keycloak.testsuite.adapter.servlet.SAMLLoginResponseHandlingTest#testAttributes

org.keycloak.testsuite.adapter.servlet.SAMLLoginResponseHandlingTest#testNilAttributeValueAttribute

org.keycloak.testsuite.adapter.servlet.SAMLClockSkewAdapterTest#testTokenTimeIsValid

org.keycloak.testsuite.adapter.servlet.SAMLClockSkewAdapterTest#testTokenHasExpired

org.keycloak.testsuite.adapter.servlet.SAMLClockSkewAdapterTest#testTokenIsNotYetValid

org.keycloak.testsuite.adapter.servlet.SAMLLogoutAdapterTest#testLogoutDestinationOptionalIfUnsignedRedirect

org.keycloak.testsuite.webauthn.AppInitiatedActionWebAuthnSkipIfExistsTest#processSetupTwice

org.keycloak.testsuite.webauthn.AppInitiatedActionWebAuthnSkipIfExistsTest#proceedSetupWebAuthnLogoutOtherSessionsChecked

org.keycloak.testsuite.webauthn.AppInitiatedActionWebAuthnSkipIfExistsTest#cancelSetupWebAuthn

Uh oh!

Choose a reason for hiding this comment