Skip to content

Commit

Permalink
Fix regression with legacy redirect patterns (#2035)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
strehle committed Sep 28, 2022
1 parent a9cd64f commit 309feae
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 6 deletions.
Expand Up @@ -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 " +
Expand Down Expand Up @@ -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,
Expand All @@ -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();

Expand All @@ -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) {
Expand Down
Expand Up @@ -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) {
Expand Down
Expand Up @@ -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/";
Expand Down
Expand Up @@ -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<String> urls, boolean result) {
Map<String, String> failed = getUnsuccessfulUrls(urls, result);
if (!failed.isEmpty()) {
Expand Down

0 comments on commit 309feae

Please sign in to comment.