Cleanup

Author: robotdan

src/main/java/io/fusionauth/http/server/HTTPRequest.java

@@ -626,12 +626,16 @@ public void setURLParameters(Map<String, List<String>> parameters) {
    *     {@code Content-Length} header was provided.
    */
   public boolean hasBody() {
+    if (isChunked()) {
+      return true;
+    }
+
     Long contentLength = getContentLength();
-    return isChunked() || (contentLength != null && contentLength > 0);
+    return contentLength != null && contentLength > 0;
   }
 
   public boolean isChunked() {
-    return getTransferEncoding() != null && getTransferEncoding().equalsIgnoreCase(TransferEncodings.Chunked);
+    return TransferEncodings.Chunked.equalsIgnoreCase(getTransferEncoding());
   }
 
   /**

src/main/java/io/fusionauth/http/server/internal/HTTPWorker.java

@@ -418,7 +418,6 @@ private Integer validatePreamble(HTTPRequest request) {
     //   However, as long as we ignore Content-Length we should be ok. Earlier specs indicate Transfer-Encoding should take precedence,
     //   later specs imply it is an error. Seems ok to allow it and just ignore it.
     if (request.getHeader(Headers.TransferEncoding) == null) {
-      var contentLength = request.getContentLength();
       var requestedContentLengthHeaders = request.getHeaders(Headers.ContentLength);
       if (requestedContentLengthHeaders != null) {
         if (requestedContentLengthHeaders.size() != 1) {
@@ -430,6 +429,7 @@ private Integer validatePreamble(HTTPRequest request) {
           return Status.BadRequest;
         }
 
+        var contentLength = request.getContentLength();
         if (contentLength == null || contentLength < 0) {
           if (debugEnabled) {
             logger.debug("Invalid request. The Content-Length must be >= 0 and <= 9,223,372,036,854,775,807. [{}]", requestedContentLengthHeaders.getFirst());
@@ -445,13 +445,9 @@ private Integer validatePreamble(HTTPRequest request) {
       request.removeHeader(Headers.ContentLength);
     }
 
-    // Content-Encoding
+    // Validate Content-Encoding, we currently support deflate and gzip.
+    // - If we see anything else we should fail, we will be unable to handle the request.
     var contentEncodings = request.getContentEncodings();
-    // TODO : If provided, I think we can safely remove the Content-Length header if present?
-    //        Although, I suppose as long as we decompress early, we should still be able to use the Content-Length header as long as
-    //        it represents the un-compressed payload.
-    //        Maybe write a test to prove that we can compress with a fixed-length w/ a Content-Length header?
-    // Current support is for gzip and deflate.
     for (var encoding : contentEncodings) {
       if (!encoding.equalsIgnoreCase(ContentEncodings.Gzip) && !encoding.equalsIgnoreCase(ContentEncodings.Deflate)) {
         // Note that while we do not expect multiple Content-Encoding headers, the last one will be used. For good measure,

src/main/java/io/fusionauth/http/server/io/HTTPInputStream.java

@@ -159,28 +159,24 @@ public int read(byte[] b, int off, int len) throws IOException {
   private void initialize() throws IOException {
     initialized = true;
 
-    // Note that isChunked() should take precedence over the fact that we have a Content-Length.
-    // - The client should not send both, but in the case they are both present we ignore Content-Length
-    //   In practice, we will remove the Content-Length header when sent in addition to Transfer-Encoding. See HTTPWorker.validatePreamble.
-    Long contentLength = request.getContentLength();
-    boolean hasBody = (contentLength != null && contentLength > 0) || request.isChunked();
-    if (!hasBody) {
-      delegate = InputStream.nullInputStream();
-    } else if (request.isChunked()) {
-      logger.trace("Client indicated it was sending an entity-body in the request. Handling body using chunked encoding.");
-      delegate = new ChunkedInputStream(pushbackInputStream, chunkedBufferSize);
-      if (instrumenter != null) {
-        instrumenter.chunkedRequest();
+    // hasBody means we are either using chunked transfer encoding or we have a non-zero Content-Length.
+    boolean hasBody = request.hasBody();
+    if (hasBody) {
+      Long contentLength = request.getContentLength();
+      // Transfer-Encoding always takes precedence over Content-Length. In practice if they were to both be present on
+      // the request we would have removed Content-Length during validation to remove ambiguity. See HTTPWorker.validatePreamble.
+      if (request.isChunked()) {
+        logger.trace("Client indicated it was sending an entity-body in the request. Handling body using chunked encoding.");
+        delegate = new ChunkedInputStream(pushbackInputStream, chunkedBufferSize);
+        if (instrumenter != null) {
+          instrumenter.chunkedRequest();
+        }
+      } else {
+        logger.trace("Client indicated it was sending an entity-body in the request. Handling body using Content-Length header {}.", contentLength);
+        delegate = new FixedLengthInputStream(pushbackInputStream, contentLength);
       }
-    } else if (contentLength != null) {
-      logger.trace("Client indicated it was sending an entity-body in the request. Handling body using Content-Length header {}.", contentLength);
-      delegate = new FixedLengthInputStream(pushbackInputStream, contentLength);
-    } else {
-      logger.trace("Client indicated it was NOT sending an entity-body in the request");
-    }
 
-    // Now that we have the InputStream set up to read the body, handle decompression.
-    if (hasBody) {
+      // Now that we have the InputStream set up to read the body, handle decompression.
       // The request may contain more than one value, apply in reverse order.
       // - These are both using the default 512 buffer size.
       for (String contentEncoding : request.getContentEncodings().reversed()) {
@@ -190,15 +186,23 @@ private void initialize() throws IOException {
           delegate = new GZIPInputStream(delegate);
         }
       }
-    }
 
-    // If we have a fixed length request that is reporting a contentLength larger than the configured maximum, fail earl.
-    // - Do this last so if anyone downstream wants to read from the InputStream it would work.
-    // - Note that it is possible that the body is compressed which would mean the contentLength represents the compressed value.
-    //   But when we decompress the bytes the result will be larger than the reported contentLength, so we can safely throw this exception.
-    if (contentLength != null && maximumContentLength != -1 && contentLength > maximumContentLength) {
-      String detailedMessage = "The maximum request size has been exceeded. The reported Content-Length is [" + contentLength + "] and the maximum request size is [" + maximumContentLength + "] bytes.";
-      throw new ContentTooLargeException(maximumContentLength, detailedMessage);
+      // If we have a fixed length request that is reporting a contentLength larger than the configured maximum, fail early.
+      // - Do this last so if anyone downstream wants to read from the InputStream it would work.
+      // - Note that it is possible that the body is compressed which would mean the contentLength represents the compressed value.
+      //   But when we decompress the bytes the result will be larger than the reported contentLength, so we can safely throw this exception.
+      if (contentLength != null && maximumContentLength != -1 && contentLength > maximumContentLength) {
+        String detailedMessage = "The maximum request size has been exceeded. The reported Content-Length is [" + contentLength + "] and the maximum request size is [" + maximumContentLength + "] bytes.";
+        throw new ContentTooLargeException(maximumContentLength, detailedMessage);
+      }
+    } else {
+      // This means that we did not find Content-Length or Transfer-Encoding on the request. Do not attempt to read from the InputStream.
+      // - Note that the spec indicates it is plausible for a client to send an entity body and omit these two headers and the server can optionally
+      //   read bytes until the end of the InputStream is reached. This would assume Connection: close was also sent because if we do not know
+      //   how to delimit the request we cannot use a persistent connection.
+      // - We aren't doing any of that - if the client wants to send bytes, it needs to send a Content-Length header, or specify Transfer-Encoding: chunked.
+      logger.trace("Client indicated it was NOT sending an entity-body in the request");
+      delegate = InputStream.nullInputStream();
     }
   }
 }

src/test/java/io/fusionauth/http/BaseSocketTest.java

@@ -67,7 +67,7 @@ private void assertResponse(String request, String chunkedExtension, String resp
 
       if (request.contains("Transfer-Encoding: chunked")) {
         // Chunk in 100 byte increments. Using a smaller chunk size to ensure we don't end up with a single chunk.
-        body = new String(chunkEncoded(body.getBytes(StandardCharsets.UTF_8), 100, chunkedExtension));
+        body = new String(chunkEncode(body.getBytes(StandardCharsets.UTF_8), 100, chunkedExtension));
       }
 
       request = request.replace("{body}", body);

src/test/java/io/fusionauth/http/BaseTest.java

@@ -105,21 +105,6 @@
  * @author Brian Pontarelli
  */
 public abstract class BaseTest {
-  protected byte[] compressUsingContentEncoding(byte[] bytes, String contentEncoding) throws Exception {
-    if (!contentEncoding.isEmpty()) {
-      var requestEncodings = contentEncoding.toLowerCase().trim().split(",");
-      for (String part : requestEncodings) {
-        String encoding = part.trim();
-        if (encoding.equals(ContentEncodings.Deflate)) {
-          bytes = deflate(bytes);
-        } else if (encoding.equals(ContentEncodings.Gzip)) {
-          bytes = gzip(bytes);
-        }
-      }
-    }
-
-    return bytes;
-  }
   /**
    * This timeout is used for the HttpClient during each test. If you are in a debugger, you will need to change this timeout to be much
    * larger, otherwise, the client might truncate the request to the server.
@@ -266,11 +251,11 @@ public Object[][] connections() {
   @DataProvider(name = "contentEncoding")
   public Object[][] contentEncoding() {
     return new Object[][]{
-        {""},
-        {"gzip"},
-        {"deflate"},
-        {"gzip, deflate"},
-        {"deflate, gzip"}
+        {""},                // No compression
+        {"gzip"},            // gzip only
+        {"deflate"},         // deflate only
+        {"gzip, deflate"},   // gzip, then deflate
+        {"deflate, gzip"}    // deflate, then gzip
     };
   }
 
@@ -460,7 +445,7 @@ protected void assertHTTPResponseEquals(Socket socket, String expectedResponse)
     }
   }
 
-  protected byte[] chunkEncoded(byte[] bytes, int chunkSize, String chunkedExtension) throws IOException {
+  protected byte[] chunkEncode(byte[] bytes, int chunkSize, String chunkedExtension) throws IOException {
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     for (var i = 0; i < bytes.length; i += chunkSize) {
       var endIndex = Math.min(i + chunkSize, bytes.length);
@@ -485,6 +470,22 @@ protected byte[] chunkEncoded(byte[] bytes, int chunkSize, String chunkedExtensi
     return out.toByteArray();
   }
 
+  protected byte[] compressUsingContentEncoding(byte[] bytes, String contentEncoding) throws Exception {
+    if (!contentEncoding.isEmpty()) {
+      var requestEncodings = contentEncoding.toLowerCase().trim().split(",");
+      for (String part : requestEncodings) {
+        String encoding = part.trim();
+        if (encoding.equals(ContentEncodings.Deflate)) {
+          bytes = deflate(bytes);
+        } else if (encoding.equals(ContentEncodings.Gzip)) {
+          bytes = gzip(bytes);
+        }
+      }
+    }
+
+    return bytes;
+  }
+
   protected byte[] deflate(byte[] bytes) throws Exception {
     ByteArrayOutputStream baseOutputStream = new ByteArrayOutputStream();
     try (DeflaterOutputStream out = new DeflaterOutputStream(baseOutputStream, true)) {

src/test/java/io/fusionauth/http/FormDataTest.java

@@ -286,7 +286,7 @@ public Builder expectResponse(String response) throws Exception {
 
           // Convert body to chunked
           // - Using a small chunk to ensure we end up with more than one chunk.
-          body = new String(chunkEncoded(body.getBytes(StandardCharsets.UTF_8), 100, null));
+          body = new String(chunkEncode(body.getBytes(StandardCharsets.UTF_8), 100, null));
         } else {
           var contentLength = body.getBytes(StandardCharsets.UTF_8).length;
           if (contentLength > 0) {

src/test/java/io/fusionauth/http/io/HTTPInputStreamTest.java

@@ -20,12 +20,10 @@
 import java.nio.charset.StandardCharsets;
 
 import io.fusionauth.http.BaseTest;
-import io.fusionauth.http.HTTPValues.ContentEncodings;
 import io.fusionauth.http.HTTPValues.Headers;
 import io.fusionauth.http.server.HTTPRequest;
 import io.fusionauth.http.server.HTTPServerConfiguration;
 import io.fusionauth.http.server.io.HTTPInputStream;
-import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import static org.testng.Assert.assertEquals;
 
@@ -37,6 +35,7 @@ public class HTTPInputStreamTest extends BaseTest {
   public void read_chunked_withPushback(String contentEncoding) throws Exception {
     // Ensure that when we read a chunked encoded body that the InputStream returns the correct number of bytes read even when
     // we read past the end of the current request and use the PushbackInputStream.
+    // - Test with optional compression as well.
 
     String content = "These pretzels are making me thirsty. These pretzels are making me thirsty. These pretzels are making me thirsty.";
     byte[] payload = content.getBytes(StandardCharsets.UTF_8);
@@ -46,26 +45,29 @@ public void read_chunked_withPushback(String contentEncoding) throws Exception {
     payload = compressUsingContentEncoding(payload, contentEncoding);
 
     // Chunk the content, add part of the next request
-    payload = chunkEncoded(payload, 38, null);
+    payload = chunkEncode(payload, 38, null);
 
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     out.write(payload);
 
     // Add part of the next request
-    out.write("GET / HTTP/1.1\r\n".getBytes(StandardCharsets.UTF_8));
+    String nextRequest = "GET / HTTP/1.1\n\r";
+    byte[] nextRequestBytes = nextRequest.getBytes(StandardCharsets.UTF_8);
+    out.write(nextRequestBytes);
 
     HTTPRequest request = new HTTPRequest();
     request.setHeader(Headers.ContentEncoding, contentEncoding);
     request.setHeader(Headers.TransferEncoding, "chunked");
 
     byte[] bytes = out.toByteArray();
-    assertReadWithPushback(bytes, content, contentLength, 16, request);
+    assertReadWithPushback(bytes, content, contentLength, nextRequestBytes, request);
   }
 
   @Test(dataProvider = "contentEncoding")
   public void read_fixedLength_withPushback(String contentEncoding) throws Exception {
     // Ensure that when we read a fixed length body that the InputStream returns the correct number of bytes read even when
     // we read past the end of the current request and use the PushbackInputStream.
+    // - Test with optional compression as well.
 
     // Fixed length body with the start of the next request in the buffer
     String content = "These pretzels are making me thirsty. These pretzels are making me thirsty. These pretzels are making me thirsty.";
@@ -82,7 +84,9 @@ public void read_fixedLength_withPushback(String contentEncoding) throws Excepti
     out.write(payload);
 
     // Add part of the next request
-    out.write("GET / HTTP/1.1\r\n".getBytes(StandardCharsets.UTF_8));
+    String nextRequest = "GET / HTTP/1.1\n\r";
+    byte[] nextRequestBytes = nextRequest.getBytes(StandardCharsets.UTF_8);
+    out.write(nextRequestBytes);
 
     HTTPRequest request = new HTTPRequest();
     request.setHeader(Headers.ContentEncoding, contentEncoding);
@@ -91,10 +95,10 @@ public void read_fixedLength_withPushback(String contentEncoding) throws Excepti
     // body length is 113, when compressed it is 68 (gzip) or 78 (deflate)
     // The number of bytes available is 129.
     byte[] bytes = out.toByteArray();
-    assertReadWithPushback(bytes, content, contentLength, 16, request);
+    assertReadWithPushback(bytes, content, contentLength, nextRequestBytes, request);
   }
 
-  private void assertReadWithPushback(byte[] bytes, String content, int contentLength, int pushedBack, HTTPRequest request)
+  private void assertReadWithPushback(byte[] bytes, String content, int contentLength, byte[] pushedBackBytes, HTTPRequest request)
       throws Exception {
     int bytesAvailable = bytes.length;
     HTTPServerConfiguration configuration = new HTTPServerConfiguration().withRequestBufferSize(bytesAvailable + 100);
@@ -106,7 +110,9 @@ private void assertReadWithPushback(byte[] bytes, String content, int contentLen
     byte[] buffer = new byte[configuration.getRequestBufferSize()];
     int read = httpInputStream.read(buffer);
 
-    // TODO : Hmm.. with compression, this is returning the compressed bytes read instead of the un-compressed bytes read. WTF?
+    // Note that the HTTPInputStream read will return the number of uncompressed bytes read. So the contentLength passed in
+    // needs to represent the actual length of the request body, not the value of the Content-Length sent on the request which
+    // would represent the size of the compressed entity body.
     assertEquals(read, contentLength);
     assertEquals(new String(buffer, 0, read), content);
 
@@ -115,12 +121,12 @@ private void assertReadWithPushback(byte[] bytes, String content, int contentLen
     assertEquals(secondRead, -1);
 
     // We have 16 bytes left over
-    assertEquals(pushbackInputStream.getAvailableBufferedBytesRemaining(), pushedBack);
+    assertEquals(pushbackInputStream.getAvailableBufferedBytesRemaining(), pushedBackBytes.length);
 
     // Next read should start at the next request
     byte[] leftOverBuffer = new byte[100];
     int leftOverRead = pushbackInputStream.read(leftOverBuffer);
-    assertEquals(leftOverRead, pushedBack);
-    assertEquals(new String(leftOverBuffer, 0, leftOverRead), "GET / HTTP/1.1\r\n");
+    assertEquals(leftOverRead, pushedBackBytes.length);
+    assertEquals(new String(leftOverBuffer, 0, leftOverRead), new String(pushedBackBytes));
   }
 }