This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 3963ff12f3 Fix BZ 69439 - Improve handling of multiple Cache-Control
headers
3963ff12f3 is described below
commit 3963ff12f31e4beb03d51f76718a0c0bc77b0dc1
Author: Mark Thomas <[email protected]>
AuthorDate: Wed Nov 20 19:17:10 2024 +0000
Fix BZ 69439 - Improve handling of multiple Cache-Control headers
Based on #777 by Chenjp
---
.../org/apache/catalina/filters/ExpiresFilter.java | 20 ++--
.../apache/catalina/filters/TestExpiresFilter.java | 127 +++++++++++++++++----
webapps/docs/changelog.xml | 5 +
3 files changed, 122 insertions(+), 30 deletions(-)
diff --git a/java/org/apache/catalina/filters/ExpiresFilter.java
b/java/org/apache/catalina/filters/ExpiresFilter.java
index f3bde19d0e..be38e58dd7 100644
--- a/java/org/apache/catalina/filters/ExpiresFilter.java
+++ b/java/org/apache/catalina/filters/ExpiresFilter.java
@@ -165,10 +165,10 @@ import org.apache.tomcat.util.buf.StringUtils;
* <h4>
* {@code ExpiresExcludedResponseStatusCodes}</h4>
* <p>
- * This directive defines the http response status codes for which the
+ * This directive defines the HTTP response status codes for which the
* {@code ExpiresFilter} will not generate expiration headers. By default, the
* {@code 304} status code ("{@code Not modified}") is skipped. The
- * value is a comma separated list of http status codes.
+ * value is a comma separated list of HTTP status codes.
* </p>
* <p>
* This directive is useful to ease usage of {@code ExpiresDefault} directive.
@@ -332,7 +332,7 @@ import org.apache.tomcat.util.buf.StringUtils;
* </p>
* <p>
* To trap the 'before write response body' event, the
- * {@code ExpiresFilter} wraps the http servlet response's writer and
+ * {@code ExpiresFilter} wraps the HTTP servlet response's writer and
* outputStream to intercept calls to the methods {@code write()},
* {@code print()}, {@code close()} and {@code flush()}. For empty response
* body (e.g. empty files), the {@code write()}, {@code print()},
@@ -517,7 +517,7 @@ public class ExpiresFilter extends FilterBase {
/**
* <p>
- * Wrapping extension of the {@link HttpServletResponse} to yrap the
"Start Write Response Body" event.
+ * Wrapping extension of the {@link HttpServletResponse} to wrap the
"Start Write Response Body" event.
* </p>
* <p>
* For performance optimization : this extended response holds the {@link
#lastModifiedHeader} and
@@ -528,12 +528,12 @@ public class ExpiresFilter extends FilterBase {
public class XHttpServletResponse extends HttpServletResponseWrapper {
/**
- * Value of the {@code Cache-Control} http response header if it has
been set.
+ * Value of the {@code Cache-Control} HTTP response header if it has
been set.
*/
private String cacheControlHeader;
/**
- * Value of the {@code Last-Modified} http response header if it has
been set.
+ * Value of the {@code Last-Modified} HTTP response header if it has
been set.
*/
private long lastModifiedHeader;
@@ -568,8 +568,12 @@ public class ExpiresFilter extends FilterBase {
@Override
public void addHeader(String name, String value) {
super.addHeader(name, value);
- if (HEADER_CACHE_CONTROL.equalsIgnoreCase(name) &&
cacheControlHeader == null) {
- cacheControlHeader = value;
+ if (HEADER_CACHE_CONTROL.equalsIgnoreCase(name)) {
+ if (cacheControlHeader == null) {
+ cacheControlHeader = value;
+ } else {
+ cacheControlHeader = StringUtils.join(cacheControlHeader,
value);
+ }
}
}
diff --git a/test/org/apache/catalina/filters/TestExpiresFilter.java
b/test/org/apache/catalina/filters/TestExpiresFilter.java
index 83a82d3a6e..edb15813dc 100644
--- a/test/org/apache/catalina/filters/TestExpiresFilter.java
+++ b/test/org/apache/catalina/filters/TestExpiresFilter.java
@@ -42,6 +42,7 @@ import
org.apache.catalina.filters.ExpiresFilter.StartingPoint;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.tomcat.util.buf.ByteChunk;
+import org.apache.tomcat.util.buf.StringUtils;
import org.apache.tomcat.util.collections.CaseInsensitiveKeyMap;
import org.apache.tomcat.util.descriptor.web.FilterDef;
import org.apache.tomcat.util.descriptor.web.FilterMap;
@@ -92,8 +93,8 @@ public class TestExpiresFilter extends TomcatBaseTest {
Assert.assertEquals(1,
expiresConfigurationDefault.getDurations().get(0).getAmount());
// VERIFY TEXT/HTML
- ExpiresConfiguration expiresConfigurationTextHtml =
expiresFilter.getExpiresConfigurationByContentType()
- .get("text/html");
+ ExpiresConfiguration expiresConfigurationTextHtml =
+
expiresFilter.getExpiresConfigurationByContentType().get("text/html");
Assert.assertEquals(StartingPoint.ACCESS_TIME,
expiresConfigurationTextHtml.getStartingPoint());
Assert.assertEquals(3,
expiresConfigurationTextHtml.getDurations().size());
@@ -111,8 +112,8 @@ public class TestExpiresFilter extends TomcatBaseTest {
Assert.assertEquals(2, twoHours.getAmount());
// VERIFY IMAGE/GIF
- ExpiresConfiguration expiresConfigurationImageGif =
expiresFilter.getExpiresConfigurationByContentType()
- .get("image/gif");
+ ExpiresConfiguration expiresConfigurationImageGif =
+
expiresFilter.getExpiresConfigurationByContentType().get("image/gif");
Assert.assertEquals(StartingPoint.LAST_MODIFICATION_TIME,
expiresConfigurationImageGif.getStartingPoint());
Assert.assertEquals(2,
expiresConfigurationImageGif.getDurations().size());
@@ -126,8 +127,8 @@ public class TestExpiresFilter extends TomcatBaseTest {
Assert.assertEquals(3, threeMinutes.getAmount());
// VERIFY IMAGE/JPG
- ExpiresConfiguration expiresConfigurationImageJpg =
expiresFilter.getExpiresConfigurationByContentType()
- .get("image/jpg");
+ ExpiresConfiguration expiresConfigurationImageJpg =
+
expiresFilter.getExpiresConfigurationByContentType().get("image/jpg");
Assert.assertEquals(StartingPoint.ACCESS_TIME,
expiresConfigurationImageJpg.getStartingPoint());
Assert.assertEquals(1,
expiresConfigurationImageJpg.getDurations().size());
@@ -137,8 +138,8 @@ public class TestExpiresFilter extends TomcatBaseTest {
Assert.assertEquals(10000, tenThousandSeconds.getAmount());
// VERIFY VIDEO/MPEG
- ExpiresConfiguration expiresConfiguration =
expiresFilter.getExpiresConfigurationByContentType()
- .get("video/mpeg");
+ ExpiresConfiguration expiresConfiguration =
+
expiresFilter.getExpiresConfigurationByContentType().get("video/mpeg");
Assert.assertEquals(StartingPoint.LAST_MODIFICATION_TIME,
expiresConfiguration.getStartingPoint());
Assert.assertEquals(1, expiresConfiguration.getDurations().size());
@@ -173,8 +174,8 @@ public class TestExpiresFilter extends TomcatBaseTest {
@Test
public void testParseExpiresConfigurationCombinedDuration() {
ExpiresFilter expiresFilter = new ExpiresFilter();
- ExpiresConfiguration actualConfiguration = expiresFilter
- .parseExpiresConfiguration("access plus 1 month 15 days 2
hours");
+ ExpiresConfiguration actualConfiguration =
+ expiresFilter.parseExpiresConfiguration("access plus 1 month
15 days 2 hours");
Assert.assertEquals(StartingPoint.ACCESS_TIME,
actualConfiguration.getStartingPoint());
@@ -226,7 +227,7 @@ public class TestExpiresFilter extends TomcatBaseTest {
}
};
- validate(servlet, null, HttpServletResponse.SC_NOT_MODIFIED);
+ validate(servlet, null, null, HttpServletResponse.SC_NOT_MODIFIED);
}
@Test
@@ -362,11 +363,11 @@ public class TestExpiresFilter extends TomcatBaseTest {
}
protected void validate(HttpServlet servlet, Integer
expectedMaxAgeInSeconds) throws Exception {
- validate(servlet, expectedMaxAgeInSeconds, HttpServletResponse.SC_OK);
+ validate(servlet, expectedMaxAgeInSeconds, null,
HttpServletResponse.SC_OK);
}
- protected void validate(HttpServlet servlet, Integer
expectedMaxAgeInSeconds, int expectedResponseStatusCode)
- throws Exception {
+ protected void validate(HttpServlet servlet, Integer
expectedMaxAgeInSeconds, Boolean expectedExpires,
+ int expectedResponseStatusCode) throws Exception {
// SETUP
@@ -401,14 +402,14 @@ public class TestExpiresFilter extends TomcatBaseTest {
// TEST
ByteChunk bc = new ByteChunk();
- Map<String, List<String>> responseHeaders = new HashMap<>();
+ Map<String,List<String>> responseHeaders = new HashMap<>();
int rc = getUrl("http://localhost:"; + getPort() + "/test", bc,
responseHeaders);
// VALIDATE
Assert.assertEquals(expectedResponseStatusCode, rc);
StringBuilder msg = new StringBuilder();
- for (Entry<String, List<String>> field :
responseHeaders.entrySet()) {
+ for (Entry<String,List<String>> field :
responseHeaders.entrySet()) {
for (String value : field.getValue()) {
msg.append((field.getKey() == null ? "" : field.getKey() +
": ") + value + "\n");
}
@@ -417,7 +418,7 @@ public class TestExpiresFilter extends TomcatBaseTest {
Integer actualMaxAgeInSeconds;
- String cacheControlHeader = getSingleHeader("Cache-Control",
responseHeaders);
+ String cacheControlHeader =
StringUtils.join(responseHeaders.get("Cache-Control"));
if (cacheControlHeader == null) {
actualMaxAgeInSeconds = null;
@@ -438,13 +439,30 @@ public class TestExpiresFilter extends TomcatBaseTest {
}
if (expectedMaxAgeInSeconds == null) {
- Assert.assertNull("actualMaxAgeInSeconds '" +
actualMaxAgeInSeconds + "' should be null",
- actualMaxAgeInSeconds);
+ Assert.assertTrue(
+ "actualMaxAgeInSeconds [" + actualMaxAgeInSeconds +
+ "] should be null or 'no-store' should be set",
+ actualMaxAgeInSeconds == null ||
+ (cacheControlHeader != null &&
cacheControlHeader.contains("no-store")));
return;
}
Assert.assertNotNull(actualMaxAgeInSeconds);
+ if (expectedExpires != null) {
+ if (expectedExpires.booleanValue()) {
+ // Expires is expected and should be greater than date
+ Assert.assertNotNull(responseHeaders.get("Expires"));
+ String expiresString =
responseHeaders.get("Expires").get(0);
+ long expiresDate =
FastHttpDateFormat.parseDate(expiresString);
+ Assert.assertTrue(
+ "Expires header value [" + expiresString + "] does
not specify a date in the future",
+ expiresDate > System.currentTimeMillis());
+ } else {
+ Assert.assertNull(responseHeaders.get("Expires"));
+ }
+ }
+
String contentType = getSingleHeader("Content-Type",
responseHeaders);
int deltaInSeconds = Math.abs(actualMaxAgeInSeconds.intValue() -
expectedMaxAgeInSeconds.intValue());
@@ -495,11 +513,11 @@ public class TestExpiresFilter extends TomcatBaseTest {
tomcat.start();
ByteChunk bc = new ByteChunk();
- Map<String, List<String>> requestHeaders = new
CaseInsensitiveKeyMap<>();
+ Map<String,List<String>> requestHeaders = new
CaseInsensitiveKeyMap<>();
List<String> ifModifiedSinceValues = new ArrayList<>();
ifModifiedSinceValues.add(FastHttpDateFormat.getCurrentDate());
requestHeaders.put("If-Modified-Since", ifModifiedSinceValues);
- Map<String, List<String>> responseHeaders = new
CaseInsensitiveKeyMap<>();
+ Map<String,List<String>> responseHeaders = new
CaseInsensitiveKeyMap<>();
int rc = getUrl("http://localhost:"; + getPort() +
"/test/bug6nnnn/bug69303.txt", bc, requestHeaders,
responseHeaders);
@@ -507,7 +525,7 @@ public class TestExpiresFilter extends TomcatBaseTest {
Assert.assertEquals(HttpServletResponse.SC_NOT_MODIFIED, rc);
StringBuilder msg = new StringBuilder();
- for (Entry<String, List<String>> field : responseHeaders.entrySet()) {
+ for (Entry<String,List<String>> field : responseHeaders.entrySet()) {
for (String value : field.getValue()) {
msg.append((field.getKey() == null ? "" : field.getKey() + ":
") + value + "\n");
}
@@ -539,4 +557,69 @@ public class TestExpiresFilter extends TomcatBaseTest {
Assert.assertNotNull(actualMaxAgeInSeconds);
Assert.assertTrue(Math.abs(actualMaxAgeInSeconds.intValue() - 420) <
3);
}
+
+
+ @Test
+ public void testBug69439CacheControlDirectiveConflict() throws Exception {
+ HttpServlet servlet = new HttpServlet() {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void service(HttpServletRequest request,
HttpServletResponse response)
+ throws ServletException, IOException {
+ response.setContentType("image/jpeg");
+ response.addHeader("Cache-Control", "public");
+ response.addHeader("Cache-Control", "no-store");
+ response.addHeader("Cache-Control", "max-age=300");
+ response.getWriter().print("Hello world");
+ }
+ };
+ // rfc9111 - 4.2.1 - If directives conflict (e.g., both max-age and
no-cache are present), the most restrictive
+ // directive should be honored.
+ // final effective Cache-Control: no-store
+ // skip ExpiresFilter
+ validate(servlet, null, Boolean.FALSE, HttpServletResponse.SC_OK);
+ }
+
+
+ @Test
+ public void testBug69439CacheControlMaxAgePredefined() throws Exception {
+ HttpServlet servlet = new HttpServlet() {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void service(HttpServletRequest request,
HttpServletResponse response)
+ throws ServletException, IOException {
+ response.setContentType("image/jpeg");
+ response.addHeader("Cache-Control", "public");
+ response.addHeader("Cache-Control", "max-age=600");
+ response.getWriter().print("Hello world");
+ }
+ };
+ // rfc9111 - 4.2.1 - If directives conflict (e.g., both max-age and
no-cache are present), the most restrictive
+ // directive should be honored.
+ // final effective Cache-Control: public, max-age=600
+ // skip ExpiresFilter
+ validate(servlet, Integer.valueOf(600), Boolean.FALSE,
HttpServletResponse.SC_OK);
+ }
+
+
+ @Test
+ public void testBug69439CacheControlUndefined() throws Exception {
+ HttpServlet servlet = new HttpServlet() {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void service(HttpServletRequest request,
HttpServletResponse response)
+ throws ServletException, IOException {
+ response.setContentType("image/jpeg");
+ response.getWriter().print("Hello world");
+ }
+ };
+ // rfc9111 - 4.2.1 - If directives conflict (e.g., both max-age and
no-cache are present), the most restrictive
+ // directive should be honored.
+ // final effective Cache-Control: public, max-age=600
+ // apply ExpiresFilter
+ validate(servlet, Integer.valueOf(60), Boolean.TRUE,
HttpServletResponse.SC_OK);
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index b038d790b9..c1bf93447e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -162,6 +162,11 @@
<fix>
Use client locale for directory listings. (remm)
</fix>
+ <fix>
+ <bug>69439</bug>: Improve the handling of multiple
+ <code>Cache-Control</code> headers in the <code>ExpiresFilter</code>.
+ Based on pull request <pr>777</pr> by Chenjp. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
