From 8705088adbbc6fdb14724a66ea767c8793aee1c7 Mon Sep 17 00:00:00 2001 From: Andrea Tabone Date: Tue, 14 Jul 2026 15:37:42 +0200 Subject: [PATCH 1/2] add sanitiziation for the returnUrl Signed-off-by: Andrea Tabone --- .../apiml/gateway/config/WebSecurity.java | 144 +++++++++++++++++- .../apiml/gateway/config/WebSecurityTest.java | 105 ++++++++++++- 2 files changed, 242 insertions(+), 7 deletions(-) diff --git a/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java b/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java index a21b7110dd..bc767ab610 100644 --- a/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java +++ b/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java @@ -83,6 +83,8 @@ import reactor.core.publisher.Mono; import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.security.KeyStoreException; import java.security.MessageDigest; @@ -131,6 +133,9 @@ public class WebSecurity { @Value("${apiml.security.enableStrictUrlValidation:false}") private boolean isStrictUrlValidationEnabled; + @Value("${apiml.service.externalUrl}") + private String externalUrl; + private final ClientConfiguration clientConfiguration; private final TokenProvider tokenProvider; @@ -213,13 +218,134 @@ SecurityWebFilterChain oauth2WebFilterChain( .build(); } - public Mono updateCookies(WebFilterExchange webFilterExchange, OAuth2AuthorizedClient oAuth2AuthorizedClient) { - webFilterExchange.getExchange().getResponse().addCookie(defaultCookieAttr(ResponseCookie.from(COOKIE_AUTH_NAME, oAuth2AuthorizedClient.getAccessToken().getTokenValue())).build()); - var location = webFilterExchange.getExchange().getRequest().getCookies().getFirst(COOKIE_RETURN_URL); - if (!HAS_NO_VALUE.test(location)) { - redirect(webFilterExchange.getExchange().getResponse(), location.getValue()); + private String sanitizeRelativeUri(URI uri) { + /* + * Reject: + * //attacker.example + * attacker.example/path + * gateway:foo + * + * Only root-relative paths are allowed. + */ + if (uri.getRawAuthority() != null + || uri.getRawUserInfo() != null) { + return CONTEXT_PATH; + } + + String path = uri.getRawPath(); + + if (StringUtils.isEmpty(path) + || !path.startsWith("/") + || path.startsWith("//")) { + return CONTEXT_PATH; + } + + return toRelativeLocation(uri); + } + + private boolean isSameOrigin(URI candidate, URI configuredExternalUri) { + if (candidate.getScheme() == null + || candidate.getHost() == null + || configuredExternalUri.getScheme() == null + || configuredExternalUri.getHost() == null + || candidate.getRawUserInfo() != null) { + return false; + } + + return candidate.getScheme().equalsIgnoreCase(configuredExternalUri.getScheme()) + && candidate.getHost().equalsIgnoreCase(configuredExternalUri.getHost()) + && effectivePort(candidate) == effectivePort(configuredExternalUri); + } + + private int effectivePort(URI uri) { + if (uri.getPort() >= 0) { + return uri.getPort(); + } + + if ("https".equalsIgnoreCase(uri.getScheme())) { + return 443; + } + + if ("http".equalsIgnoreCase(uri.getScheme())) { + return 80; + } + + return -1; + } + + // defense-in-depth + private String toRelativeLocation(URI uri) { + String path = StringUtils.defaultIfEmpty(uri.getRawPath(), "/"); + + StringBuilder location = new StringBuilder(path); + + if (uri.getRawQuery() != null) { + location.append('?').append(uri.getRawQuery()); + } + + if (uri.getRawFragment() != null) { + location.append('#').append(uri.getRawFragment()); } + + return location.toString(); + } + + private String sanitizeReturnUrl(String candidate) { + if (StringUtils.isBlank(candidate)) { + return CONTEXT_PATH; + } + + String lowercaseCandidate = candidate.toLowerCase(Locale.ROOT); + + // Backslashes can be interpreted as slashes by some clients. + // Also reject encoded backslashes and CR/LF characters. + if (candidate.contains("\\") + || lowercaseCandidate.contains("%5c") + || lowercaseCandidate.contains("%0d") + || lowercaseCandidate.contains("%0a")) { + return CONTEXT_PATH; + } + + try { + URI candidateUri = new URI(candidate); + + if (!candidateUri.isAbsolute()) { + return sanitizeRelativeUri(candidateUri); + } + + if (StringUtils.isBlank(externalUrl)) { + return CONTEXT_PATH; + } + URI configuredExternalUri = new URI(externalUrl); + + if (!isSameOrigin(candidateUri, configuredExternalUri)) { + return CONTEXT_PATH; + } + + /* + * Even for an accepted absolute same-origin URL, return only its + * path/query/fragment. This guarantees that Location never contains + * a user-controlled authority. + */ + return toRelativeLocation(candidateUri); + } catch (URISyntaxException | IllegalArgumentException e) { + return CONTEXT_PATH; + } + } + public Mono updateCookies(WebFilterExchange webFilterExchange, OAuth2AuthorizedClient oAuth2AuthorizedClient) { + ServerWebExchange exchange = webFilterExchange.getExchange(); + + exchange.getResponse().addCookie(defaultCookieAttr(ResponseCookie.from(COOKIE_AUTH_NAME, oAuth2AuthorizedClient.getAccessToken().getTokenValue())).build()); + + HttpCookie locationCookie = exchange.getRequest().getCookies().getFirst(COOKIE_RETURN_URL); + + String location = HAS_NO_VALUE.test(locationCookie) + ? CONTEXT_PATH + : sanitizeReturnUrl(locationCookie.getValue()); + + redirect(exchange.getResponse(), location); clearCookies(webFilterExchange); + return Mono.empty(); } @@ -468,7 +594,7 @@ public Mono saveAuthorizationRequest(OAuth2AuthorizationRequest authorizat exchange.getResponse().addCookie( createCookie(COOKIE_NONCE, String.valueOf(authorizationRequest.getAttributes().get(OidcParameterNames.NONCE))) ); - exchange.getResponse().addCookie(createCookie(COOKIE_RETURN_URL, getReturnUrl(exchange))); + exchange.getResponse().addCookie(createCookie(COOKIE_RETURN_URL, getSafeReturnUrl(exchange))); exchange.getResponse().addCookie(createCookie(COOKIE_STATE, authorizationRequest.getState())); return Mono.empty(); } @@ -478,6 +604,12 @@ String getReturnUrl(ServerWebExchange exchange) { .orElse(exchange.getRequest().getHeaders().getFirst(HttpHeaders.ORIGIN)); } + // Only a same-origin relative path or an absolute URL matching apiml.service.externalUrl is trusted; + // anything else falls back to the gateway's own root to prevent open-redirect/phishing via returnUrl. + private String getSafeReturnUrl(ServerWebExchange exchange) { + return sanitizeReturnUrl(getReturnUrl(exchange)); + } + @Override public Mono removeAuthorizationRequest(ServerWebExchange exchange) { Mono requestMono = loadAuthorizationRequest(exchange); diff --git a/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java b/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java index b578ff2e4f..51c5db21de 100644 --- a/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java +++ b/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java @@ -15,7 +15,9 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.beans.factory.annotation.Autowired; @@ -64,6 +66,7 @@ import java.util.HashMap; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -111,7 +114,7 @@ void shouldAddRegistryAuthorityToAllowedUser() { @Test void whenAccessTokenIsAvailable_thenAddItAsCookie() { var token = "thisIsToken"; - var location = "localhost:10010"; + var location = "/gateway/some/path"; var webFilterExchange = mock(WebFilterExchange.class); var exchange = mock(ServerWebExchange.class); when(webFilterExchange.getExchange()).thenReturn(exchange); @@ -491,6 +494,106 @@ void saveAuthorizationRequest_whenCalled_shouldSaveCookies () { } + @Nested + class SanitizeReturnUrl { + + private static final String EXTERNAL_URL = "https://gateway.zowe.example:10010"; + + WebSecurity.ApimlServerAuthorizationRequestRepository requestRepository; + + @BeforeEach + void setUp() { + var webSecurity = new WebSecurity(null, tokenProvider, basicAuthProvider, applicationContext); + ReflectionTestUtils.setField(webSecurity, "externalUrl", EXTERNAL_URL); + var resolver = mock(ServerOAuth2AuthorizationRequestResolver.class); + requestRepository = webSecurity.new ApimlServerAuthorizationRequestRepository(resolver); + } + + private String sanitize(String returnUrlQueryParam, String originHeader) { + var request = mock(ServerHttpRequest.class); + var headers = new HttpHeaders(); + if (originHeader != null) { + headers.set(HttpHeaders.ORIGIN, originHeader); + } + when(request.getHeaders()).thenReturn(headers); + + var queryParams = new LinkedMultiValueMap(); + if (returnUrlQueryParam != null) { + queryParams.add("returnUrl", returnUrlQueryParam); + } + when(request.getQueryParams()).thenReturn(queryParams); + + var exchange = mock(ServerWebExchange.class); + when(exchange.getRequest()).thenReturn(request); + var response = mock(ServerHttpResponse.class); + when(exchange.getResponse()).thenReturn(response); + + var cookies = new LinkedMultiValueMap(); + doAnswer(invocation -> { + var cookie = invocation.getArgument(0, ResponseCookie.class); + cookies.add(cookie.getName(), cookie); + return null; + }).when(response).addCookie(any(ResponseCookie.class)); + + var oauth2AuthReq = OAuth2AuthorizationRequest.authorizationCode() + .clientId("test-client") + .state("test-state") + .authorizationUri("https://test.auth.server/oauth/authorize") + .attributes(attrs -> attrs.put(OidcParameterNames.NONCE, "test-nonce")) + .build(); + + requestRepository.saveAuthorizationRequest(oauth2AuthReq, exchange).block(); + + return cookies.getFirst(WebSecurity.COOKIE_RETURN_URL).getValue(); + } + + static Stream safeAndUnsafeReturnUrls() { + return Stream.of( + // no returnUrl and no Origin header at all + Arguments.of(null, WebSecurity.CONTEXT_PATH), + Arguments.of("", WebSecurity.CONTEXT_PATH), + // plain relative paths are passed through untouched + Arguments.of("/gateway/foo", "/gateway/foo"), + Arguments.of("/gateway/foo?bar=1#frag", "/gateway/foo?bar=1#frag"), + // protocol-relative and backslash tricks that resolve to a different host + Arguments.of("//attacker.example/x", WebSecurity.CONTEXT_PATH), + Arguments.of("/\\attacker.example", WebSecurity.CONTEXT_PATH), + Arguments.of("/foo%5Cattacker.example", WebSecurity.CONTEXT_PATH), + Arguments.of("/foo%0D%0ASet-Cookie:evil=1", WebSecurity.CONTEXT_PATH), + // absolute URLs matching the configured externalUrl are accepted, authority stripped + Arguments.of(EXTERNAL_URL + "/gateway/foo?x=1#frag", "/gateway/foo?x=1#frag"), + Arguments.of(EXTERNAL_URL, "/"), + Arguments.of("HTTPS://GATEWAY.ZOWE.EXAMPLE:10010/gateway/foo", "/gateway/foo"), + // absolute URLs that differ from externalUrl in host, port, scheme or userinfo + Arguments.of("https://attacker.example/x", WebSecurity.CONTEXT_PATH), + Arguments.of("https://gateway.zowe.example:9999/x", WebSecurity.CONTEXT_PATH), + Arguments.of("http://gateway.zowe.example:10010/x", WebSecurity.CONTEXT_PATH), + Arguments.of("https://user@gateway.zowe.example:10010/x", WebSecurity.CONTEXT_PATH), + // non-http(s) schemes and malformed URIs + Arguments.of("javascript:alert(1)", WebSecurity.CONTEXT_PATH), + Arguments.of("mailto:foo@bar.com", WebSecurity.CONTEXT_PATH), + Arguments.of("http://exa mple.com", WebSecurity.CONTEXT_PATH) + ); + } + + @ParameterizedTest + @MethodSource("safeAndUnsafeReturnUrls") + void sanitizesReturnUrlQueryParam(String returnUrl, String expected) { + assertThat(sanitize(returnUrl, null)).isEqualTo(expected); + } + + @Test + void whenNoReturnUrlParam_thenFallsBackToOriginHeader_sameOrigin() { + assertThat(sanitize(null, EXTERNAL_URL)).isEqualTo("/"); + } + + @Test + void whenNoReturnUrlParam_thenFallsBackToOriginHeader_crossOrigin() { + assertThat(sanitize(null, "https://attacker.example")).isEqualTo(WebSecurity.CONTEXT_PATH); + } + + } + @Nested @ExtendWith(MockitoExtension.class) class ApimlStrictServerWebExchangeFirewall { From 0b31760e8237198009b4063852e904c8030ad78c Mon Sep 17 00:00:00 2001 From: Andrea Tabone Date: Tue, 14 Jul 2026 15:40:40 +0200 Subject: [PATCH 2/2] tests polishing Signed-off-by: Andrea Tabone --- .../apiml/gateway/config/WebSecurityTest.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java b/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java index 51c5db21de..8eca59c306 100644 --- a/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java +++ b/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java @@ -64,6 +64,7 @@ import java.net.InetSocketAddress; import java.util.HashMap; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Stream; @@ -133,9 +134,9 @@ void whenAccessTokenIsAvailable_thenAddItAsCookie() { when(accessToken.getTokenValue()).thenReturn(token); webSecurity.updateCookies(webFilterExchange, oAuth2AuthorizedClient); - assertEquals(token, serverHttpResponse.getCookies().getFirst("apimlAuthenticationToken").getValue()); - assertEquals("", serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_NONCE).getValue()); - assertEquals("", serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_STATE).getValue()); + assertEquals(token, Objects.requireNonNull(serverHttpResponse.getCookies().getFirst("apimlAuthenticationToken")).getValue()); + assertEquals("", Objects.requireNonNull(serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_NONCE)).getValue()); + assertEquals("", Objects.requireNonNull(serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_STATE)).getValue()); assertEquals(location, serverHttpResponse.getHeaders().getFirst(HttpHeaders.LOCATION)); } @@ -246,8 +247,6 @@ void oauth2WebFilterChain_whenClientConfigurationConfigured_shouldConfigureSecur ServerHttpSecurity.AuthorizeExchangeSpec authorizeExchangeSpec = mock(ServerHttpSecurity.AuthorizeExchangeSpec.class); ServerHttpSecurity.AuthorizeExchangeSpec.Access access = mock(ServerHttpSecurity.AuthorizeExchangeSpec.Access.class); - ServerHttpSecurity.OAuth2LoginSpec oauth2LoginSpec = mock(ServerHttpSecurity.OAuth2LoginSpec.class); - ServerHttpSecurity.HeaderSpec headerSpec = mock(ServerHttpSecurity.HeaderSpec.class); when(http.headers(any())).thenReturn(http); when(http.securityContextRepository(any())).thenReturn(http); @@ -489,8 +488,8 @@ void saveAuthorizationRequest_whenCalled_shouldSaveCookies () { requestRepository.saveAuthorizationRequest(oauth2AuthReq, exchange).block(); - assertThat(cookies.getFirst(WebSecurity.COOKIE_NONCE).getValue()).isEqualTo("test-nonce"); - assertThat(cookies.getFirst(WebSecurity.COOKIE_STATE).getValue()).isEqualTo("test-state"); + assertThat(Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_NONCE)).getValue()).isEqualTo("test-nonce"); + assertThat(Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_STATE)).getValue()).isEqualTo("test-state"); } @@ -544,7 +543,7 @@ private String sanitize(String returnUrlQueryParam, String originHeader) { requestRepository.saveAuthorizationRequest(oauth2AuthReq, exchange).block(); - return cookies.getFirst(WebSecurity.COOKIE_RETURN_URL).getValue(); + return Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_RETURN_URL)).getValue(); } static Stream safeAndUnsafeReturnUrls() { @@ -603,7 +602,7 @@ class ApimlStrictServerWebExchangeFirewall { @Mock private StrictServerWebExchangeFirewall nonRoutingFirewall; private WebSecurity.ApimlStrictServerWebExchangeFirewall apimlStrictServerWebExchangeFirewall; - private ApplicationInfo applicationInfo = ApplicationInfo.builder().build(); + private final ApplicationInfo applicationInfo = ApplicationInfo.builder().build(); @BeforeEach void setUp() {