Follow redirects based on the request and strip credentials on cross-…

modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/apache/client5/ApacheHttpComponents5FlowableHttpClient.java

@@ -32,10 +32,13 @@
 import org.apache.hc.client5.http.async.methods.SimpleBody;
 import org.apache.hc.client5.http.async.methods.SimpleHttpResponse;
 import org.apache.hc.client5.http.async.methods.SimpleResponseConsumer;
+import org.apache.hc.client5.http.SchemePortResolver;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.entity.mime.HttpMultipartMode;
 import org.apache.hc.client5.http.entity.mime.MultipartEntityBuilder;
 import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy;
+import org.apache.hc.client5.http.impl.DefaultRedirectStrategy;
+import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
 import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient;
 import org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder;
 import org.apache.hc.client5.http.impl.async.HttpAsyncClients;
@@ -46,7 +49,9 @@
 import org.apache.hc.client5.http.ssl.TrustSelfSignedStrategy;
 import org.apache.hc.core5.concurrent.FutureCallback;
 import org.apache.hc.core5.http.ContentType;
+import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpEntity;
+import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.HttpVersion;
 import org.apache.hc.core5.http.ParseException;
 import org.apache.hc.core5.http.io.entity.ByteArrayEntity;
@@ -55,6 +60,7 @@
 import org.apache.hc.core5.http.nio.AsyncRequestProducer;
 import org.apache.hc.core5.http.nio.entity.AsyncEntityProducers;
 import org.apache.hc.core5.http.nio.support.AsyncRequestBuilder;
+import org.apache.hc.core5.http.protocol.HttpContext;
 import org.apache.hc.core5.io.CloseMode;
 import org.apache.hc.core5.io.ModalCloseable;
 import org.apache.hc.core5.net.WWWFormCodec;
@@ -82,6 +88,8 @@ public class ApacheHttpComponents5FlowableHttpClient implements FlowableAsyncHtt
     private static final Pattern SPACE_CHARACTER_PATTERN = Pattern.compile(" ");
     private static final String ENCODED_SPACE_CHARACTER = "%20";
 
+    private static final String[] SENSITIVE_REDIRECT_HEADERS = { "Authorization", "Cookie", "Proxy-Authorization" };
+
     protected final Logger logger = LoggerFactory.getLogger(getClass());
 
     protected HttpAsyncClient client;
@@ -119,6 +127,11 @@ public ApacheHttpComponents5FlowableHttpClient(HttpClientConfig config, Consumer
             retryCount = config.getRequestRetryLimit();
         }
         httpClientBuilder.setRetryStrategy(new DefaultHttpRequestRetryStrategy(retryCount, TimeValue.ZERO_MILLISECONDS));
+
+        // By default HttpClient refuses to follow a redirect to a different host when the request carries credentials or
+        // cookies. Instead strip those sensitive headers on such cross-host redirects and follow the redirect, so that
+        // credentials are never leaked to the redirect target while still allowing the download to be retrieved.
+        httpClientBuilder.setRedirectStrategy(new SensitiveHeadersStrippingRedirectStrategy());
         clientBuilderCustomizer.accept(httpClientBuilder);
 
         // system settings
@@ -396,4 +409,42 @@ public void cancelled() {
             return responseFuture;
         }
     }
+
+    /**
+     * A {@link DefaultRedirectStrategy} that, instead of refusing to follow a redirect to a different authority when
+     * the request carries credentials or cookies (the default behaviour), strips those sensitive headers from the
+     * redirect request and allows it to be followed. This way the redirect is honoured without leaking credentials to
+     * the redirect target. The redirect request is copied by the exec chain after this check, so removing the headers
+     * here removes them from the request that is actually sent.
+     */
+    protected static class SensitiveHeadersStrippingRedirectStrategy extends DefaultRedirectStrategy {
+
+        protected final SchemePortResolver schemePortResolver = DefaultSchemePortResolver.INSTANCE;
+
+        @Override
+        public boolean isRedirectAllowed(HttpHost currentTarget, HttpHost newTarget,
+                org.apache.hc.core5.http.HttpRequest redirect, HttpContext context) {
+            if (!isSameAuthority(currentTarget, newTarget)) {
+                for (String header : SENSITIVE_REDIRECT_HEADERS) {
+                    redirect.removeHeaders(header);
+                }
+                for (Header header : redirect.getHeaders()) {
+                    if (header.isSensitive()) {
+                        redirect.removeHeaders(header.getName());
+                    }
+                }
+            }
+            return true;
+        }
+
+        protected boolean isSameAuthority(HttpHost first, HttpHost second) {
+            if (first == null || second == null) {
+                return false;
+            }
+            if (!first.getHostName().equalsIgnoreCase(second.getHostName())) {
+                return false;
+            }
+            return schemePortResolver.resolve(first) == schemePortResolver.resolve(second);
+        }
+    }
 }

modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/spring/reactive/SpringWebClientFlowableHttpClient.java

@@ -51,7 +51,10 @@
 import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
 import io.netty.handler.timeout.ReadTimeoutHandler;
 import reactor.netty.http.client.HttpClient;
+import reactor.netty.http.client.HttpClientRequest;
+import reactor.netty.http.client.HttpClientResponse;
 import reactor.netty.resources.ConnectionProvider;
+import reactor.util.context.Context;
 
 /**
  * @author Filip Hrisafov
@@ -63,6 +66,9 @@ public class SpringWebClientFlowableHttpClient implements FlowableAsyncHttpClien
     private static final Pattern SPACE_CHARACTER_PATTERN = Pattern.compile(" ");
     private static final String ENCODED_SPACE_CHARACTER = "%20";
 
+    // Class qualified to avoid collisions with other keys that might be present in the Reactor context.
+    private static final String FOLLOW_REDIRECT_CONTEXT_KEY = SpringWebClientFlowableHttpClient.class.getName() + ".followRedirect";
+
     protected final Logger logger = LoggerFactory.getLogger(getClass());
 
     protected final WebClient webClient;
@@ -73,7 +79,12 @@ public SpringWebClientFlowableHttpClient(HttpClientConfig config) {
                 .builder("flowableHttpClient")
                 .maxConnections(500)
                 .build())
-                .compress(true);
+                .compress(true)
+                // Whether a redirect is followed is decided per request through the Reactor context written in
+                // callAsync (populated from HttpRequest#isNoRedirects). Reactor Netty drops the sensitive headers
+                // (Authorization, Proxy-Authorization, ...) when a followed redirect targets a different domain, so
+                // credentials are not leaked to the redirect target.
+                .followRedirect(SpringWebClientFlowableHttpClient::shouldFollowRedirect);
 
         if (config.isDisableCertVerify()) {
             try {
@@ -155,12 +166,18 @@ public AsyncExecutableHttpRequest prepareRequest(HttpRequest requestInfo) {
             setHeaders(headersSpec, requestInfo.getHttpHeaders());
             setHeaders(headersSpec, requestInfo.getSecureHttpHeaders());
 
-            return new WebClientExecutableHttpRequest(headersSpec);
+            return new WebClientExecutableHttpRequest(headersSpec, !requestInfo.isNoRedirects());
         } catch (URISyntaxException ex) {
             throw new FlowableException("Invalid URL exception occurred", ex);
         }
     }
 
+    protected static boolean shouldFollowRedirect(HttpClientRequest request, HttpClientResponse response) {
+        boolean followRedirect = response.currentContextView().getOrDefault(FOLLOW_REDIRECT_CONTEXT_KEY, Boolean.FALSE);
+        int statusCode = response.status().code();
+        return followRedirect && statusCode >= 300 && statusCode < 400;
+    }
+
     protected WebClient determineWebClient(HttpRequest requestInfo) {
         if (requestInfo.getTimeout() <= 0) {
             return webClient;
@@ -274,16 +291,19 @@ protected org.flowable.http.common.api.HttpHeaders toFlowableHeaders(HttpHeaders
     protected class WebClientExecutableHttpRequest implements AsyncExecutableHttpRequest {
 
         protected final WebClient.RequestHeadersSpec<?> request;
+        protected final boolean followRedirect;
 
-        public WebClientExecutableHttpRequest(WebClient.RequestHeadersSpec<?> request) {
+        public WebClientExecutableHttpRequest(WebClient.RequestHeadersSpec<?> request, boolean followRedirect) {
             this.request = request;
+            this.followRedirect = followRedirect;
         }
 
         @Override
         public CompletableFuture<HttpResponse> callAsync() {
             return request
                     .exchangeToMono(response -> response.toEntity(ByteArrayResource.class))
                     .map(SpringWebClientFlowableHttpClient.this::toFlowableHttpResponse)
+                    .contextWrite(Context.of(FOLLOW_REDIRECT_CONTEXT_KEY, followRedirect))
                     .toFuture();
         }
 

modules/flowable-http/src/test/java/org/flowable/http/FlowableHttpClientRedirectTest.java

@@ -0,0 +1,141 @@
+/* Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.flowable.http;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.time.Duration;
+import java.util.stream.Stream;
+
+import org.flowable.http.bpmn.HttpServiceTaskTestServer;
+import org.flowable.http.common.api.HttpHeaders;
+import org.flowable.http.common.api.HttpRequest;
+import org.flowable.http.common.api.HttpResponse;
+import org.flowable.http.common.api.client.FlowableHttpClient;
+import org.flowable.http.common.impl.HttpClientConfig;
+import org.flowable.http.common.impl.apache.client5.ApacheHttpComponents5FlowableHttpClient;
+import org.flowable.http.common.impl.spring.reactive.SpringWebClientFlowableHttpClient;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.ArgumentsProvider;
+import org.junit.jupiter.params.provider.ArgumentsSource;
+import org.junit.jupiter.params.support.ParameterDeclarations;
+
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+/**
+ * Tests that the HTTP clients follow redirects driven by {@link HttpRequest#isNoRedirects()} and do not leak
+ * credentials to a different host when a redirect crosses hosts. Both hosts are served by the same test server, reached
+ * as {@code localhost} vs {@code 127.0.0.1} to simulate a cross-host redirect.
+ */
+class FlowableHttpClientRedirectTest {
+
+    protected static final String LOCALHOST_BASE = "http://localhost:9798";
+    protected static final String LOOPBACK_IP_BASE = "http://127.0.0.1:9798";
+
+    @BeforeEach
+    void setUp() {
+        HttpServiceTaskTestServer.setUp();
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(FlowableHttpClientArgumentProvider.class)
+    void followsRedirectByDefault(FlowableHttpClient httpClient) {
+        HttpRequest request = new HttpRequest();
+        request.setMethod("GET");
+        request.setUrl(LOCALHOST_BASE + "/redirect?location=" + LOCALHOST_BASE + "/echo-authorization");
+
+        HttpResponse response = httpClient.prepareRequest(request).call();
+
+        assertThat(response.getStatusCode()).isEqualTo(200);
+        assertThat(response.getBody()).isEqualTo("auth=<none>");
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(FlowableHttpClientArgumentProvider.class)
+    void doesNotFollowRedirectWhenNoRedirects(FlowableHttpClient httpClient) {
+        HttpRequest request = new HttpRequest();
+        request.setMethod("GET");
+        request.setUrl(LOCALHOST_BASE + "/redirect?location=" + LOCALHOST_BASE + "/echo-authorization");
+        request.setNoRedirects(true);
+
+        HttpResponse response = httpClient.prepareRequest(request).call();
+
+        assertThat(response.getStatusCode()).isEqualTo(302);
+        assertThat(response.getHttpHeaders().get("Location"))
+                .containsExactly(LOCALHOST_BASE + "/echo-authorization");
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(RedirectSafeFlowableHttpClientArgumentProvider.class)
+    void doesNotForwardCredentialsOnCrossHostRedirect(FlowableHttpClient httpClient) {
+        HttpRequest request = new HttpRequest();
+        request.setMethod("GET");
+        request.setUrl(LOCALHOST_BASE + "/redirect?location=" + LOOPBACK_IP_BASE + "/echo-authorization");
+        HttpHeaders secureHeaders = new HttpHeaders();
+        secureHeaders.add("Authorization", "Bearer test-token");
+        request.setSecureHttpHeaders(secureHeaders);
+
+        HttpResponse response = httpClient.prepareRequest(request).call();
+
+        assertThat(response.getStatusCode()).isEqualTo(200);
+        assertThat(response.getBody())
+                .as("Authorization must not be forwarded to a different host on redirect")
+                .isEqualTo("auth=<none>");
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(RedirectSafeFlowableHttpClientArgumentProvider.class)
+    void forwardsCredentialsOnSameHostRedirect(FlowableHttpClient httpClient) {
+        HttpRequest request = new HttpRequest();
+        request.setMethod("GET");
+        request.setUrl(LOCALHOST_BASE + "/redirect?location=" + LOCALHOST_BASE + "/echo-authorization");
+        HttpHeaders secureHeaders = new HttpHeaders();
+        secureHeaders.add("Authorization", "Bearer test-token");
+        request.setSecureHttpHeaders(secureHeaders);
+
+        HttpResponse response = httpClient.prepareRequest(request).call();
+
+        assertThat(response.getStatusCode()).isEqualTo(200);
+        assertThat(response.getBody())
+                .as("Authorization may be forwarded when the redirect stays on the same host")
+                .isEqualTo("auth=Bearer test-token");
+    }
+
+    protected static HttpClientConfig createClientConfig() {
+        HttpClientConfig config = new HttpClientConfig();
+        config.setConnectTimeout(Duration.ofSeconds(5));
+        config.setSocketTimeout(Duration.ofSeconds(5));
+        config.setConnectionRequestTimeout(Duration.ofSeconds(5));
+        config.setRequestRetryLimit(5);
+        config.setDisableCertVerify(true);
+        return config;
+    }
+
+    /**
+     * Only the clients that strip credentials on cross-host redirects: the Apache HttpClient 5 and the Spring WebClient
+     * based clients. The legacy Apache HttpClient 4 based client is intentionally not covered.
+     */
+    public static class RedirectSafeFlowableHttpClientArgumentProvider implements ArgumentsProvider {
+
+        @Override
+        public Stream<? extends Arguments> provideArguments(ParameterDeclarations parameters, ExtensionContext context) {
+            HttpClientConfig config = createClientConfig();
+            return Stream.of(
+                    Arguments.of(new SpringWebClientFlowableHttpClient(config)),
+                    Arguments.of(new ApacheHttpComponents5FlowableHttpClient(config))
+            );
+        }
+    }
+}

modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.java

@@ -37,6 +37,15 @@ public void testMapException() {
         super.testMapException();
     }
 
+    @Test
+    @Deployment
+    @Override
+    // We override because we have a different BPMN XML: unlike the default Apache HttpClient 4 based client, the Spring
+    // WebClient based client follows POST redirects, so the redirect has to be explicitly disallowed to observe the 302.
+    public void testHttpPost3XX() {
+        super.testHttpPost3XX();
+    }
+
     @Test
     @Deployment(resources = "org/flowable/http/bpmn/HttpServiceTaskTest.testConnectTimeout.bpmn20.xml")
     @Override

modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskTestServer.java

@@ -109,6 +109,8 @@ public class HttpServiceTaskTestServer {
             contextHandler.addServlet(new ServletHolder(new DeleteResponseServlet()), "/delete");
             contextHandler.addServlet(new ServletHolder(new ClasspathResourceServlet()), "/resource");
             contextHandler.addServlet(new ServletHolder(new BinaryEchoServlet()), "/binary");
+            contextHandler.addServlet(new ServletHolder(new RedirectServlet()), "/redirect");
+            contextHandler.addServlet(new ServletHolder(new EchoAuthorizationServlet()), "/echo-authorization");
             server.setHandler(contextHandler);
             server.start();
         } catch (Exception e) {
@@ -388,6 +390,37 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
         }
     }
 
+    /**
+     * Answers with a redirect (302 by default) to the URL given in the {@code location} query parameter. Used to test
+     * client-side redirect following without depending on an external host.
+     */
+    protected static class RedirectServlet extends HttpServlet {
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
+            String location = req.getParameter("location");
+            String statusParam = req.getParameter("status");
+            int status = StringUtils.isNotEmpty(statusParam) ? Integer.parseInt(statusParam) : HttpServletResponse.SC_FOUND;
+            resp.setStatus(status);
+            resp.setHeader("Location", location);
+        }
+    }
+
+    /**
+     * Echoes the received {@code Authorization} header back in the response body (as {@code auth=<value>}, or
+     * {@code auth=<none>} when absent). Used to verify whether credentials are forwarded across a redirect.
+     */
+    protected static class EchoAuthorizationServlet extends HttpServlet {
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+            String authorization = req.getHeader("Authorization");
+            resp.setStatus(200);
+            resp.setContentType("text/plain");
+            resp.getWriter().print("auth=" + (authorization != null ? authorization : "<none>"));
+        }
+    }
+
     public static void setUp() {
         // No setup required
     }