From 309feae86ebf63706d9b70c88307a18f4bcd06bd Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Wed, 28 Sep 2022 06:35:19 +0200 Subject: [PATCH] Fix regression with legacy redirect patterns (#2035) * Fix regression with legacy redirect patterns Port wildcards were working, but because of spring change this fails now Bring it back now with this PR. Remark: legacy patterns were in past and again now written to log with warnings, so that you see them Example WARN --- LegacyRedirectResolver: OAuth client app is configured with a redirect_uri which performs implicit or wildcard matching in legacy redirect uri matching mode. In this instance, the requested uri http://localhost:3000/authentication/callback matches the configured uri http://localhost:*/authentication/callback. Please consider configuring your requested redirect uri to exactly match the redirect_uri for this client. * refactoring and test added * cleanup --- .../uaa/oauth/beans/LegacyRedirectResolver.java | 14 +++++++++----- .../identity/uaa/util/UaaUrlUtils.java | 2 +- .../oauth/beans/LegacyRedirectResolverTest.java | 12 ++++++++++++ .../identity/uaa/util/UaaUrlUtilsTest.java | 9 +++++++++ 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index a338aa0cf03..e6314ac960e 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -32,6 +32,9 @@ public class LegacyRedirectResolver extends org.cloudfoundry.identity.uaa.oauth.beans.org.springframework.security.oauth2.provider.endpoint.DefaultRedirectResolver { private static final Logger logger = LoggerFactory.getLogger(LegacyRedirectResolver.class); + + public static final String LEGACY_PORT_WILDCAR = ":*"; + static final String MSG_TEMPLATE = "OAuth client %s is configured with a redirect_uri which performs implicit or " + "wildcard matching in legacy redirect uri matching mode. In this instance, the requested uri %s matches the " + "configured uri %s. Please consider configuring your requested redirect uri to exactly match the " + @@ -101,9 +104,10 @@ private void logConfiguredRedirectUrisWhichOnlyMatchFuzzily(String clientId, Set // when the standard Spring library class disagrees (i.e. when it acts more strictly). registeredRedirectUris.stream() .filter(registeredRedirectUri -> - requestedRedirect != null && + registeredRedirectUri.contains(LEGACY_PORT_WILDCAR) || + (requestedRedirect != null && this.redirectMatches(requestedRedirect, registeredRedirectUri) && - !specCompliantRedirectMatcher.redirectMatches(requestedRedirect, registeredRedirectUri) + !specCompliantRedirectMatcher.redirectMatches(requestedRedirect, registeredRedirectUri)) ) .forEach(registeredRedirectUri -> logger.warn(String.format(MSG_TEMPLATE, clientId, @@ -113,8 +117,8 @@ private void logConfiguredRedirectUrisWhichOnlyMatchFuzzily(String clientId, Set } private static String normalizeWildcardUri(String uriClient) { - boolean hasWildcarPort = uriClient.contains(":*"); - String uri = hasWildcarPort ? uriClient.replace(":*", StringUtils.EMPTY) : uriClient; + boolean hasWildcarPort = uriClient.contains(LEGACY_PORT_WILDCAR); + String uri = hasWildcarPort ? uriClient.replace(LEGACY_PORT_WILDCAR, StringUtils.EMPTY) : uriClient; UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUriString(uri); UriComponents nonNormalizedUri = uriComponentsBuilder.build(); @@ -132,7 +136,7 @@ private static String normalizeWildcardUri(String uriClient) { throw new IllegalArgumentException("URI host and scheme must not be null"); } - return uriComponentsBuilder.build().toString().replace(":99999", ":*"); + return uriComponentsBuilder.build().toString().replace(":99999", LEGACY_PORT_WILDCAR); } private static String redactSensitiveInformation(String uri) { diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java index d22738ddfc1..982dc4287f7 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java @@ -77,7 +77,7 @@ private static UriComponentsBuilder getURIBuilder( "(([a-zA-Z0-9\\-\\*\\_]+\\.){0,255}" + //subdomains (RFC1035) limited, regex backtracking disabled "[a-zA-Z0-9\\-\\_]+\\.)?" + //hostname "[a-zA-Z0-9\\-]+" + //tld - "(:[0-9]+)?(/.*|$)" //port and path + "((:[0-9]+)|(:\\*))?(/.*|$)" //port and path ); public static boolean isValidRegisteredRedirectUrl(String url) { diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java index ec1988cfd15..3e894fe0033 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java @@ -157,6 +157,18 @@ void warnsOnExplicitDomainExpansion() { ); } + @Test + void warnsOnPortWildCard() { + final String configuredRedirectUri = "https://example.com:*/*"; + final String requestedRedirectUri = "https://example.com:443/callback"; + ClientDetails client = createClient("foo", configuredRedirectUri); + + resolver.resolveRedirect(requestedRedirectUri, client); + assertThat(logEvents, hasItem( + warning(expectedWarning(client.getClientId(), requestedRedirectUri, configuredRedirectUri))) + ); + } + @Test void warnsOnImplicitPathExpansion() { final String configuredRedirectUri = "https://example.com/"; diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java index 6b921f16fbb..e0948d7cf46 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java @@ -647,6 +647,15 @@ void getRequestPathCombinesServletPathAndPathInfo( assertEquals(expected, UaaUrlUtils.getRequestPath(request)); } + @Test + void testLegacyUriWithPortWildCard() { + assertTrue(UaaUrlUtils.isValidRegisteredRedirectUrl("http://localhost:*/callback")); + + assertFalse(UaaUrlUtils.isValidRegisteredRedirectUrl(Strings.EMPTY_STRING)); + assertFalse(UaaUrlUtils.isValidRegisteredRedirectUrl("http://localhost:80*/callback")); + assertFalse(UaaUrlUtils.isValidRegisteredRedirectUrl("http://localhost:*8/callback")); + } + private static void validateRedirectUri(List urls, boolean result) { Map failed = getUnsuccessfulUrls(urls, result); if (!failed.isEmpty()) {