This is an automated email from the ASF dual-hosted git repository.
nscendoni pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-auth-oauth-client.git
The following commit(s) were added to refs/heads/master by this push:
new e1f98cf SLING-13363: Harden post-login redirect validation against
open redirect (#52)
e1f98cf is described below
commit e1f98cfe932b4311ba4e5006905ce35fef8fe597
Author: Nicola Scendoni <[email protected]>
AuthorDate: Thu Sep 24 16:21:22 2026 +0200
SLING-13363: Harden post-login redirect validation against open redirect
(#52)
The application-controlled post-login redirect parameter had weak
validation and was decoded twice on the OAuth callback path, which
together could turn a validated value into a protocol-relative URL
(e.g. //evil.com). On a servlet container/proxy that emits a relative
Location header, this enables an open redirect.
- OAuthCallbackServlet.handleRedirect: stop URL-decoding the redirect
a second time; use the value stored in the encrypted state cookie
verbatim, and re-validate it as defense in depth right before
sendRedirect.
- RedirectHelper.validateRedirect: replace the weak denylist (reject
leading '//') with a positive check (isSafeRelativePath) that
requires a single leading '/' and rejects '//', '/\', any embedded
backslash, control characters, and any value that parses to a URI
with a scheme/authority/host.
- OidcAuthenticationHandler.getAuthenticationRequestUri: validate the
request.getRequestURI() fallback (used when no redirect parameter
is supplied) instead of using it unchecked, failing safe (no
post-login redirect) if validation fails.
- Add regression tests for the double-decode bypass, the backslash
denylist bypass, direct isSafeRelativePath edge cases, and the
unsafe request-URI fallback.
Co-authored-by: Copilot <[email protected]>
---
.../oauth_client/impl/OAuthCallbackServlet.java | 22 ++++++---
.../impl/OidcAuthenticationHandler.java | 18 +++++--
.../auth/oauth_client/impl/RedirectHelper.java | 37 +++++++++++++-
.../impl/OAuthCallbackServletTest.java | 37 ++++++++++++++
.../impl/OidcAuthenticationHandlerTest.java | 57 ++++++++++++++++++++++
.../auth/oauth_client/impl/RedirectHelperTest.java | 55 ++++++++++++++++++++-
6 files changed, 212 insertions(+), 14 deletions(-)
diff --git
a/src/main/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServlet.java
b/src/main/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServlet.java
index 579e48a..48e32c1 100644
---
a/src/main/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServlet.java
+++
b/src/main/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServlet.java
@@ -27,11 +27,8 @@ import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
-import java.net.URLDecoder;
-import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Map;
-import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Collectors;
@@ -247,11 +244,22 @@ public class OAuthCallbackServlet extends
SlingAllMethodsServlet {
private static void handleRedirect(@NotNull OAuthCookieValue clientState,
@NotNull HttpServletResponse response)
throws IOException {
- Optional<String> redirect =
Optional.ofNullable(clientState.redirect());
- if (redirect.isEmpty()) {
+ String redirect = clientState.redirect();
+ if (redirect == null || redirect.isEmpty()) {
response.setStatus(HttpServletResponse.SC_NO_CONTENT);
- } else {
- response.sendRedirect(URLDecoder.decode(redirect.get(),
StandardCharsets.UTF_8));
+ return;
+ }
+ // Do NOT URL-decode again: the value stored in the (encrypted) cookie
is already the decoded
+ // request parameter. A second decode would turn an innocuous-looking
"/%2Fevil.com" into a
+ // protocol-relative "//evil.com", defeating the leading-"//" check
and enabling an open
+ // redirect. Re-validate here as defense in depth before emitting the
Location header.
+ try {
+ RedirectHelper.validateRedirect(redirect);
+ } catch (OAuthEntryPointException e) {
+ logger.warn("Refusing to redirect to invalid post-login location
'{}'", redirect);
+ response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
+ return;
}
+ response.sendRedirect(redirect);
}
}
diff --git
a/src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java
b/src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java
index 4ea79dd..f912317 100644
---
a/src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java
+++
b/src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java
@@ -678,10 +678,22 @@ public class OidcAuthenticationHandler extends
DefaultAuthenticationFeedbackHand
// then after the authentication redirect to the requested uri
// Extract path and query from the uri
- redirect = request.getRequestURI();
+ String requestUri = request.getRequestURI();
String queryString = request.getQueryString();
- if (queryString != null && !queryString.isEmpty()) {
- redirect = redirect + "?" + queryString;
+ String candidateRedirect =
+ (queryString != null && !queryString.isEmpty()) ?
requestUri + "?" + queryString : requestUri;
+ // request.getRequestURI() is server-derived and should always be
a safe site-relative
+ // path, but validate it anyway as defense in depth before it
reaches the redirect sink,
+ // and fail safe (no post-login redirect) rather than propagate an
unvalidated value.
+ try {
+ RedirectHelper.validateRedirect(candidateRedirect);
+ redirect = candidateRedirect;
+ } catch (OAuthEntryPointException e) {
+ logger.warn(
+ "Refusing to use request URI '{}' as post-login
redirect target: {}",
+ candidateRedirect,
+ e.getMessage());
+ redirect = null;
}
}
diff --git
a/src/main/java/org/apache/sling/auth/oauth_client/impl/RedirectHelper.java
b/src/main/java/org/apache/sling/auth/oauth_client/impl/RedirectHelper.java
index e62bb8d..7c828dd 100644
--- a/src/main/java/org/apache/sling/auth/oauth_client/impl/RedirectHelper.java
+++ b/src/main/java/org/apache/sling/auth/oauth_client/impl/RedirectHelper.java
@@ -169,10 +169,43 @@ class RedirectHelper {
if (redirect == null || redirect.isEmpty()) {
return;
}
- if (!redirect.startsWith("/") || redirect.startsWith("//")) {
+ if (!isSafeRelativePath(redirect)) {
String message = "Invalid redirect URL: " + redirect;
- // Relative redirect within the same domain is allowed
+ // Only a site-relative path within the same domain is allowed
throw new OAuthEntryPointException(message, new
IllegalArgumentException(message));
}
}
+
+ /**
+ * Returns {@code true} only if {@code redirect} is a safe site-relative
path that cannot be
+ * turned into a cross-origin redirect. A single leading {@code '/'} is
required; the following
+ * are rejected because browsers may treat them as absolute/cross-origin
targets:
+ * <ul>
+ * <li>protocol-relative URLs ({@code //host} or {@code /\host} — a
backslash is normalised to
+ * {@code /} by browsers);</li>
+ * <li>any backslash anywhere (browsers normalise {@code \} to {@code
/});</li>
+ * <li>control characters (may be stripped by browsers to reveal a
different target);</li>
+ * <li>absolute URIs (with a scheme) or URIs carrying an authority/host
component.</li>
+ * </ul>
+ */
+ static boolean isSafeRelativePath(@NotNull String redirect) {
+ if (!redirect.startsWith("/") || redirect.startsWith("//") ||
redirect.startsWith("/\\")) {
+ return false;
+ }
+ for (int i = 0; i < redirect.length(); i++) {
+ char c = redirect.charAt(i);
+ if (c == '\\' || c < 0x20 || c == 0x7f) {
+ return false;
+ }
+ }
+ try {
+ URI uri = new URI(redirect);
+ if (uri.isAbsolute() || uri.getAuthority() != null ||
uri.getHost() != null) {
+ return false;
+ }
+ } catch (URISyntaxException e) {
+ return false;
+ }
+ return true;
+ }
}
diff --git
a/src/test/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServletTest.java
b/src/test/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServletTest.java
index 334dc20..674d919 100644
---
a/src/test/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServletTest.java
+++
b/src/test/java/org/apache/sling/auth/oauth_client/impl/OAuthCallbackServletTest.java
@@ -237,4 +237,41 @@ class OAuthCallbackServletTest {
.as("location header")
.isEqualTo("/local-redirect");
}
+
+ /**
+ * Regression test for the redirect double-decode open-redirect vector.
+ *
+ * <p>At flow start the entry point reads {@code redirect=/%252Fevil.com};
{@code getParameter}
+ * decodes it once to {@code /%2Fevil.com}, which passes validation and is
stored in the (encrypted)
+ * state cookie. The callback must emit that value verbatim. A second
URL-decode here would turn it
+ * into the protocol-relative {@code //evil.com}, which a container
emitting relative redirects would
+ * send to the browser as a cross-origin redirect to {@code evil.com}.
+ */
+ @Test
+ void redirectIsNotDoubleDecoded() throws IOException, ServletException {
+ successfulExecution("bar|mock-oidc-local|/%2Fevil.com");
+
+ assertThat(context.response().getStatus()).as("response
code").isEqualTo(HttpServletResponse.SC_FOUND);
+
+ String location = context.response().getHeader("Location");
+ assertThat(location)
+ .as("Location must not be double-decoded into a
protocol-relative URL")
+ .isEqualTo("/%2Fevil.com")
+ .doesNotStartWith("//");
+ }
+
+ /**
+ * Regression test for the denylist bypass: a backslash after the leading
slash ({@code /\evil.com})
+ * is normalised by browsers to {@code //evil.com}. The callback
re-validates the stored redirect and
+ * must reject it rather than emit it in a {@code Location} header.
+ */
+ @Test
+ void redirectWithBackslashIsRejected() throws IOException,
ServletException {
+ successfulExecution("bar|mock-oidc-local|/\\evil.com");
+
+ assertThat(context.response().getStatus()).as("response
code").isEqualTo(HttpServletResponse.SC_BAD_REQUEST);
+ assertThat(context.response().getHeader("Location"))
+ .as("no redirect must be emitted for a rejected target")
+ .isNull();
+ }
}
diff --git
a/src/test/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandlerTest.java
b/src/test/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandlerTest.java
index f07d87d..34a0dd3 100644
---
a/src/test/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandlerTest.java
+++
b/src/test/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandlerTest.java
@@ -1597,6 +1597,63 @@ class OidcAuthenticationHandlerTest {
}));
}
+ /**
+ * Regression test: when no {@code redirect} parameter is supplied, the
fallback to
+ * {@code request.getRequestURI()} must still be validated before being
stored in the
+ * (encrypted) state cookie and later emitted in the callback's {@code
Location} header. An
+ * unsafe request URI (e.g. one containing a backslash, which browsers
normalise to a second
+ * slash) must be rejected and fail safe, i.e. no post-login redirect is
stored, rather than
+ * being propagated unchecked.
+ */
+ @Test
+ void requestCredentialsWithUnsafeRequestURIFallsBackSafely() {
+ // This is the class used by Sling to configure the Authentication
Handler
+ OidcProviderMetadataRegistry oidcProviderMetadataRegistry =
mock(OidcProviderMetadataRegistry.class);
+ String mockIdPUrl = "http://localhost:8080";;
+
when(oidcProviderMetadataRegistry.getJWKSetURI(mockIdPUrl)).thenReturn(URI.create(mockIdPUrl
+ "/jwks.json"));
+
when(oidcProviderMetadataRegistry.getIssuer(mockIdPUrl)).thenReturn(ISSUER);
+ when(oidcProviderMetadataRegistry.getAuthorizationEndpoint(mockIdPUrl))
+ .thenReturn(URI.create(mockIdPUrl + "/authorize"));
+
when(oidcProviderMetadataRegistry.getTokenEndpoint(mockIdPUrl)).thenReturn(URI.create(mockIdPUrl
+ "/token"));
+
+ connections.add(new MockOidcConnection(
+ new String[] {"openid"},
+ MOCK_OIDC_PARAM,
+ "client-id",
+ "client-secret",
+ "http://localhost:8080";,
+ new String[] {"access_type=offline"},
+ oidcProviderMetadataRegistry));
+
+ config = createConfig(Map.of(
+ "defaultConnectionName",
+ MOCK_OIDC_PARAM,
+ "callbackUri",
+ "http://redirect";,
+ "pkceEnabled",
+ false,
+ "path",
+ new String[] {"/"}));
+
+ when(request.getParameter("c")).thenReturn(MOCK_OIDC_PARAM);
+
when(request.getParameter(RedirectHelper.PARAMETER_NAME_REDIRECT)).thenReturn(null);
+ when(request.getRequestURI()).thenReturn("/\\evil.com");
+ MockSlingHttpServletResponse mockResponse = new
MockSlingHttpServletResponse();
+
+ createOidcAuthenticationHandler();
+ assertTrue(oidcAuthenticationHandler.requestCredentials(request,
mockResponse));
+
+ // The unsafe request URI must not be stored as the post-login
redirect target
+ assertTrue(Arrays.stream(mockResponse.getCookies()).anyMatch(cookie ->
{
+ if
(OAuthCookieValue.COOKIE_NAME_REQUEST_KEY.equals(cookie.getName())) {
+ OAuthCookieValue oauthCookieValue = new
OAuthCookieValue(cookie.getValue(), cryptoService);
+ assertNull(oauthCookieValue.redirect());
+ return true;
+ }
+ return false;
+ }));
+ }
+
@Test
void requestCredentialsWithResourceAttribute() {
// This is the class used by Sling to configure the Authentication
Handler
diff --git
a/src/test/java/org/apache/sling/auth/oauth_client/impl/RedirectHelperTest.java
b/src/test/java/org/apache/sling/auth/oauth_client/impl/RedirectHelperTest.java
index 0bc6420..f1f6e53 100644
---
a/src/test/java/org/apache/sling/auth/oauth_client/impl/RedirectHelperTest.java
+++
b/src/test/java/org/apache/sling/auth/oauth_client/impl/RedirectHelperTest.java
@@ -133,10 +133,18 @@ class RedirectHelperTest {
"//example.com/path",
"https://example.com/path";,
"ftp://example.com/path";,
- "javascript:alert('xss')"
+ "javascript:alert('xss')",
+ "/\\evil.com",
+ "/\\\\evil.com",
+ "/path\\to\\evil.com",
+ "/path\ttab",
+ "/path\nnewline",
+ "/path\rreturn",
})
void testValidateRedirectWithInvalidUrl(String url) {
- // Should throw exception for absolute URLs (cross-site redirect)
+ // Should throw exception for absolute URLs (cross-site redirect) and
for the
+ // backslash/control-character bypasses that a browser may normalise
into a
+ // protocol-relative or otherwise unexpected target.
OAuthEntryPointException exception =
assertThrows(OAuthEntryPointException.class, () ->
RedirectHelper.validateRedirect(url));
@@ -144,6 +152,49 @@ class RedirectHelperTest {
assertTrue(exception.getCause() instanceof IllegalArgumentException);
}
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "/",
+ "/valid/path",
+ "/another/valid/path",
+ "/path?query=value",
+ "/path#fragment",
+ "/path%20encoded",
+ })
+ void testIsSafeRelativePathAcceptsSiteRelativePaths(String redirect) {
+ assertTrue(RedirectHelper.isSafeRelativePath(redirect), () -> redirect
+ " should be considered safe");
+ }
+
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ // no leading slash
+ "path",
+ "",
+ // protocol-relative / backslash bypasses
+ "//evil.com",
+ "/\\evil.com",
+ "/\\\\evil.com",
+ "\\\\evil.com",
+ "\\/evil.com",
+ // embedded backslash anywhere in the path
+ "/path\\to\\evil.com",
+ // control characters
+ "/path\u0000null",
+ "/path\ttab",
+ "/path\nnewline",
+ "/path\rreturn",
+ "/path\u007Fdel",
+ // absolute URIs / URIs carrying an authority
+ "http://evil.com";,
+ "https://evil.com/path";,
+ "//evil.com/path",
+ })
+ void testIsSafeRelativePathRejectsUnsafeInput(String redirect) {
+ assertFalse(RedirectHelper.isSafeRelativePath(redirect), () ->
redirect + " should be considered unsafe");
+ }
+
@Test
void testBuildRedirectTargetWithSingleAudience() {
ResolvedConnection conn = createMockResolvedConnection();
