Skip to content

Commit

Permalink
fix(redirect): add redirect handling and validate url for 301 and 302…
Browse files Browse the repository at this point in the history 
… status codes
  • Loading branch information
@ciurescuraul
ciurescuraul committed Aug 24, 2023
1 parent 4e4e913 commit 3eaa719
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 153 deletions.
39 changes: 26 additions & 13 deletions orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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";
Expand All @@ -58,6 +64,7 @@ public class HttpClientUtils {

public HttpClientUtils(UserConfiguredUrlRestrictions userConfiguredUrlRestrictions) {
this.httpClient = create(userConfiguredUrlRestrictions.getHttpClientProperties());
this.urlRestrictions = userConfiguredUrlRestrictions;
}

private CloseableHttpClient create(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
Expand Down
168 changes: 28 additions & 140 deletions orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/util/HttpClientUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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());
Expand All @@ -121,68 +125,22 @@ public void testPermanentlyMovedWithRetryEnabled() throws IOException {
get(resource)
.willReturn(
aResponse()
.withStatus(301) // Moved Permanently
.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 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());
Expand All @@ -193,109 +151,39 @@ 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());
// Create the HttpClientUtils instance with the configuration
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));
});
}

}

0 comments on commit 3eaa719

Please sign in to comment.