fix: block spoofable IP headers from rate limiting and restrict SAML RelayState redirects - #3999
Open
strehle wants to merge 1 commit into
Open
fix: block spoofable IP headers from rate limiting and restrict SAML RelayState redirects#3999strehle wants to merge 1 commit into
strehle wants to merge 1 commit into
Conversation
…RelayState redirects
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens redirect and client-IP handling in the UAA server by (1) restricting SAML RelayState / redirect override URLs to the same host to reduce open-redirect risk, and (2) expanding the default set of filtered (spoofable) IP-related headers so rate limiting won’t trust them by default.
Changes:
- Restrict SAML RelayState redirects to URLs whose host matches the current request host.
- Restrict
override.redirect_uriandform_redirect_urihandling to same-host URLs. - Add
X-Client-IPandX-Real-IPto the defaultservlet.filtered-headerslist.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestAwareAuthenticationSuccessHandler.java | Enforces same-host validation for RelayState and redirect override/form parameters. |
| server/src/main/java/org/cloudfoundry/identity/uaa/UaaProperties.java | Extends default filtered headers to include X-Client-IP / X-Real-IP. |
| server/src/test/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestAwareAuthenticationSuccessHandlerTests.java | Updates/extends tests to assert same-host acceptance and external-host rejection. |
| server/src/test/java/org/cloudfoundry/identity/uaa/UaaPropertiesTest.java | Updates defaults test to include the newly filtered headers. |
Comments suppressed due to low confidence (1)
server/src/main/java/org/cloudfoundry/identity/uaa/web/UaaSavedRequestAwareAuthenticationSuccessHandler.java:76
- This redirect validation depends on
UaaUrlUtils.uriHasMatchingHost, which currently performs a case-sensitive hostname comparison and is not null-safe for thehostnameargument. With the newly added host restriction, that can cause legitimate same-host override URLs to be rejected unexpectedly. Consider fixinguriHasMatchingHostcentrally to compare hosts case-insensitively and handle nulls.
public String determineTargetUrl(HttpServletRequest request, HttpServletResponse response) {
String redirectAttribute = request.getAttribute(URI_OVERRIDE_ATTRIBUTE) instanceof String overrideString ? overrideString : null;
String redirectFormParam = request.getParameter(FORM_REDIRECT_PARAMETER);
if (redirectAttribute != null && UaaUrlUtils.uriHasMatchingHost(redirectAttribute, request.getServerName())) {
log.debug("Returning redirectAttribute saved URI: {}", redirectAttribute);
return redirectAttribute;
} else if (UaaUrlUtils.uriHasMatchingHost(redirectFormParam, request.getServerName())) {
return redirectFormParam;
Comment on lines
47
to
51
| String relayState = UaaStringUtils.getCleanedUserControlString(request.getParameter(Saml2ParameterNames.RELAY_STATE), UaaStringUtils.EMPTY_STRING); | ||
| if (UaaStringUtils.hasText(relayState) && UaaUrlUtils.isUrl(relayState)) { | ||
| if (UaaStringUtils.hasText(relayState) && UaaUrlUtils.isUrl(relayState) | ||
| && UaaUrlUtils.uriHasMatchingHost(relayState, request.getServerName())) { | ||
| log.debug("Redirecting to relayState URI: {}", relayState); | ||
| this.getRedirectStrategy().sendRedirect(request, response, relayState); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: block spoofable IP headers from rate limiting and restrict SAML) RelayState redirects