fix: sanitize OIDC returnUrl to prevent open redirect

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<Object> 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<Object> 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<Void> 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<OAuth2AuthorizationRequest> removeAuthorizationRequest(ServerWebExchange exchange) {
             Mono<OAuth2AuthorizationRequest> requestMono = loadAuthorizationRequest(exchange);

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;
@@ -62,8 +64,10 @@
 
 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;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -111,7 +115,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);
@@ -130,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));
         }
 
@@ -243,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);
@@ -486,8 +488,108 @@ 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");
+
+    }
+
+    @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<String, String>();
+            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<String, ResponseCookie>();
+            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 Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_RETURN_URL)).getValue();
+        }
+
+        static Stream<Arguments> 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);
+        }
 
     }
 
@@ -500,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() {