From b34b257f925bde8bc1d25b7c38696329438725cf Mon Sep 17 00:00:00 2001 From: Krystian <24556350+ciurescuraul@users.noreply.github.com> Date: Tue, 5 Sep 2023 19:59:46 +0300 Subject: [PATCH] fix(redirect): Allow for redirects instead of throwing exception (#4498) * fix(redirect): Allow for redirects instead of throwing exception * fix(redirect): Allow for redirects instead of throwing exception * fix(redirect): add redirect handling and validate url for 301 and 302 status codes * fix(redirect): Add redirect handling and validate url --------- Co-authored-by: Jason --- orca-core/orca-core.gradle | 2 +- .../orca/pipeline/util/HttpClientUtils.java | 76 ++++++++++++++++--- .../pipeline/util/HttpClientUtilsTest.java | 70 +++++++++++++++++ 3 files changed, 138 insertions(+), 10 deletions(-) diff --git a/orca-core/orca-core.gradle b/orca-core/orca-core.gradle index 98b0796013..0c2629a9ea 100644 --- a/orca-core/orca-core.gradle +++ b/orca-core/orca-core.gradle @@ -62,7 +62,7 @@ dependencies { testImplementation(project(":orca-test")) testImplementation(project(":orca-test-groovy")) - testImplementation("com.github.tomakehurst:wiremock:2.15.0") + testImplementation("com.github.tomakehurst:wiremock:2.17.0") testImplementation("org.junit.jupiter:junit-jupiter-api") testImplementation("org.assertj:assertj-core") testImplementation("org.mockito:mockito-core") 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 33e6e612b7..0b6b6e71e6 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 @@ -26,8 +26,11 @@ import java.util.List; import java.util.concurrent.TimeUnit; 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; import org.apache.http.client.methods.CloseableHttpResponse; @@ -36,6 +39,7 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.impl.client.HttpClients; +import org.apache.http.impl.client.LaxRedirectStrategy; import org.apache.http.protocol.HttpContext; import org.apache.http.protocol.HttpCoreContext; import org.slf4j.Logger; @@ -46,9 +50,11 @@ 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"; + private static final String LOCATION_HEADER = "Location"; private static List RETRYABLE_500_HTTP_STATUS_CODES = Arrays.asList( HttpStatus.SC_SERVICE_UNAVAILABLE, @@ -57,6 +63,7 @@ public class HttpClientUtils { public HttpClientUtils(UserConfiguredUrlRestrictions userConfiguredUrlRestrictions) { this.httpClient = create(userConfiguredUrlRestrictions.getHttpClientProperties()); + this.urlRestrictions = userConfiguredUrlRestrictions; } private CloseableHttpClient create( @@ -82,11 +89,30 @@ public boolean retryRequest( (statusCode == 429 || RETRYABLE_500_HTTP_STATUS_CODES.contains(statusCode)) && executionCount <= httpClientProperties.getMaxRetryAttempts(); - if ((statusCode >= 300) && (statusCode <= 399)) { - throw new RetryRequestException( - String.format( - "Attempted redirect from %s to %s which is not supported", - currentReq.getURI(), response.getFirstHeader("LOCATION").getValue())); + try { + RedirectStrategy laxRedirectStrategy = + new CustomRedirectStrategy().getLaxRedirectStrategy(); + + boolean isRedirected = + 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(), redirectLocation); + + httpClientBuilder.setRedirectStrategy(laxRedirectStrategy).build(); + return false; // Don't allow retry for redirection + } + } catch (ProtocolException protocolException) { + LOGGER.error( + "Failed redirect from {} to {}. Error: {}", + currentReq.getRequestLine().getUri(), + response.getFirstHeader(LOCATION_HEADER).getValue(), + protocolException.getMessage()); } if (!shouldRetry) { throw new RetryRequestException( @@ -130,10 +156,7 @@ public long getRetryInterval() { String proxyHostname = System.getProperty(JVM_HTTP_PROXY_HOST); if (proxyHostname != null) { - int proxyPort = - System.getProperty(JVM_HTTP_PROXY_PORT) != null - ? Integer.parseInt(System.getProperty(JVM_HTTP_PROXY_PORT)) - : 8080; + int proxyPort = getProxyPort(); LOGGER.info( "Found system properties for proxy configuration. Setting up http client to use proxy with " + "hostname {} and port {}", @@ -160,9 +183,44 @@ public String httpGetAsString(String url) throws IOException { } } + private int getProxyPort() { + String proxyPort = System.getProperty(JVM_HTTP_PROXY_PORT); + int defaultProxyPort = 8080; + + if (proxyPort != null) { + try { + return Integer.parseInt(proxyPort); + } catch (NumberFormatException e) { + LOGGER.warn( + "Invalid proxy port number: {}. Using default port: {}", proxyPort, defaultProxyPort); + } + } + return defaultProxyPort; + } + class RetryRequestException extends RuntimeException { RetryRequestException(String cause) { super(cause); } } + + class CustomRedirectStrategy { + public RedirectStrategy getLaxRedirectStrategy() { + return new LaxRedirectStrategy() { + @Override + public boolean isRedirected( + HttpRequest request, HttpResponse response, HttpContext context) { + int statusCode = response.getStatusLine().getStatusCode(); + + if (statusCode == HttpStatus.SC_MOVED_PERMANENTLY + || statusCode == HttpStatus.SC_MOVED_TEMPORARILY) { + + return response.getFirstHeader(LOCATION_HEADER).getValue() != null + && !response.getFirstHeader(LOCATION_HEADER).getValue().isEmpty(); + } + return false; + } + }; + } + } } 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 4e8511345f..8587e1ce71 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 @@ -18,6 +18,8 @@ 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; @@ -26,12 +28,15 @@ 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; public class HttpClientUtilsTest { private final String host = "localhost"; private final int port = 8080; private final String resource = "/v1/text"; + private final String redirectUrl = "https://spinnaker.io/"; private final UserConfiguredUrlRestrictions.Builder config = new UserConfiguredUrlRestrictions.Builder(); @@ -105,4 +110,69 @@ public void testTwoRetryForSocketException() { // then: verify(3, getRequestedFor(urlEqualTo(resource))); } + + @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()); + // Create the HttpClientUtils instance with the configuration + HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); + + stubFor( + get(resource) + .willReturn(aResponse().withStatus(httpStatus).withHeader("Location", redirectUrl))); + + // when: + String response = + httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); + + // then: + assertNotNull(response); + verify(1, getRequestedFor(urlEqualTo(resource))); + } + + @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()); + // Create the HttpClientUtils instance with the configuration + HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); + + stubFor( + get(resource) + .willReturn(aResponse().withStatus(httpStatus).withHeader("Location", redirectUrl))); + + // when: + String response = + httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); + + // then: + assertNotNull(response); + verify(1, getRequestedFor(urlEqualTo(resource))); + } + + @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()); + // Create the HttpClientUtils instance with the configuration + HttpClientUtils httpClientUtils = new HttpClientUtils(config.build()); + + stubFor( + get(invalidUrl) + .willReturn(aResponse().withStatus(301).withHeader("Location", redirectUrl))); + + // when & then + assertThrows( + HttpClientUtils.RetryRequestException.class, + () -> { + httpClientUtils.httpGetAsString(String.format("http://%s:%s%s", host, port, resource)); + }); + } }