Skip to content

Commit 1485642

Browse files
joemahady-commjoemahady-comm
authored
Fix UAA on standard ports (#3621)
* [TNZ-27070]: Fix UAA on standard ports with mismatching Location and Destination * [TNZ-27070]: Fix UAA on standard ports * [TNZ-27070]: Tidy code * [TNZ-27070]: Clean up code * [TNZ-27070]: refactor tests after review * [TNZ-27070]: refactor tests
1 parent d52aba3 commit 1485642

4 files changed

Lines changed: 217 additions & 1 deletion

File tree

‎server/src/main/java/org/cloudfoundry/identity/uaa/provider/saml/OpenSaml4AuthenticationProvider.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,8 @@
100100
import java.util.Map;
101101
import java.util.function.Consumer;
102102

103+
import static org.cloudfoundry.identity.uaa.util.UaaUrlUtils.normalizeUrlForPortComparison;
104+
103105
/**
104106
* This was copied from Spring Security, and modified to work with Open SAML 4.0.x
105107
* The original class only works with Open SAML 4.1.x+
@@ -225,7 +227,9 @@ public static Converter<ResponseToken, Saml2ResponseValidatorResult> createDefau
225227
String issuer = response.getIssuer().getValue();
226228
String destination = response.getDestination();
227229
String location = token.getRelyingPartyRegistration().getAssertionConsumerServiceLocation();
228-
if (StringUtils.hasText(destination) && !destination.equals(location)) {
230+
String normalizedDestination = normalizeUrlForPortComparison(destination);
231+
String normalizedLocation = normalizeUrlForPortComparison(location);
232+
if (StringUtils.hasText(destination) && !normalizedDestination.equals(normalizedLocation)) {
229233
String message = "Invalid destination [%s], location [%s] combo for SAML response [%s]"
230234
.formatted(destination, location, response.getID());
231235
result = result.concat(new Saml2Error(Saml2ErrorCodes.INVALID_DESTINATION, message));

‎server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,4 +356,31 @@ private static String decodeUriPath(final String path) {
356356

357357
throw new IllegalArgumentException("Aborted url decoding for repeatedly encoded path");
358358
}
359+
360+
/**
361+
* Normalizes a URL for port comparison by removing standard ports (80 for HTTP, 443 for HTTPS).
362+
* This ensures that URLs like "http://example.com" and "http://example.com:80" are treated as equivalent.
363+
*
364+
* @param url the URL to normalize
365+
* @return the normalized URL with standard ports removed, or the original URL if malformed
366+
*/
367+
public static String normalizeUrlForPortComparison(String url) {
368+
if (url == null) {
369+
return null;
370+
}
371+
try {
372+
URI uri = new URI(url);
373+
int port = uri.getPort();
374+
String scheme = uri.getScheme();
375+
376+
if (("http".equalsIgnoreCase(scheme) && port == 80) ||
377+
("https".equalsIgnoreCase(scheme) && port == 443)) {
378+
return new URI(scheme, uri.getUserInfo(), uri.getHost(), -1,
379+
uri.getPath(), uri.getQuery(), uri.getFragment()).toString();
380+
}
381+
} catch (Exception ignored) {
382+
// ignore
383+
}
384+
return url;
385+
}
359386
}

‎server/src/test/java/org/cloudfoundry/identity/uaa/provider/saml/OpenSaml4AuthenticationProviderUnitTests.java‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,140 @@ void authenticateWhenInvalidDestinationThenThrowAuthenticationException() {
138138
.satisfies(errorOf(Saml2ErrorCodes.INVALID_DESTINATION));
139139
}
140140

141+
@Test
142+
void authenticateWhenDestinationHasStandardHttpPortButLocationDoesNotThenSucceeds() {
143+
// Test scenario where destination includes explicit port 80 but location doesn't
144+
String destinationWithPort = "http://localhost:80/uaa/saml/SSO/alias/integration-saml-entity-id";
145+
String locationWithoutPort = "http://localhost/uaa/saml/SSO/alias/integration-saml-entity-id";
146+
147+
Response response = response(destinationWithPort, ASSERTING_PARTY_ENTITY_ID);
148+
Assertion assertion = assertion();
149+
// Set the recipient in subject confirmation to match the location (without port)
150+
assertion.getSubject().getSubjectConfirmations().forEach(sc ->
151+
sc.getSubjectConfirmationData().setRecipient(locationWithoutPort));
152+
response.getAssertions().add(assertion);
153+
154+
RelyingPartyRegistration.Builder registrationBuilder = verifying(registration())
155+
.assertionConsumerServiceLocation(locationWithoutPort);
156+
157+
Saml2AuthenticationToken token = token(signed(response), registrationBuilder);
158+
159+
// This should not throw an exception due to URL normalization
160+
assertThatNoException().isThrownBy(() -> this.provider.authenticate(token));
161+
}
162+
163+
@Test
164+
void authenticateWhenDestinationHasStandardHttpsPortButLocationDoesNotThenSucceeds() {
165+
// Test scenario where destination includes explicit port 443 but location doesn't
166+
String destinationWithPort = "https://localhost:443/uaa/saml/SSO/alias/integration-saml-entity-id";
167+
String locationWithoutPort = "https://localhost/uaa/saml/SSO/alias/integration-saml-entity-id";
168+
169+
Response response = response(destinationWithPort, ASSERTING_PARTY_ENTITY_ID);
170+
Assertion assertion = assertion();
171+
// Set the recipient in subject confirmation to match the location (without port)
172+
assertion.getSubject().getSubjectConfirmations().forEach(sc ->
173+
sc.getSubjectConfirmationData().setRecipient(locationWithoutPort));
174+
response.getAssertions().add(assertion);
175+
176+
RelyingPartyRegistration.Builder registrationBuilder = verifying(registration())
177+
.assertionConsumerServiceLocation(locationWithoutPort);
178+
179+
Saml2AuthenticationToken token = token(signed(response), registrationBuilder);
180+
181+
// This should not throw an exception due to URL normalization
182+
assertThatNoException().isThrownBy(() -> this.provider.authenticate(token));
183+
}
184+
185+
@Test
186+
void authenticateWhenLocationHasStandardPortButDestinationDoesNotThenSucceeds() {
187+
// Test reverse scenario where location includes explicit port but destination doesn't
188+
String destinationWithoutPort = "http://localhost/uaa/saml/SSO/alias/integration-saml-entity-id";
189+
String locationWithPort = "http://localhost:80/uaa/saml/SSO/alias/integration-saml-entity-id";
190+
191+
Response response = response(destinationWithoutPort, ASSERTING_PARTY_ENTITY_ID);
192+
Assertion assertion = assertion();
193+
// Set the recipient in subject confirmation to match the location (with port)
194+
assertion.getSubject().getSubjectConfirmations().forEach(sc ->
195+
sc.getSubjectConfirmationData().setRecipient(locationWithPort));
196+
response.getAssertions().add(assertion);
197+
198+
RelyingPartyRegistration.Builder registrationBuilder = verifying(registration())
199+
.assertionConsumerServiceLocation(locationWithPort);
200+
201+
Saml2AuthenticationToken token = token(signed(response), registrationBuilder);
202+
203+
// This should not throw an exception due to URL normalization
204+
assertThatNoException().isThrownBy(() -> this.provider.authenticate(token));
205+
}
206+
207+
@Test
208+
void authenticateWhenNonStandardPortMismatchThenThrowsException() {
209+
// Test that non-standard ports still cause validation failure when they don't match
210+
String destinationWithPort8080 = "http://localhost:8080/uaa/saml/SSO/alias/integration-saml-entity-id";
211+
String locationWithPort8081 = "http://localhost:8081/uaa/saml/SSO/alias/integration-saml-entity-id";
212+
213+
Response response = response(destinationWithPort8080, ASSERTING_PARTY_ENTITY_ID);
214+
Assertion assertion = assertion();
215+
// Set the recipient in subject confirmation to match the location (port 8081)
216+
assertion.getSubject().getSubjectConfirmations().forEach(sc ->
217+
sc.getSubjectConfirmationData().setRecipient(locationWithPort8081));
218+
response.getAssertions().add(assertion);
219+
220+
RelyingPartyRegistration.Builder registrationBuilder = verifying(registration())
221+
.assertionConsumerServiceLocation(locationWithPort8081);
222+
223+
Saml2AuthenticationToken token = token(signed(response), registrationBuilder);
224+
225+
// This should still throw an exception since ports don't match and aren't standard
226+
assertThatExceptionOfType(Saml2AuthenticationException.class)
227+
.isThrownBy(() -> this.provider.authenticate(token))
228+
.satisfies(errorOf(Saml2ErrorCodes.INVALID_DESTINATION));
229+
}
230+
231+
@Test
232+
void authenticateWhenDestinationIsEmpty_thenSkipsValidationWithoutNPE() {
233+
// Test that when destination is empty/null, validation is skipped without throwing NPE
234+
String validLocation = DESTINATION;
235+
236+
// Create response with null/empty destination
237+
Response response = response(null, ASSERTING_PARTY_ENTITY_ID);
238+
Assertion assertion = assertion();
239+
assertion.getSubject().getSubjectConfirmations().forEach(sc ->
240+
sc.getSubjectConfirmationData().setRecipient(validLocation));
241+
response.getAssertions().add(assertion);
242+
243+
RelyingPartyRegistration.Builder registrationBuilder = verifying(registration())
244+
.assertionConsumerServiceLocation(validLocation);
245+
246+
Saml2AuthenticationToken token = token(signed(response), registrationBuilder);
247+
248+
// This should not throw a NullPointerException when destination is null
249+
// The validation should be skipped since StringUtils.hasText(destination) returns false
250+
assertThatNoException().isThrownBy(() -> this.provider.authenticate(token));
251+
}
252+
253+
@Test
254+
void authenticateWhenMalformedUrlsButIdentical_thenSucceeds() {
255+
// Test that malformed URLs that can't be normalized still work if they're identical
256+
// This tests the robustness of the comparison when normalization might fail
257+
String malformedUrl = "http://[malformed:url:with:brackets]/saml/SSO/alias/integration-saml-entity-id";
258+
259+
Response response = response(malformedUrl, ASSERTING_PARTY_ENTITY_ID);
260+
Assertion assertion = assertion();
261+
assertion.getSubject().getSubjectConfirmations().forEach(sc ->
262+
sc.getSubjectConfirmationData().setRecipient(malformedUrl));
263+
response.getAssertions().add(assertion);
264+
265+
RelyingPartyRegistration.Builder registrationBuilder = verifying(registration())
266+
.assertionConsumerServiceLocation(malformedUrl);
267+
268+
Saml2AuthenticationToken token = token(signed(response), registrationBuilder);
269+
270+
// Even with malformed URLs that normalization can't handle,
271+
// authentication should succeed if both URLs are identical
272+
assertThatNoException().isThrownBy(() -> this.provider.authenticate(token));
273+
}
274+
141275
@Test
142276
void authenticateWhenNoAssertionsPresentThenThrowAuthenticationException() {
143277
Saml2AuthenticationToken token = token();

‎server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import static org.assertj.core.api.Assertions.assertThat;
2828
import static org.assertj.core.api.Assertions.fail;
2929
import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
30+
import static org.cloudfoundry.identity.uaa.util.UaaUrlUtils.normalizeUrlForPortComparison;
3031
import static org.mockito.Mockito.mock;
3132

3233
@ExtendWith(PollutionPreventionExtension.class)
@@ -710,4 +711,54 @@ private static Map<String, String> getUnsuccessfulUrls(List<String> urls, boolea
710711
private static List<String> convertToHttps(List<String> urls) {
711712
return urls.stream().map(url -> url.replace("http:", "https:")).toList();
712713
}
714+
715+
// Tests for normalizeUrlForPortComparison method
716+
@ParameterizedTest(name = "normalizeUrlForPortComparison: \"{0}\" should become \"{1}\"")
717+
@CsvSource({
718+
// Standard port removal
719+
"'http://example.com:80/path', 'http://example.com/path'",
720+
"'https://example.com:443/path', 'https://example.com/path'",
721+
722+
// Non-standard ports preserved
723+
"'http://example.com:8080/path', 'http://example.com:8080/path'",
724+
"'https://example.com:8443/path', 'https://example.com:8443/path'",
725+
726+
// URLs without explicit ports remain unchanged
727+
"'http://example.com/path', 'http://example.com/path'",
728+
"'https://example.com/path', 'https://example.com/path'",
729+
730+
// Query parameters and fragments preserved
731+
"'http://example.com:80/path?param1=value1&param2=value2', 'http://example.com/path?param1=value1&param2=value2'",
732+
"'https://example.com:443/path#section1', 'https://example.com/path#section1'",
733+
734+
// Complex URLs with user info
735+
"'http://user:pass@example.com:80/path/to/resource?query=value#fragment', 'http://user:pass@example.com/path/to/resource?query=value#fragment'",
736+
737+
// Subdomains preserved
738+
"'https://subdomain.example.com:443/path', 'https://subdomain.example.com/path'",
739+
740+
// Different schemes keep non-standard ports
741+
"'ftp://example.com:80/path', 'ftp://example.com:80/path'",
742+
743+
// SAML-specific URL
744+
"'https://uaa.example.com:443/saml/SSO/alias/provider-name?RelayState=https://app.example.com&param=value#section', 'https://uaa.example.com/saml/SSO/alias/provider-name?RelayState=https://app.example.com&param=value#section'",
745+
746+
// Malformed URLs return unchanged
747+
"'not-a-valid-url', 'not-a-valid-url'",
748+
"'http://[invalid:url:with:colons:everywhere', 'http://[invalid:url:with:colons:everywhere'",
749+
"'http://example.com:80/path with spaces and special chars!@#$%^&*()', 'http://example.com:80/path with spaces and special chars!@#$%^&*()'",
750+
751+
// Empty string
752+
"'', ''"
753+
})
754+
void normalizeUrlForPortComparison_parameterizedTests(String inputUrl, String expectedUrl) {
755+
String result = normalizeUrlForPortComparison(inputUrl);
756+
assertThat(result).isNotNull().isEqualTo(expectedUrl); // Ensure we never return null for non-null input
757+
}
758+
759+
@Test
760+
void normalizeUrlForPortComparison_withNullUrl_returnsNull() {
761+
String result = normalizeUrlForPortComparison(null);
762+
assertThat(result).isNull();
763+
}
713764
}

0 commit comments

Comments
 (0)