Repository navigation
Fix SAML endpoint 500 on unauthenticated LogoutRequest with no logout URL - #52592
msaravanan77 wants to merge 4 commits into
Conversation
… URL SamlService.BindingProtocol.logoutRequest() falls through to its default POST/Redirect response path whenever the request is unauthenticated and carries no matching session index. That path calls SamlProtocol.getLogoutServiceUrl(), which returns null when the client has no logout service URL configured for the binding in use and no management URL to fall back to. The null destination then reaches KeycloakUriBuilder.fromUri(), which throws IllegalArgumentException, surfacing to the client as an unhandled HTTP 500 instead of a clean error. Guard the null/blank destination the same way the method already guards validateDestination() failures a few lines above: log an INVALID_SAML_LOGOUT_REQUEST event and return 400 INVALID_REQUEST. SOAP binding is excluded from the check since it doesn't need a destination URL. Adds a regression test to LogoutTest exercising the exact scenario: an unauthenticated logout request (cookies cleared) sent via REDIRECT binding to a client that only has its POST logout service URL configured. Closes keycloak#51371 Signed-off-by: msaravanan77 <msaravanan77@gmail.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Prevents unhandled HTTP 500s when processing unauthenticated SAML LogoutRequests for clients that lack a logout service URL for the request binding, returning a consistent 400 INVALID_REQUEST instead.
Changes:
- Add a guard in
SamlService.logoutRequest()to detect missing/blank logout destination URLs (non-SOAP) and return400 INVALID_REQUEST. - Add an integration test reproducing the REDIRECT-binding case when only a POST logout service URL is configured.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
services/src/main/java/org/keycloak/protocol/saml/SamlService.java |
Adds a defensive check for missing logout destination URL to avoid builder errors and return a 400. |
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/LogoutTest.java |
Adds a regression test ensuring the scenario returns 400 instead of 500. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…th no logout URL Follow-up to the previous commit addressing PR review feedback: the fix guards both bindings, but only the REDIRECT-bound scenario was covered by a test. Add the mirror case - the "sales-post" client reconfigured with only its REDIRECT single-logout-service URL, receiving an unauthenticated POST-bound LogoutRequest - which must likewise be rejected with 400 INVALID_REQUEST rather than failing with a 500. Signed-off-by: msaravanan77 <msaravanan77@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
There was a problem hiding this comment.
🟢 Approval recommended
The focused guard addresses the failure safely, and both affected bindings have regression coverage.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Closes #51371
Problem
SamlService.BindingProtocol.logoutRequest()falls through to its default POST/RedirectLogoutResponsepath whenever the incomingLogoutRequestis unauthenticated (no identity cookie) and carries no matching session index. That path callsSamlProtocol.getLogoutServiceUrl(), which returnsnullwhen the client has no single-logout-service URL configured for the binding in use and no management URL to fall back to.The
nulldestination then flows into the response builders (builder.destination(null)/binding.redirectBinding(...).response(null)), whereKeycloakUriBuilder.fromUri()throwsIllegalArgumentException: uri parameter is null. It is wrapped into aRuntimeExceptionand surfaces to the client as an unhandled HTTP 500 instead of a clean error.Both HTTP-Redirect and HTTP-POST bindings are affected.
Fix
Guard the
null/blank destination the same way the method already guardsvalidateDestination()failures a few lines above: log anINVALID_SAML_LOGOUT_REQUESTevent and return 400INVALID_REQUEST. This matches the behaviour the issue asks for and is consistent with how the same method already rejects other unusable logout requests.SOAP binding is excluded from the check since its response path does not need a destination URL.
Testing
LogoutTest.testLogoutNoLogoutServiceUrlConfiguredForBindingReturnsBadRequestreproduces the exact scenario: thesales-postclient is configured with only its POST single-logout-service URL, then an unauthenticated (clearCookies())LogoutRequestis sent over the REDIRECT binding — so there is no destination for theLogoutResponse. The test asserts a400response rather than a500.LogoutTest.testLogoutNoLogoutServiceUrlConfiguredForPostBindingReturnsBadRequestcovers the mirror case: the client reconfigured with only its REDIRECT SLO URL, receiving an unauthenticated POST-boundLogoutRequest.AI agent disclosure
This PR was authored with the assistance of an AI agent (Claude Code) working from a prompt. The author has reviewed and understands the change, verified it against the codebase, and is responsible for addressing review feedback.
🤖 Generated with Claude Code