Fix BZ 69466 - rework handling of HEAD requests

Headers explicitly set by users will not be removed and any header
present in a HEAD request will also be present in the equivalent GET
request. There may be some headers, as per RFC 9110, section 9.3.2, that
are present in a GET request that are not present in the equivalent HEAD
request.

https://bz.apache.org/bugzilla/show_bug.cgi?id=69466

java/org/apache/catalina/connector/OutputBuffer.java

@@ -220,9 +220,14 @@ public void close() throws IOException {
             flushCharBuffer();
         }
 
-        if ((!coyoteResponse.isCommitted()) && (coyoteResponse.getContentLengthLong() == -1)) {
-            // If this didn't cause a commit of the response, the final content
-            // length can be calculated.
+        // Content length can be calculated if:
+        // - the response has not been committed
+        // AND
+        // - the content length has not been explicitly set
+        // AND
+        // - some content has been written OR this is NOT a HEAD request
+        if ((!coyoteResponse.isCommitted()) && (coyoteResponse.getContentLengthLong() == -1) &&
+                ((bb.remaining() > 0 || !coyoteResponse.getRequest().method().equals("HEAD")))) {
             coyoteResponse.setContentLength(bb.remaining());
         }
 

java/org/apache/coyote/http11/Http11Processor.java

@@ -895,7 +895,7 @@ protected final void prepareResponse() throws IOException {
 
         boolean head = request.method().equals("HEAD");
         if (head) {
-            // No entity body
+            // Any entity body, if present, should not be sent
             outputBuffer.addActiveFilter(outputFilters[Constants.VOID_FILTER]);
             contentDelimitation = true;
         }
@@ -935,6 +935,13 @@ protected final void prepareResponse() throws IOException {
             headers.setValue("Content-Length").setLong(contentLength);
             outputBuffer.addActiveFilter(outputFilters[Constants.IDENTITY_FILTER]);
             contentDelimitation = true;
+        } else if (head) {
+            /*
+             * The OutputBuffer can't differentiate between a zero length response because GET would have produced a
+             * zero length body and a zero length response because HEAD didn't write any content. Therefore skip setting
+             * the "transfer-encoding: chunked" header as that may not be consistent with what would be seen with the
+             * equivalent GET.
+             */
         } else {
             // If the response code supports an entity body and we're on
             // HTTP 1.1 then we chunk unless we have a Connection: close header
@@ -1034,15 +1041,6 @@ protected final void prepareResponse() throws IOException {
             headers.setValue("Server").setString(server);
         }
 
-        // Exclude some HTTP header fields where the value is determined only
-        // while generating the content as per section 9.3.2 of RFC 9110.
-        if (head) {
-            headers.removeHeader("content-length");
-            headers.removeHeader("content-range");
-            headers.removeHeader("trailer");
-            headers.removeHeader("transfer-encoding");
-        }
-
         writeHeaders(response.getStatus(), headers);
 
         outputBuffer.commit();

java/org/apache/coyote/http2/StreamProcessor.java

@@ -255,13 +255,6 @@ static void prepareHeaders(Request coyoteRequest, Response coyoteResponse, boole
                 headers.setValue("Server").setString(server);
             }
         }
-
-        // Exclude some HTTP header fields where the value is determined only
-        // while generating the content as per section 9.3.2 of RFC 9110.
-        if (coyoteRequest != null && coyoteRequest.method().equals("HEAD")) {
-            headers.removeHeader("content-length");
-            headers.removeHeader("content-range");
-        }
     }
 
 

test/jakarta/servlet/http/HttpServletDoHeadBaseTest.java

@@ -22,7 +22,6 @@
 import java.lang.reflect.Field;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -55,11 +54,12 @@ public class HttpServletDoHeadBaseTest extends Http2TestBase {
     private static final String VALID = "** valid data **";
     private static final String INVALID = "* invalid data *";
 
-    protected static final Integer BUFFERS[] = new Integer[] { Integer.valueOf (16), Integer.valueOf(8 * 1024), Integer.valueOf(16 * 1024) };
+    protected static final Integer BUFFERS[] =
+            new Integer[] { Integer.valueOf(16), Integer.valueOf(8 * 1024), Integer.valueOf(16 * 1024) };
 
-    protected static final Integer COUNTS[] = new Integer[] { Integer.valueOf(0), Integer.valueOf(1),
-            Integer.valueOf(511), Integer.valueOf(512), Integer.valueOf(513),
-            Integer.valueOf(1023), Integer.valueOf(1024), Integer.valueOf(1025) };
+    protected static final Integer COUNTS[] =
+            new Integer[] { Integer.valueOf(0), Integer.valueOf(1), Integer.valueOf(511), Integer.valueOf(512),
+                    Integer.valueOf(513), Integer.valueOf(1023), Integer.valueOf(1024), Integer.valueOf(1025) };
 
     @Parameter(2)
     public boolean useLegacy;
@@ -90,26 +90,26 @@ public void testDoHead() throws Exception {
         Assert.assertEquals(HttpServletResponse.SC_OK, rc);
         out.recycle();
 
-        Map<String,List<String>> headHeaders = new HashMap<>();
+        Map<String,List<String>> headHeaders = new CaseInsensitiveKeyMap<>();
         rc = headUrl(path, out, headHeaders);
         Assert.assertEquals(HttpServletResponse.SC_OK, rc);
 
-        // Headers should be the same part from:
-        // - Date header may be different
-        // Exclude some HTTP header fields where the value is determined only
-        // while generating the content as per section 9.3.2 of RFC 9110.
-        // (previously RFC 7231, section 4.3.2)
-        getHeaders.remove("content-length");
-        getHeaders.remove("content-range");
-        getHeaders.remove("trailer");
-        getHeaders.remove("transfer-encoding");
+        // Date header is likely to be different so just remove it from both GET and HEAD.
+        getHeaders.remove("date");
+        headHeaders.remove("date");
+        /*
+         * There are some headers that are optional for HEAD. See RFC 9110, section 9.3.2. If present, they must be the
+         * same for both GET and HEAD. If not present in HEAD, remove them from GET.
+         */
+        for (String header : TesterConstants.OPTIONAL_HEADERS_WITH_HEAD) {
+            if (!headHeaders.containsKey(header)) {
+                getHeaders.remove(header);
+            }
+        }
 
         Assert.assertEquals(getHeaders.size(), headHeaders.size());
-        for (Map.Entry<String, List<String>> getHeader : getHeaders.entrySet()) {
+        for (Map.Entry<String,List<String>> getHeader : getHeaders.entrySet()) {
             String headerName = getHeader.getKey();
-            if ("date".equalsIgnoreCase(headerName)) {
-                continue;
-            }
             Assert.assertTrue(headerName, headHeaders.containsKey(headerName));
             List<String> getValues = getHeader.getValue();
             List<String> headValues = headHeaders.get(headerName);
@@ -168,18 +168,30 @@ public void testDoHeadHttp2() throws Exception {
             int i = 0;
             int j = 0;
             for (; i < getHeaders.length; i++) {
-                // Exclude some HTTP header fields where the value is determined
-                // only while generating the content as per section 9.3.2 of RFC
-                // 9110.
-                if (getHeaders[i].contains("content-length") || getHeaders[i].contains("content-range") ) {
-                    // Skip
-                } else {
-                    // Headers should be the same, ignoring the first character which is the steam ID
-                    Assert.assertEquals(getHeaders[i] + "\n" + traceGet + traceHead, '3', getHeaders[i].charAt(0));
-                    Assert.assertEquals(headHeaders[j] + "\n" + traceGet + traceHead, '5', headHeaders[j].charAt(0));
-                    Assert.assertEquals(traceGet + traceHead, getHeaders[i].substring(1), headHeaders[j].substring(1));
-                    j++;
+                /*
+                 * There are some headers that are optional for HEAD. See RFC 9110, section 9.3.2. If present, they must
+                 * be the same for both GET and HEAD. If not present in HEAD, ignore them in GET.
+                 */
+                boolean skip = false;
+                for (String optional : TesterConstants.OPTIONAL_HEADERS_WITH_HEAD) {
+                    if (getHeaders[i].contains(optional)) {
+                        if (!headHeaders[i].contains(optional)) {
+                            // Skip it
+                            skip = true;
+                        }
+                        // Found it. Don't need to check the remaining headers
+                        break;
+                    }
                 }
+                if (skip) {
+                    continue;
+                }
+
+                // Remaining headers should be the same, ignoring the first character which is the steam ID
+                Assert.assertEquals(getHeaders[i] + "\n" + traceGet + traceHead, '3', getHeaders[i].charAt(0));
+                Assert.assertEquals(headHeaders[j] + "\n" + traceGet + traceHead, '5', headHeaders[j].charAt(0));
+                Assert.assertEquals(traceGet + traceHead, getHeaders[i].substring(1), headHeaders[j].substring(1));
+                j++;
             }
 
             // Stream 5 should have one more trace entry
@@ -202,7 +214,8 @@ protected void configureAndStartWebApplication() throws LifecycleException {
         Tomcat.addServlet(ctxt, "simple", new SimpleServlet());
         ctxt.addServletMappingDecoded("/simple", "simple");
 
-        HeadTestServlet s = new HeadTestServlet(bufferSize, useWriter, invalidWriteCount, resetType, validWriteCount, explicitFlush);
+        HeadTestServlet s = new HeadTestServlet(bufferSize, useWriter, invalidWriteCount, resetType, validWriteCount,
+                explicitFlush);
         Wrapper w = Tomcat.addServlet(ctxt, "HeadTestServlet", s);
         if (useLegacy) {
             w.addInitParameter(HttpServlet.LEGACY_DO_HEAD, "true");
@@ -247,33 +260,25 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
                 /*
                  * Using legacy HEAD handling with a Writer.
                  *
-                 * HttpServlet wraps the response. The test is sensitive to
-                 * buffer sizes. For the tests to pass the number of
-                 * characters that can be written before the response is
-                 * committed needs to be the same.
+                 * HttpServlet wraps the response. The test is sensitive to buffer sizes. For the tests to pass the
+                 * number of characters that can be written before the response is committed needs to be the same.
                  *
-                 * Internally, the Tomcat response buffer defaults to 8192
-                 * bytes. When a Writer is used, an additional 8192 byte buffer
-                 * is used for char->byte.
+                 * Internally, the Tomcat response buffer defaults to 8192 bytes. When a Writer is used, an additional
+                 * 8192 byte buffer is used for char->byte.
                  *
-                 * The char->byte buffer used by HttpServlet processing HEAD
-                 * requests in legacy mode is created as a 512 byte buffer.
+                 * The char->byte buffer used by HttpServlet processing HEAD requests in legacy mode is created as a 512
+                 * byte buffer.
                  *
-                 * If the response isn't reset, none of this matters as it is
-                 * just the size of the response buffer and the size of the
-                 * response that determines if chunked encoding is used.
-                 * However, if the response is reset then things get interesting
-                 * as the amount of response data that can be written before
-                 * committing the response is the combination of the char->byte
-                 * buffer and the response buffer. The size of the response
-                 * buffer needs to be adjusted so that both GET and HEAD can
-                 * write the same amount of data before committing the response.
-                 * To make matters worse, to obtain the correct behaviour the
-                 * response buffer size needs to be reset back to 8192 after the
-                 * reset.
+                 * If the response isn't reset, none of this matters as it is just the size of the response buffer and
+                 * the size of the response that determines if chunked encoding is used. However, if the response is
+                 * reset then things get interesting as the amount of response data that can be written before
+                 * committing the response is the combination of the char->byte buffer and the response buffer. The size
+                 * of the response buffer needs to be adjusted so that both GET and HEAD can write the same amount of
+                 * data before committing the response. To make matters worse, to obtain the correct behaviour the
+                 * response buffer size needs to be reset back to 8192 after the reset.
                  *
-                 * This is why the legacy mode is problematic and the new
-                 * approach of the container handling HEAD is better.
+                 * This is why the legacy mode is problematic and the new approach of the container handling HEAD is
+                 * better.
                  */
 
                 // Response buffer is always no smaller than originalBufferSize

test/jakarta/servlet/http/TestHttpServlet.java

@@ -45,10 +45,6 @@
 
 public class TestHttpServlet extends TomcatBaseTest {
 
-    /*
-     * Nature of test has changed from original bug report since content-length
-     * is no longer returned for HEAD requests as allowed by RFC 9110.
-     */
     @Test
     public void testBug53454() throws Exception {
         Tomcat tomcat = getTomcatInstance();
@@ -68,7 +64,7 @@ public void testBug53454() throws Exception {
                resHeaders);
 
         Assert.assertEquals(HttpServletResponse.SC_OK, rc);
-        Assert.assertNull(resHeaders.get("Content-Length"));
+        Assert.assertEquals(LargeBodyServlet.RESPONSE_LENGTH, resHeaders.get("Content-Length").get(0));
     }
 
 
@@ -181,20 +177,29 @@ private void doTestHead(Servlet servlet) throws Exception {
 
         int rc = getUrl(path, out, getHeaders);
         Assert.assertEquals(HttpServletResponse.SC_OK, rc);
-        removeGeneratingContentHeaders(getHeaders);
         out.recycle();
 
-        Map<String,List<String>> headHeaders = new HashMap<>();
+        Map<String,List<String>> headHeaders = new CaseInsensitiveKeyMap<>();
         rc = headUrl(path, out, headHeaders);
         Assert.assertEquals(HttpServletResponse.SC_OK, rc);
 
+        // Date header is likely to be different so just remove it from both GET and HEAD.
+        getHeaders.remove("date");
+        headHeaders.remove("date");
+        /*
+         * There are some headers that are optional for HEAD. See RFC 9110, section 9.3.2. If present, they must be the
+         * same for both GET and HEAD. If not present in HEAD, remove them from GET.
+         */
+        for (String header : TesterConstants.OPTIONAL_HEADERS_WITH_HEAD) {
+            if (!headHeaders.containsKey(header)) {
+                getHeaders.remove(header);
+            }
+        }
+
         // Headers should be the same (apart from Date)
         Assert.assertEquals(getHeaders.size(), headHeaders.size());
         for (Map.Entry<String, List<String>> getHeader : getHeaders.entrySet()) {
             String headerName = getHeader.getKey();
-            if ("date".equalsIgnoreCase(headerName)) {
-                continue;
-            }
             Assert.assertTrue(headerName, headHeaders.containsKey(headerName));
             List<String> getValues = getHeader.getValue();
             List<String> headValues = headHeaders.get(headerName);
@@ -208,18 +213,6 @@ private void doTestHead(Servlet servlet) throws Exception {
     }
 
 
-    /*
-     * Removes headers that are not expected to appear in the response to the
-     * equivalent HEAD request.
-     */
-    private void removeGeneratingContentHeaders(Map<String,List<String>> headers) {
-        headers.remove("content-length");
-        headers.remove("content-range");
-        headers.remove("trailer");
-        headers.remove("transfer-encoding");
-    }
-
-
     @Test
     public void testDoOptions() throws Exception {
         doTestDoOptions(new OptionsServlet(), "GET, HEAD, OPTIONS");

test/jakarta/servlet/http/TesterConstants.java

@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package jakarta.servlet.http;
+
+public class TesterConstants {
+
+    public static final String[] OPTIONAL_HEADERS_WITH_HEAD =
+            new String[] { "content-length", "vary", "transfer-encoding", "trailers" };
+}