Skip to content

Commit

Permalink
fix(redirect): Allow for redirects instead of throwing exception (#4498)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@ciurescuraul @jasonmcintosh
ciurescuraul and jasonmcintosh authored Sep 5, 2023
1 parent 27b697e commit b34b257
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 10 deletions.
2 changes: 1 addition & 1 deletion orca-core/orca-core.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
76 changes: 67 additions & 9 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 @@ -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;
Expand All @@ -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;
Expand All @@ -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<Integer> RETRYABLE_500_HTTP_STATUS_CODES =
Arrays.asList(
HttpStatus.SC_SERVICE_UNAVAILABLE,
Expand All @@ -57,6 +63,7 @@ public class HttpClientUtils {

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

private CloseableHttpClient create(
Expand All @@ -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(
Expand Down Expand Up @@ -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 {}",
Expand All @@ -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;
}
};
}
}
}
70 changes: 70 additions & 0 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 @@ -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;
Expand All @@ -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();

Expand Down Expand Up @@ -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));
});
}
}

0 comments on commit b34b257

Please sign in to comment.