From f285db9ae89a4e707af1ff15fa903a4042926121 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 6 Nov 2024 12:03:22 +1100 Subject: [PATCH 1/3] Fix #12481 Handle headers with 304 responses Allow Content-Length header to be set with a 304 response Follow RFC9110 recommendations for headers to be sent with 304 response. --- .../eclipse/jetty/server/ResourceService.java | 108 ++++++++++++------ .../server/internal/HttpChannelState.java | 7 +- .../jetty/server/HttpServerTestBase.java | 33 ++++++ .../server/handler/ResourceHandlerTest.java | 8 ++ 4 files changed, 116 insertions(+), 40 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java index 1dcdf600a9b5..38c026bc09fb 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.Enumeration; import java.util.List; import java.util.Map; @@ -58,6 +59,12 @@ public class ResourceService private static final Logger LOG = LoggerFactory.getLogger(ResourceService.class); private static final int NO_CONTENT_LENGTH = -1; private static final int USE_KNOWN_CONTENT_LENGTH = -2; + public static final EnumSet CONTENT_HEADERS = EnumSet.of( + HttpHeader.LAST_MODIFIED, + HttpHeader.CONTENT_LENGTH, + HttpHeader.CONTENT_TYPE + ); + public static final PreEncodedHttpField ACCEPT_RANGES_BYTES = new PreEncodedHttpField(HttpHeader.ACCEPT_RANGES, "bytes"); private final List _precompressedFormats = new ArrayList<>(); private final Map> _preferredEncodingOrderCache = new ConcurrentHashMap<>(); @@ -351,6 +358,7 @@ protected boolean passConditionalHeaders(Request request, Response response, Htt if (matched != null) { response.getHeaders().put(HttpHeader.ETAG, matched); + putNotModifiedHeaders(response, content); writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304); return true; } @@ -368,17 +376,18 @@ protected boolean passConditionalHeaders(Request request, Response response, Htt String mdlm = content.getLastModifiedValue(); if (ifms.equals(mdlm)) { + putNotModifiedHeaders(response, content); writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304); return true; } - // TODO: what should we do when we get a crappy date? long ifmsl = HttpDateTime.parseToEpoch(ifms); if (ifmsl != -1) { long lm = content.getResource().lastModified().toEpochMilli(); if (lm != -1 && lm / 1000 <= ifmsl / 1000) { + putNotModifiedHeaders(response, content); writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304); return true; } @@ -388,7 +397,6 @@ protected boolean passConditionalHeaders(Request request, Response response, Htt // Parse the if[un]modified dates and compare to resource if (ifums != null && ifm == null) { - // TODO: what should we do when we get a crappy date? long ifumsl = HttpDateTime.parseToEpoch(ifums); if (ifumsl != -1) { @@ -728,55 +736,79 @@ protected void writeHttpContent(Request request, Response response, Callback cal protected void putHeaders(Response response, HttpContent content, long contentLength) { - // TODO it is very inefficient to do many put's to a HttpFields, as each put is a full iteration. - // it might be better remove headers en masse and then just add the extras: - // NOTE: If these headers come from a Servlet Filter we shouldn't override them here. -// headers.remove(EnumSet.of( -// HttpHeader.LAST_MODIFIED, -// HttpHeader.CONTENT_LENGTH, -// HttpHeader.CONTENT_TYPE, -// HttpHeader.CONTENT_ENCODING, -// HttpHeader.ETAG, -// HttpHeader.ACCEPT_RANGES, -// HttpHeader.CACHE_CONTROL -// )); -// HttpField lm = content.getLastModified(); -// if (lm != null) -// headers.add(lm); -// etc. + HttpFields.Mutable headers = response.getHeaders(); + // Existing etags have priority over content etags (often set by compression handler) + if (_etags && !headers.contains(HttpHeader.ETAG)) + { + HttpField et = content.getETag(); + if (et != null) + headers.add(et); + } + + // Existing content encoding is kept only if not set for content. + HttpField ce = content.getContentEncoding(); + if (ce != null) + headers.put(ce); + + // Remove content headers and re-add if we have them + headers.remove(CONTENT_HEADERS); HttpField lm = content.getLastModified(); if (lm != null) - response.getHeaders().put(lm); - + headers.add(lm); if (contentLength == USE_KNOWN_CONTENT_LENGTH) - { - response.getHeaders().put(content.getContentLength()); - } + headers.add(content.getContentLength()); else if (contentLength > NO_CONTENT_LENGTH) - { - response.getHeaders().put(HttpHeader.CONTENT_LENGTH, contentLength); - } - + headers.add(HttpHeader.CONTENT_LENGTH, contentLength); HttpField ct = content.getContentType(); if (ct != null) - response.getHeaders().put(ct); + headers.add(ct); - HttpField ce = content.getContentEncoding(); - if (ce != null) - response.getHeaders().put(ce); + putHeaders(response); + } + protected void putNotModifiedHeaders(Response response, HttpContent content) + { + HttpFields.Mutable headers = response.getHeaders(); + + boolean sendLastModified = true; + + // Existing etags have priority over content etags (often set by compression handler) if (_etags) { - HttpField et = content.getETag(); - if (et != null) - response.getHeaders().put(et); + if (headers.contains(HttpHeader.ETAG)) + { + sendLastModified = false; + } + else + { + HttpField et = content.getETag(); + if (et != null) + { + headers.add(et); + sendLastModified = false; + } + } + } + + // Send last modified only if there is no etag + if (sendLastModified) + { + HttpField lm = content.getLastModified(); + if (lm != null) + headers.put(lm); } - if (_acceptRanges && !response.getHeaders().contains(HttpHeader.ACCEPT_RANGES)) - response.getHeaders().put(new PreEncodedHttpField(HttpHeader.ACCEPT_RANGES, "bytes")); - if (_cacheControl != null && !response.getHeaders().contains(HttpHeader.CACHE_CONTROL)) - response.getHeaders().put(_cacheControl); + putHeaders(response); + } + + protected void putHeaders(Response response) + { + HttpFields.Mutable headers = response.getHeaders(); + if (_acceptRanges && !headers.contains(HttpHeader.ACCEPT_RANGES)) + headers.add(ACCEPT_RANGES_BYTES); + if (_cacheControl != null && !headers.contains(HttpHeader.CACHE_CONTROL)) + headers.add(_cacheControl); } /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f0758d5084ee..b315e9353806 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -1250,7 +1250,8 @@ public void write(boolean last, ByteBuffer content, Callback callback) long committedContentLength = httpChannelState._committedContentLength; long contentLength = committedContentLength >= 0 ? committedContentLength : getHeaders().getLongField(HttpHeader.CONTENT_LENGTH); - if (contentLength >= 0 && totalWritten != contentLength) + if (contentLength >= 0 && totalWritten != contentLength && + !(totalWritten == 0 && (HttpMethod.HEAD.is(_request.getMethod()) || getStatus() == HttpStatus.NOT_MODIFIED_304))) { // If the content length were not compatible with what was written, then we need to abort. String lengthError = null; @@ -1525,7 +1526,9 @@ public void succeeded() long totalWritten = response._contentBytesWritten; long committedContentLength = httpChannelState._committedContentLength; - if (committedContentLength >= 0 && committedContentLength != totalWritten && !(totalWritten == 0 && HttpMethod.HEAD.is(_request.getMethod()))) + if (committedContentLength >= 0 && + committedContentLength != totalWritten && + !(totalWritten == 0 && (HttpMethod.HEAD.is(_request.getMethod()) || response.getStatus() == HttpStatus.NOT_MODIFIED_304))) failure = ExceptionUtil.combine(failure, new IOException("content-length %d != %d written".formatted(committedContentLength, totalWritten))); // Is the request fully consumed? diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java index 2a2b1a6cedc9..a0a76f546884 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java @@ -1456,6 +1456,39 @@ public boolean handle(Request request, Response response, Callback callback) thr } } + @Test + public void test304WithContentLength() throws Exception + { + startServer(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + response.setStatus(304); + response.getHeaders().add(HttpHeader.CONTENT_LENGTH, 10); + callback.succeeded(); + return true; + } + }); + + try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort())) + { + OutputStream os = client.getOutputStream(); + InputStream is = client.getInputStream(); + + os.write((""" + GET /R1 HTTP/1.1\r + Host: localhost\r + Connection: close\r + + """).getBytes(StandardCharsets.ISO_8859_1)); + + String in = IO.toString(is); + assertThat(in, containsString("304 Not Modified")); + assertThat(in, containsString("Content-Length: 10")); + } + } + @Test public void testBlockedClient() throws Exception { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java index 6643cd6fbe0c..edadd36f69a6 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java @@ -1480,6 +1480,7 @@ public void testCachingPrecompressedFilesCachedEtagged() throws Exception \r """.formatted(eTag1))); assertThat(response3.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response3.getField(ETAG), notNullValue()); HttpTester.Response response4 = HttpTester.parseResponse( _local.getResponse(""" @@ -1491,6 +1492,7 @@ public void testCachingPrecompressedFilesCachedEtagged() throws Exception \r """.formatted(eTag2))); assertThat(response4.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response3.getField(ETAG), notNullValue()); } assertThat(contentFactory.getCachedFiles(), is(2)); @@ -1989,6 +1991,7 @@ public void testEtagIfNoneMatchNotModifiedFile() throws Exception \r """.formatted(etag))); assertThat(response.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response.getField(ETAG), notNullValue()); assertThat(response.getContent(), is("")); } @@ -2346,6 +2349,7 @@ public void testIfETag(String content) throws Exception """.replace("@ETAG@", etag)); response = HttpTester.parseResponse(rawResponse); assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response.getField(ETAG), notNullValue()); rawResponse = _local.getResponse(""" GET /context/file.txt HTTP/1.1\r @@ -2356,6 +2360,7 @@ public void testIfETag(String content) throws Exception """.replace("@ETAG@", etag)); response = HttpTester.parseResponse(rawResponse); assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response.getField(ETAG), notNullValue()); rawResponse = _local.getResponse(""" GET /context/file.txt HTTP/1.1\r @@ -2468,6 +2473,7 @@ public void testIfModified(String content) throws Exception """.replace("@LASTMODIFIED@", lastModified)); response = HttpTester.parseResponse(rawResponse); assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response.getField(LAST_MODIFIED), notNullValue()); rawResponse = _local.getResponse(""" GET /context/file.txt HTTP/1.1\r @@ -2488,6 +2494,7 @@ public void testIfModified(String content) throws Exception """.replace("@DATE@", DateGenerator.formatDate(System.currentTimeMillis() + 10000))); response = HttpTester.parseResponse(rawResponse); assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304)); + assertThat(response.getField(LAST_MODIFIED), notNullValue()); rawResponse = _local.getResponse(""" GET /context/file.txt HTTP/1.1\r @@ -2537,6 +2544,7 @@ public void testIfModifiedSince() throws Exception """.formatted(lastModified))); assertThat(response.getStatus(), equalTo(304)); + assertThat(response.getField(LAST_MODIFIED), notNullValue()); assertThat(response.getContent(), is("")); response = HttpTester.parseResponse( From 5b7f4348dfc32ad803712631bc5c2d606ad71039 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 6 Nov 2024 14:42:57 +1100 Subject: [PATCH 2/3] Fix #12481 Handle headers with 304 responses Allow Content-Length header to be set with a 304 response Follow RFC9110 recommendations for headers to be sent with 304 response. --- .../eclipse/jetty/server/ResourceService.java | 32 +++---------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java index 38c026bc09fb..a9fa057d3e8e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java @@ -770,34 +770,10 @@ else if (contentLength > NO_CONTENT_LENGTH) protected void putNotModifiedHeaders(Response response, HttpContent content) { HttpFields.Mutable headers = response.getHeaders(); - - boolean sendLastModified = true; - - // Existing etags have priority over content etags (often set by compression handler) - if (_etags) - { - if (headers.contains(HttpHeader.ETAG)) - { - sendLastModified = false; - } - else - { - HttpField et = content.getETag(); - if (et != null) - { - headers.add(et); - sendLastModified = false; - } - } - } - - // Send last modified only if there is no etag - if (sendLastModified) - { - HttpField lm = content.getLastModified(); - if (lm != null) - headers.put(lm); - } + // send only lastModified, as it is too difficult to determine the etag with compression + HttpField lm = content.getLastModified(); + if (lm != null) + headers.put(lm); putHeaders(response); } From 606c30cf0980001f7c450054e57190e0e0f0a08a Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 7 Nov 2024 08:57:58 +1100 Subject: [PATCH 3/3] updates from review --- .../main/java/org/eclipse/jetty/server/ResourceService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java index a9fa057d3e8e..b367f376ea01 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java @@ -59,12 +59,12 @@ public class ResourceService private static final Logger LOG = LoggerFactory.getLogger(ResourceService.class); private static final int NO_CONTENT_LENGTH = -1; private static final int USE_KNOWN_CONTENT_LENGTH = -2; - public static final EnumSet CONTENT_HEADERS = EnumSet.of( + private static final EnumSet CONTENT_HEADERS = EnumSet.of( HttpHeader.LAST_MODIFIED, HttpHeader.CONTENT_LENGTH, HttpHeader.CONTENT_TYPE ); - public static final PreEncodedHttpField ACCEPT_RANGES_BYTES = new PreEncodedHttpField(HttpHeader.ACCEPT_RANGES, "bytes"); + private static final PreEncodedHttpField ACCEPT_RANGES_BYTES = new PreEncodedHttpField(HttpHeader.ACCEPT_RANGES, "bytes"); private final List _precompressedFormats = new ArrayList<>(); private final Map> _preferredEncodingOrderCache = new ConcurrentHashMap<>();