From 3eaa719208e107a21697ef67e097e3560c5e695d Mon Sep 17 00:00:00 2001 From: Raul Ciurescu <24556350+ciurescuraul@users.noreply.github.com> Date: Thu, 24 Aug 2023 18:44:39 +0300 Subject: [PATCH] fix(redirect): add redirect handling and validate url for 301 and 302 status codes --- .../orca/pipeline/util/HttpClientUtils.java | 39 ++-- .../pipeline/util/HttpClientUtilsTest.java | 168 +++--------------- 2 files changed, 54 insertions(+), 153 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtils.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtils.java index 0017f92474..385b92cda3 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtils.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtils.java @@ -19,13 +19,11 @@ import com.google.common.io.CharStreams; import com.google.common.util.concurrent.Uninterruptibles; import com.netflix.spinnaker.orca.config.UserConfiguredUrlRestrictions; -import java.io.IOException; -import java.io.InputStreamReader; -import java.io.Reader; -import java.util.Arrays; -import java.util.List; -import java.util.concurrent.TimeUnit; -import org.apache.http.*; +import org.apache.http.HttpHost; +import org.apache.http.HttpRequest; +import org.apache.http.HttpResponse; +import org.apache.http.HttpStatus; +import org.apache.http.ProtocolException; import org.apache.http.client.RedirectStrategy; import org.apache.http.client.ServiceUnavailableRetryStrategy; import org.apache.http.client.config.RequestConfig; @@ -42,10 +40,18 @@ import org.slf4j.LoggerFactory; import org.springframework.stereotype.Component; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.Reader; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.TimeUnit; + @Component public class HttpClientUtils { private CloseableHttpClient httpClient; + private final UserConfiguredUrlRestrictions urlRestrictions; private static final Logger LOGGER = LoggerFactory.getLogger(HttpClientUtils.class); private static final String JVM_HTTP_PROXY_HOST = "http.proxyHost"; private static final String JVM_HTTP_PROXY_PORT = "http.proxyPort"; @@ -58,6 +64,7 @@ public class HttpClientUtils { public HttpClientUtils(UserConfiguredUrlRestrictions userConfiguredUrlRestrictions) { this.httpClient = create(userConfiguredUrlRestrictions.getHttpClientProperties()); + this.urlRestrictions = userConfiguredUrlRestrictions; } private CloseableHttpClient create( @@ -91,10 +98,13 @@ public boolean retryRequest( laxRedirectStrategy.isRedirected(currentReq, response, context); if (isRedirected) { + // verify that we are not redirecting to a restricted url + String redirectLocation = response.getFirstHeader(LOCATION_HEADER).getValue(); + urlRestrictions.validateURI(redirectLocation); + LOGGER.info( "Attempt redirect from {} to {} ", - currentReq.getURI(), - response.getFirstHeader(LOCATION_HEADER).getValue()); + currentReq.getURI(), redirectLocation); httpClientBuilder.setRedirectStrategy(laxRedirectStrategy).build(); return false; // Don't allow retry for redirection @@ -202,11 +212,14 @@ class CustomRedirectStrategy { public RedirectStrategy getLaxRedirectStrategy() { return new LaxRedirectStrategy() { @Override - public boolean isRedirected( - HttpRequest request, HttpResponse response, HttpContext context) { - + public boolean isRedirected(HttpRequest request, + HttpResponse response, + HttpContext context) { int statusCode = response.getStatusLine().getStatusCode(); - if ((statusCode >= 300) && (statusCode <= 399)) { + + if (statusCode == HttpStatus.SC_MOVED_PERMANENTLY || + statusCode == HttpStatus.SC_MOVED_TEMPORARILY) { + return response.getFirstHeader(LOCATION_HEADER).getValue() != null && !response.getFirstHeader(LOCATION_HEADER).getValue().isEmpty(); } diff --git a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtilsTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtilsTest.java index 50dd2fc226..771c65b50c 100644 --- a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtilsTest.java +++ b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtilsTest.java @@ -16,18 +16,21 @@ package com.netflix.spinnaker.orca.pipeline.util; -import static com.github.tomakehurst.wiremock.client.WireMock.*; -import static org.assertj.core.api.Fail.fail; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; - import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.http.Fault; import com.netflix.spinnaker.orca.config.UserConfiguredUrlRestrictions; -import java.io.IOException; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.io.IOException; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.assertj.core.api.Fail.fail; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; public class HttpClientUtilsTest { @@ -109,8 +112,9 @@ public void testTwoRetryForSocketException() { verify(3, getRequestedFor(urlEqualTo(resource))); } - @Test - public void testPermanentlyMovedWithRetryEnabled() throws IOException { + @ParameterizedTest + @ValueSource(ints = {301, 302}) + public void testRedirectWithRetryEnabled(int httpStatus) throws IOException { // Set up the configuration to enable retry functionality config.setHttpClientProperties( UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(true).build()); @@ -121,7 +125,7 @@ public void testPermanentlyMovedWithRetryEnabled() throws IOException { get(resource) .willReturn( aResponse() - .withStatus(301) // Moved Permanently + .withStatus(httpStatus) .withHeader("Location", redirectUrl))); // when: @@ -129,60 +133,14 @@ public void testPermanentlyMovedWithRetryEnabled() throws IOException { httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); // then: - verify(1, getRequestedFor(urlEqualTo(resource))); assertNotNull(response); - } - - @Test - public void testPermanentlyMovedWithRetryDisabled() throws IOException { - // Set up the configuration to enable retry functionality - config.setHttpClientProperties( - UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(false).build()); - // Create the HttpClientUtils instance with the configuration - HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); - - stubFor( - get(resource) - .willReturn( - aResponse() - .withStatus(301) // Moved Permanently - .withHeader("Location", redirectUrl))); - - // when: - String response = - httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - - // then: verify(1, getRequestedFor(urlEqualTo(resource))); - assertNotNull(response); } - @Test - public void testTemporaryRedirectWithRetryEnabled() throws IOException { - // Set up the configuration to enable retry functionality - config.setHttpClientProperties( - UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(true).build()); - // Create the HttpClientUtils instance with the configuration - HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); - - stubFor( - get(resource) - .willReturn( - aResponse() - .withStatus(307) // Temporary Redirect - .withHeader("Location", redirectUrl))); - - // when: - String response = - httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - // then: - verify(1, getRequestedFor(urlEqualTo(resource))); - assertNotNull(response); - } - - @Test - public void testTemporaryRedirectWithRetryDisabled() throws IOException { + @ParameterizedTest + @ValueSource(ints = {301, 302}) + public void testRedirectWithRetryDisabled(int httpStatus) throws IOException { // Set up the configuration to enable retry functionality config.setHttpClientProperties( UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(false).build()); @@ -193,91 +151,22 @@ public void testTemporaryRedirectWithRetryDisabled() throws IOException { get(resource) .willReturn( aResponse() - .withStatus(307) // Temporary Redirect + .withStatus(httpStatus) .withHeader("Location", redirectUrl))); // when: String response = httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - // then: - verify(1, getRequestedFor(urlEqualTo(resource))); - assertNotNull(response); - } - - @Test - public void testTemporaryRedirectThrowsRetryRequestException() throws IOException { - // Set up the configuration to enable retry functionality - config.setHttpClientProperties( - UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(true).build()); - // Create the HttpClientUtils instance with the configuration - HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); - - stubFor( - get(resource) - .willReturn( - aResponse() - .withStatus(307) // Temporary Redirect - .withHeader("Location", ""))); // then: - assertThrows( - HttpClientUtils.RetryRequestException.class, - () -> { - httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - }); - } - - @Test - public void testPermanentRedirectWithRetryEnabled() throws IOException { - // Set up the configuration to enable retry functionality - config.setHttpClientProperties( - UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(true).build()); - // Create the HttpClientUtils instance with the configuration - HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); - - stubFor( - get(resource) - .willReturn( - aResponse() - .withStatus(308) // Permanent Redirect - .withHeader("Location", redirectUrl))); - - // when: - String response = - httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - - // then: - verify(1, getRequestedFor(urlEqualTo(resource))); assertNotNull(response); - } - - @Test - public void testPermanentRedirectWithRetryDisabled() throws IOException { - // Set up the configuration to enable retry functionality - config.setHttpClientProperties( - UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(false).build()); - // Create the HttpClientUtils instance with the configuration - HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); - - stubFor( - get(resource) - .willReturn( - aResponse() - .withStatus(308) // Permanent Redirect - .withHeader("Location", redirectUrl))); - - // when: - String response = - httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - - // then: verify(1, getRequestedFor(urlEqualTo(resource))); - assertNotNull(response); } - @Test - public void testPermanentRedirectThrowsRetryRequestException() throws IOException { + @ParameterizedTest + @ValueSource(strings = {"192.168.1.1", "127.0.0.1"}) + void testRedirectThrowsRetryRequestExceptionForInvalidUrl(String invalidUrl) { // Set up the configuration to enable retry functionality config.setHttpClientProperties( UserConfiguredUrlRestrictions.HttpClientProperties.builder().enableRetry(true).build()); @@ -285,17 +174,16 @@ public void testPermanentRedirectThrowsRetryRequestException() throws IOExceptio HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); stubFor( - get(resource) + get(invalidUrl) .willReturn( aResponse() - .withStatus(308) // Permanent Redirect - .withHeader("Location", ""))); + .withStatus(301) + .withHeader("Location", redirectUrl))); - // then: - assertThrows( - HttpClientUtils.RetryRequestException.class, - () -> { - httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); - }); + // when & then + assertThrows(HttpClientUtils.RetryRequestException.class, () -> { + httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); + }); } + }