Skip to content

Commit

Permalink
refactor(webhook): use exceptions to communicate invalid request/resp…
Browse files Browse the repository at this point in the history 
…onse size

instead of cancelling the call.  The resulting exceptions from canceling were different
locally and in CI, and I suspect throwing our exceptions makes it easier to provide better
error messages to the user.
  • Loading branch information
@dbyron-sf
dbyron-sf committed Feb 15, 2025
1 parent 2b5f3f6 commit 182b4c6
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 68 deletions.
101 changes: 47 additions & 54 deletions ...webhook/src/main/java/com/netflix/spinnaker/orca/webhook/config/WebhookConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,11 @@ public ClientHttpRequestFactory webhookRequestFactory(
.addNetworkInterceptor(
chain -> {
Request request = chain.request();
if (!validRequestSize(request, webhookProperties.getMaxRequestBytes())) {
chain.call().cancel();
}
validateRequestSize(request, webhookProperties.getMaxRequestBytes());

Response response = chain.proceed(request);

if (!validResponseSize(response, webhookProperties.getMaxResponseBytes())) {
chain.call().cancel();
}
validateResponseSize(response, webhookProperties.getMaxResponseBytes());

if (webhookProperties.isVerifyRedirects() && response.isRedirect()) {
// verify that we are not redirecting to a restricted url
Expand All @@ -142,27 +138,27 @@ public ClientHttpRequestFactory webhookRequestFactory(
return requestFactory;
}

/** Return true if the request size is valid, false otherwise. */
private boolean validRequestSize(Request request, long maxRequestBytes) throws IOException {
/** Return if the request size is valid. Throw an exception otherwise. */
private void validateRequestSize(Request request, long maxRequestBytes) throws IOException {
if (maxRequestBytes <= 0L) {
return true;
return;
}

long headerSize = request.headers().byteCount();

// If the headers are already too big, no need to deal with the body size
if (headerSize > maxRequestBytes) {
log.info(
"rejecting request to {} with {} byte(s) of headers, maxRequestBytes: {}",
request.url(),
headerSize,
maxRequestBytes);
return false;
String message =
String.format(
"rejecting request to %s with %s byte(s) of headers, maxRequestBytes: %s",
request.url(), headerSize, maxRequestBytes);
log.info(message);
throw new IllegalArgumentException(message);
}

RequestBody requestBody = request.body();
if (requestBody == null) {
return true;
return;
}

long contentLength = 0L;
Expand All @@ -173,10 +169,12 @@ private boolean validRequestSize(Request request, long maxRequestBytes) throws I
// OkHttp3ClientRequest.executeInternal) has a byte array argument, this
// doesn't seem possible. Reject the request in case it ends up being
// too big. Yes, this is paranoid, but seems safer.
log.error(
"unable to verify size of body in request to {}, content length not set",
request.url());
return false;
String message =
String.format(
"unable to verify size of body in request to %s, content length not set",
request.url());
log.error(message);
throw new IllegalStateException(message);
}
} catch (IOException e) {
log.error("exception retrieving content length of request to {}", request.url(), e);
Expand All @@ -185,38 +183,36 @@ private boolean validRequestSize(Request request, long maxRequestBytes) throws I

long totalSize = headerSize + contentLength;
if (totalSize > maxRequestBytes) {
log.info(
"rejecting {} byte request to {}, maxRequestBytes: {}",
totalSize,
request.url(),
maxRequestBytes);
return false;
String message =
String.format(
"rejecting request to %s with %s byte body, maxRequestBytes: {}",
request.url(), totalSize, maxRequestBytes);
log.info(message);
throw new IllegalArgumentException(message);
}

return true;
}

/** Return true if the response size is valid, false otherwise. */
private boolean validResponseSize(Response response, long maxResponseBytes) throws IOException {
/** Return if the response size is valid. Throw an exception otherwise. */
private void validateResponseSize(Response response, long maxResponseBytes) throws IOException {
if (maxResponseBytes <= 0L) {
return true;
return;
}

long headerSize = response.headers().byteCount();

// If the headers are already too big, no need to deal with the body size
if (headerSize > maxResponseBytes) {
log.info(
"rejecting response from {} with {} byte(s) of headers, maxResponseBytes: {}",
response.request().url(),
headerSize,
maxResponseBytes);
return false;
String message =
String.format(
"rejecting response from %s with %s byte(s) of headers, maxResponseBytes: %s",
response.request().url(), headerSize, maxResponseBytes);
log.info(message);
throw new IllegalArgumentException(message);
}

ResponseBody responseBody = response.body();
if (responseBody == null) {
return true;
return;
}

long contentLength = responseBody.contentLength();
Expand All @@ -235,18 +231,17 @@ private boolean validResponseSize(Response response, long maxResponseBytes) thro
// Request one more than the number of allowed bytes remaining. If that
// succeeds, the response is too long.
if (peeked.request(remainingAllowedBytes + 1)) {
log.info(
"rejecting response from {}. headers: {} byte(s), body > {} byte(s), maxResponseBytes: {}",
response.request().url(),
headerSize,
remainingAllowedBytes,
maxResponseBytes);
return false;
String message =
String.format(
"rejecting response from %s. headers: %s byte(s), body > %s byte(s), maxResponseBytes: %s",
response.request().url(), headerSize, remainingAllowedBytes, maxResponseBytes);
log.info(message);
throw new IllegalArgumentException(message);
}

// There aren't enough bytes in the response to satisfy the peek
// request. In other words, the response is short enough to be valid.
return true;
return;
} catch (IOException e) {
log.error("exception peeking into response from {}", response.request().url(), e);
throw e;
Expand All @@ -255,15 +250,13 @@ private boolean validResponseSize(Response response, long maxResponseBytes) thro

long totalSize = headerSize + contentLength;
if (totalSize > maxResponseBytes) {
log.info(
"rejecting {} byte response from {}, maxResponseBytes: {}",
totalSize,
response.request().url(),
maxResponseBytes);
return false;
String message =
String.format(
"rejecting %s byte response from %s, maxResponseBytes: %s",
totalSize, response.request().url(), maxResponseBytes);
log.info(message);
throw new IllegalArgumentException(message);
}

return true;
}

private X509TrustManager webhookX509TrustManager() {
Expand Down
22 changes: 8 additions & 14 deletions ...-webhook/src/test/java/com/netflix/spinnaker/orca/webhook/service/WebhookServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl;
import com.netflix.spinnaker.orca.webhook.config.WebhookConfiguration;
import com.netflix.spinnaker.orca.webhook.config.WebhookProperties;
import java.io.IOException;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -48,7 +47,6 @@
import org.springframework.http.ResponseEntity;
import org.springframework.http.client.ClientHttpRequestFactory;
import org.springframework.http.converter.json.Jackson2ObjectMapperBuilder;
import org.springframework.web.client.ResourceAccessException;

class WebhookServiceTest {

Expand Down Expand Up @@ -221,9 +219,8 @@ void testRequestHeadersTooBig() throws Exception {
Throwable thrown = catchThrowable(() -> webhookService.callWebhook(stage));

assertThat(thrown)
.isInstanceOf(ResourceAccessException.class)
.hasRootCauseExactlyInstanceOf(IOException.class)
.hasRootCauseMessage("Canceled");
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("rejecting request to " + url);

apiProvider.verify(0, RequestPatternBuilder.allRequests());
}
Expand All @@ -246,9 +243,8 @@ void testRequestHeadersAndBodyTooBig() throws Exception {
Throwable thrown = catchThrowable(() -> webhookService.callWebhook(stage));

assertThat(thrown)
.isInstanceOf(ResourceAccessException.class)
.hasRootCauseExactlyInstanceOf(IOException.class)
.hasRootCauseMessage("Canceled");
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("rejecting request to " + url);

apiProvider.verify(0, RequestPatternBuilder.allRequests());
}
Expand Down Expand Up @@ -308,9 +304,8 @@ void testResponseHeadersTooBig() throws Exception {
Throwable thrown = catchThrowable(() -> webhookService.callWebhook(stage));

assertThat(thrown)
.isInstanceOf(ResourceAccessException.class)
.hasRootCauseExactlyInstanceOf(IOException.class)
.hasRootCauseMessage("Canceled");
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("rejecting response from " + url);

apiProvider.verify(getRequestedFor(urlPathEqualTo(path)));
}
Expand All @@ -337,9 +332,8 @@ void testResponseHeadersAndBodyTooBig() throws Exception {
Throwable thrown = catchThrowable(() -> webhookService.callWebhook(stage));

assertThat(thrown)
.isInstanceOf(ResourceAccessException.class)
.hasRootCauseExactlyInstanceOf(IOException.class)
.hasRootCauseMessage("Canceled");
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("rejecting response from " + url);

apiProvider.verify(getRequestedFor(urlPathEqualTo(path)));
}
Expand Down

0 comments on commit 182b4c6

Please sign in to comment.