From 0698cc6b8cee5fd267e713b619745c3c7e829cc0 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Tue, 26 Nov 2024 10:24:25 +1300 Subject: [PATCH] Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests (#5704) * Add tests * Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests * Fix style * Handle HttpURLConnection switching GET->POST and not supporting PATCH * Update protocol test (cherry picked from commit b2fcb7e18ab040c5b8feb23346c879120f179126) --- .../bugfix-AWSSDKforJavav2-c212259.json | 6 + .../impl/ApacheHttpRequestFactory.java | 64 +++++++--- ...nnectionHttpClientDefaultWireMockTest.java | 30 +++++ .../http/SdkHttpClientDefaultTestSuite.java | 114 +++++++++++++++--- 4 files changed, 180 insertions(+), 34 deletions(-) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-c212259.json diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json b/.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json new file mode 100644 index 000000000000..aa0eca4ab0fe --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS SDK for Java v2", + "contributor": "Xtansia", + "description": "Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests" +} diff --git a/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java b/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java index cfb22343ba3f..a9979152e553 100644 --- a/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java +++ b/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java @@ -24,14 +24,7 @@ import org.apache.http.HttpEntity; import org.apache.http.HttpHeaders; import org.apache.http.client.config.RequestConfig; -import org.apache.http.client.methods.HttpDelete; import org.apache.http.client.methods.HttpEntityEnclosingRequestBase; -import org.apache.http.client.methods.HttpGet; -import org.apache.http.client.methods.HttpHead; -import org.apache.http.client.methods.HttpOptions; -import org.apache.http.client.methods.HttpPatch; -import org.apache.http.client.methods.HttpPost; -import org.apache.http.client.methods.HttpPut; import org.apache.http.client.methods.HttpRequestBase; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.http.HttpExecuteRequest; @@ -116,28 +109,29 @@ private void addRequestConfig(final HttpRequestBase base, private HttpRequestBase createApacheRequest(HttpExecuteRequest request, URI uri) { - switch (request.httpRequest().method()) { + SdkHttpMethod method = request.httpRequest().method(); + switch (method) { case HEAD: - return new HttpHead(uri); case GET: - return new HttpGet(uri); case DELETE: - return new HttpDelete(uri); case OPTIONS: - return new HttpOptions(uri); + if (hasContentStream(request)) { + return createEntityEnclosingRequest(request, method, uri); + } + return new HttpRequestImpl(method, uri); case PATCH: - return wrapEntity(request, new HttpPatch(uri)); case POST: - return wrapEntity(request, new HttpPost(uri)); case PUT: - return wrapEntity(request, new HttpPut(uri)); + return createEntityEnclosingRequest(request, method, uri); default: - throw new RuntimeException("Unknown HTTP method name: " + request.httpRequest().method()); + throw new RuntimeException("Unknown HTTP method name: " + method); } } - private HttpRequestBase wrapEntity(HttpExecuteRequest request, - HttpEntityEnclosingRequestBase entityEnclosingRequest) { + + + private HttpRequestBase createEntityEnclosingRequest(HttpExecuteRequest request, SdkHttpMethod method, URI uri) { + HttpEntityEnclosingRequestBase entityEnclosingRequest = new HttpEntityEnclosingRequestImpl(method, uri); /* * We should never reuse the entity of the previous request, since @@ -149,7 +143,7 @@ private HttpRequestBase wrapEntity(HttpExecuteRequest request, * preparation for the retry. Eventually, these wrappers would * return incorrect validation result. */ - if (request.contentStreamProvider().isPresent()) { + if (hasContentStream(request)) { HttpEntity entity = new RepeatableInputStreamRequestEntity(request); if (!request.httpRequest().firstMatchingHeader(HttpHeaders.CONTENT_LENGTH).isPresent() && !entity.isChunked()) { entity = ApacheUtils.newBufferedHttpEntity(entity); @@ -160,6 +154,10 @@ private HttpRequestBase wrapEntity(HttpExecuteRequest request, return entityEnclosingRequest; } + private static boolean hasContentStream(HttpExecuteRequest request) { + return request.contentStreamProvider().isPresent(); + } + /** * Configures the headers in the specified Apache HTTP request. */ @@ -192,4 +190,32 @@ private String getHostHeaderValue(SdkHttpRequest request) { ? request.host() + ":" + request.port() : request.host(); } + + private static final class HttpRequestImpl extends HttpRequestBase { + private final SdkHttpMethod method; + + private HttpRequestImpl(SdkHttpMethod method, URI uri) { + this.method = method; + setURI(uri); + } + + @Override + public String getMethod() { + return this.method.toString(); + } + } + + private static final class HttpEntityEnclosingRequestImpl extends HttpEntityEnclosingRequestBase { + private final SdkHttpMethod method; + + private HttpEntityEnclosingRequestImpl(SdkHttpMethod method, URI uri) { + this.method = method; + setURI(uri); + } + + @Override + public String getMethod() { + return this.method.toString(); + } + } } diff --git a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java index 66cbc4e3b814..bd69df1422fa 100644 --- a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java +++ b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java @@ -15,11 +15,16 @@ package software.amazon.awssdk.http.urlconnection; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.net.ProtocolException; import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLSocketFactory; +import org.junit.Test; import org.junit.jupiter.api.AfterEach; import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.http.SdkHttpClientDefaultTestSuite; +import software.amazon.awssdk.http.SdkHttpMethod; public class UrlConnectionHttpClientDefaultWireMockTest extends SdkHttpClientDefaultTestSuite { @@ -32,4 +37,29 @@ protected SdkHttpClient createSdkHttpClient() { public void reset() { HttpsURLConnection.setDefaultSSLSocketFactory((SSLSocketFactory) SSLSocketFactory.getDefault()); } + + @Test + @Override + public void supportsGetRequestWithRequestBody() throws Exception { + // HttpURLConnection is hard-coded to switch GET requests with a body to POST requests, in #getOutputStream0. + testForResponseCode(200, SdkHttpMethod.GET, SdkHttpMethod.POST, true); + } + + @Test + @Override + public void supportsPatchRequestWithoutRequestBody() { + // HttpURLConnection does not support PATCH requests. + assertThatThrownBy(super::supportsPatchRequestWithoutRequestBody) + .hasRootCauseInstanceOf(ProtocolException.class) + .hasRootCauseMessage("Invalid HTTP method: PATCH"); + } + + @Test + @Override + public void supportsPatchRequestWithRequestBody() { + // HttpURLConnection does not support PATCH requests. + assertThatThrownBy(super::supportsPatchRequestWithRequestBody) + .hasRootCauseInstanceOf(ProtocolException.class) + .hasRootCauseMessage("Invalid HTTP method: PATCH"); + } } diff --git a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java index e0e300b848d4..d6859a4955cf 100644 --- a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java +++ b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java @@ -61,7 +61,7 @@ public void supportsResponseCode200() throws Exception { @Test public void supportsResponseCode200Head() throws Exception { // HEAD is special due to closing of the connection immediately and streams are null - testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD); + testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false); } @Test @@ -76,7 +76,7 @@ public void supportsResponseCode403() throws Exception { @Test public void supportsResponseCode403Head() throws Exception { - testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD); + testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false); } @Test @@ -98,22 +98,99 @@ public void supportsResponseCode500() throws Exception { public void validatesHttpsCertificateIssuer() { SdkHttpClient client = createSdkHttpClient(); - SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST); + SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST, true); assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call) .isInstanceOf(SSLHandshakeException.class); } + @Test + public void supportsDeleteRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.DELETE, false); + } + + @Test + public void supportsDeleteRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.DELETE, true); + } + + @Test + public void supportsGetRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.GET, false); + } + + @Test + public void supportsGetRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.GET, true); + } + + @Test + public void supportsHeadRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.HEAD, false); + } + + @Test + public void supportsHeadRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.HEAD, true); + } + + @Test + public void supportsOptionsRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.OPTIONS, false); + } + + @Test + public void supportsOptionsRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.OPTIONS, true); + } + + @Test + public void supportsPatchRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.PATCH, false); + } + + @Test + public void supportsPatchRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.PATCH, true); + } + + @Test + public void supportsPostRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.POST, false); + } + + @Test + public void supportsPostRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.POST, true); + } + + @Test + public void supportsPutRequestWithoutRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.PUT, false); + } + + @Test + public void supportsPutRequestWithRequestBody() throws Exception { + testForResponseCode(200, SdkHttpMethod.PUT, true); + } + private void testForResponseCode(int returnCode) throws Exception { - testForResponseCode(returnCode, SdkHttpMethod.POST); + testForResponseCode(returnCode, SdkHttpMethod.POST, true); } - private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Exception { + protected void testForResponseCode(int returnCode, SdkHttpMethod method, boolean includeBody) throws Exception { + testForResponseCode(returnCode, method, method, includeBody); + } + + protected void testForResponseCode(int returnCode, + SdkHttpMethod method, + SdkHttpMethod expectedMethod, + boolean includeBody) throws Exception { SdkHttpClient client = createSdkHttpClient(); stubForMockRequest(returnCode); - SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method); + SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method, includeBody); HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder() .request(req) .contentStreamProvider(req.contentStreamProvider() @@ -121,14 +198,14 @@ private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Ex .build()) .call(); - validateResponse(rsp, returnCode, method); + validateResponse(rsp, returnCode, expectedMethod, includeBody); } protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCode) throws Exception { SdkHttpMethod sdkHttpMethod = SdkHttpMethod.POST; stubForMockRequest(returnCode); - SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod); + SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod, true); HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder() .request(req) .contentStreamProvider(req.contentStreamProvider() @@ -136,7 +213,7 @@ protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCod .build()) .call(); - validateResponse(rsp, returnCode, sdkHttpMethod); + validateResponse(rsp, returnCode, sdkHttpMethod, true); } private void stubForMockRequest(int returnCode) { @@ -151,17 +228,24 @@ private void stubForMockRequest(int returnCode) { mockServer.stubFor(any(urlPathEqualTo("/")).willReturn(responseBuilder)); } - private void validateResponse(HttpExecuteResponse response, int returnCode, SdkHttpMethod method) throws IOException { + private void validateResponse(HttpExecuteResponse response, + int returnCode, + SdkHttpMethod method, + boolean expectBody) throws IOException { RequestMethod requestMethod = RequestMethod.fromString(method.name()); RequestPatternBuilder patternBuilder = RequestPatternBuilder.newRequestPattern(requestMethod, urlMatching("/")) .withHeader("Host", containing("localhost")) .withHeader("User-Agent", equalTo("hello-world!")); - if (method == SdkHttpMethod.HEAD) { - patternBuilder.withRequestBody(absent()); + if (expectBody) { + patternBuilder.withRequestBody(equalTo("Body")) + .withHeader("Content-Length", equalTo("4")); } else { - patternBuilder.withRequestBody(equalTo("Body")); + patternBuilder.withRequestBody(absent()); + if (method != SdkHttpMethod.PATCH && method != SdkHttpMethod.POST && method != SdkHttpMethod.PUT) { + patternBuilder.withoutHeader("Content-Length"); + } } mockServer.verify(1, patternBuilder); @@ -177,14 +261,14 @@ private void validateResponse(HttpExecuteResponse response, int returnCode, SdkH mockServer.resetMappings(); } - private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method) { + private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method, boolean includeBody) { URI uri = URI.create(uriString); SdkHttpFullRequest.Builder requestBuilder = SdkHttpFullRequest.builder() .uri(uri) .method(method) .putHeader("Host", uri.getHost()) .putHeader("User-Agent", "hello-world!"); - if (method != SdkHttpMethod.HEAD) { + if (includeBody) { byte[] content = "Body".getBytes(StandardCharsets.UTF_8); requestBuilder.putHeader("Content-Length", Integer.toString(content.length)); requestBuilder.contentStreamProvider(() -> new ByteArrayInputStream(content));