diff --git a/modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/apache/client5/ApacheHttpComponents5FlowableHttpClient.java b/modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/apache/client5/ApacheHttpComponents5FlowableHttpClient.java index 5fcd8cde251..ef899772774 100644 --- a/modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/apache/client5/ApacheHttpComponents5FlowableHttpClient.java +++ b/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); + } + } } diff --git a/modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/spring/reactive/SpringWebClientFlowableHttpClient.java b/modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/spring/reactive/SpringWebClientFlowableHttpClient.java index 27b7abc15f8..be770075e2c 100644 --- a/modules/flowable-http-common/src/main/java/org/flowable/http/common/impl/spring/reactive/SpringWebClientFlowableHttpClient.java +++ b/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,9 +291,11 @@ 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 @@ -284,6 +303,7 @@ public CompletableFuture callAsync() { return request .exchangeToMono(response -> response.toEntity(ByteArrayResource.class)) .map(SpringWebClientFlowableHttpClient.this::toFlowableHttpResponse) + .contextWrite(Context.of(FOLLOW_REDIRECT_CONTEXT_KEY, followRedirect)) .toFuture(); } diff --git a/modules/flowable-http/src/test/java/org/flowable/http/FlowableHttpClientRedirectTest.java b/modules/flowable-http/src/test/java/org/flowable/http/FlowableHttpClientRedirectTest.java new file mode 100644 index 00000000000..6e73ff286db --- /dev/null +++ b/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="); + } + + @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="); + } + + @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 provideArguments(ParameterDeclarations parameters, ExtensionContext context) { + HttpClientConfig config = createClientConfig(); + return Stream.of( + Arguments.of(new SpringWebClientFlowableHttpClient(config)), + Arguments.of(new ApacheHttpComponents5FlowableHttpClient(config)) + ); + } + } +} diff --git a/modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.java b/modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.java index 900af52e586..e8608ded7c5 100644 --- a/modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.java +++ b/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 diff --git a/modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskTestServer.java b/modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskTestServer.java index b88b88ccab8..a10138ff3fe 100644 --- a/modules/flowable-http/src/test/java/org/flowable/http/bpmn/HttpServiceTaskTestServer.java +++ b/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=}, or + * {@code auth=} 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 : "")); + } + } + public static void setUp() { // No setup required } diff --git a/modules/flowable-http/src/test/resources/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.testHttpPost3XX.bpmn20.xml b/modules/flowable-http/src/test/resources/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.testHttpPost3XX.bpmn20.xml new file mode 100644 index 00000000000..14077cc9490 --- /dev/null +++ b/modules/flowable-http/src/test/resources/org/flowable/http/bpmn/HttpServiceTaskSpringWebClientTest.testHttpPost3XX.bpmn20.xml @@ -0,0 +1,67 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file