This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 8.5.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push: new bc379af Further ETag fixes bc379af is described below commit bc379af8e355e969d02bb60fb49e3026667522fa Author: Mark Thomas <ma...@apache.org> AuthorDate: Wed Aug 12 12:46:00 2020 +0100 Further ETag fixes Revert addition of useWeakComparisonWithIfMatch Always use strong comparison for If-Match Correct regression previous fix that returned wrong etag for a 304 response to If-None-Match Based on pull requests by Sergey Ponomarev --- conf/web.xml | 8 --- .../apache/catalina/servlets/DefaultServlet.java | 76 ++++++++++------------ .../TestDefaultServletIfMatchRequests.java | 59 +++++++++-------- webapps/docs/changelog.xml | 3 +- webapps/docs/default-servlet.xml | 6 -- 5 files changed, 70 insertions(+), 82 deletions(-) diff --git a/conf/web.xml b/conf/web.xml index d31b3a8..4392dcd 100644 --- a/conf/web.xml +++ b/conf/web.xml @@ -109,14 +109,6 @@ <!-- with a Range header as a partial PUT? Note --> <!-- that RFC 7233 clarified that Range headers are --> <!-- only valid for GET requests. [true] --> - <!-- --> - <!-- useWeakComparisonWithIfMatch --> - <!-- When comparing entity tags for If-Match --> - <!-- headers should a weak comparison be used --> - <!-- rather than the strong comparison required by --> - <!-- RFC 7232? A weak comparison is used by default --> - <!-- since the default resources implementation --> - <!-- generates weak entity tags. [true] --> <servlet> <servlet-name>default</servlet-name> diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java b/java/org/apache/catalina/servlets/DefaultServlet.java index e832a86..6785da2 100644 --- a/java/org/apache/catalina/servlets/DefaultServlet.java +++ b/java/org/apache/catalina/servlets/DefaultServlet.java @@ -287,8 +287,6 @@ public class DefaultServlet extends HttpServlet { */ private boolean allowPartialPut = true; - protected boolean useWeakComparisonWithIfMatch = true; - // --------------------------------------------------------- Public Methods @@ -358,11 +356,6 @@ public class DefaultServlet extends HttpServlet { useAcceptRanges = Boolean.parseBoolean(getServletConfig().getInitParameter("useAcceptRanges")); } - if (getServletConfig().getInitParameter("useWeakComparisonWithIfMatch") != null) { - useWeakComparisonWithIfMatch = Boolean.parseBoolean( - getServletConfig().getInitParameter("useWeakComparisonWithIfMatch")); - } - // Sanity check on the specified buffer sizes if (input < 256) { input = 256; @@ -2317,31 +2310,32 @@ public class DefaultServlet extends HttpServlet { * request processing is stopped * @throws IOException an IO error occurred */ - protected boolean checkIfMatch(HttpServletRequest request, - HttpServletResponse response, WebResource resource) + protected boolean checkIfMatch(HttpServletRequest request, HttpServletResponse response, WebResource resource) throws IOException { - String eTag = generateETag(resource); - // Default servlet uses weak matching so we strip any leading "W/" and - // then compare using equals - if (eTag.startsWith("W/")) { - eTag = eTag.substring(2); - } String headerValue = request.getHeader("If-Match"); - if (headerValue != null && !headerValue.equals("*")) { + if (headerValue != null) { - Set<String> eTags = EntityTag.parseEntityTag(new StringReader(headerValue), useWeakComparisonWithIfMatch); - if (eTags == null) { - if (debug > 10) { - log("DefaultServlet.checkIfMatch: Invalid header value [" + headerValue + "]"); + boolean conditionSatisfied = false; + + if (!headerValue.equals("*")) { + String resourceETag = generateETag(resource); + + // RFC 7232 requires strong comparison for If-Match headers + Set<String> headerETags = EntityTag.parseEntityTag(new StringReader(headerValue), false); + if (headerETags == null) { + if (debug > 10) { + log("DefaultServlet.checkIfMatch: Invalid header value [" + headerValue + "]"); + } + response.sendError(HttpServletResponse.SC_BAD_REQUEST); + return false; } - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return false; + conditionSatisfied = headerETags.contains(resourceETag); + } else { + conditionSatisfied = true; } - // If none of the given ETags match, 412 Precondition failed is - // sent back - if (!eTags.contains(eTag)) { + if (!conditionSatisfied) { response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED); return false; } @@ -2397,31 +2391,35 @@ public class DefaultServlet extends HttpServlet { * request processing is stopped * @throws IOException an IO error occurred */ - protected boolean checkIfNoneMatch(HttpServletRequest request, - HttpServletResponse response, WebResource resource) + protected boolean checkIfNoneMatch(HttpServletRequest request, HttpServletResponse response, WebResource resource) throws IOException { - String eTag = generateETag(resource); - // If-None-Match uses weak comparison so strip the weak indicator if - // present - if (eTag.startsWith("W/")) { - eTag = eTag.substring(2); - } String headerValue = request.getHeader("If-None-Match"); if (headerValue != null) { boolean conditionSatisfied = false; + String resourceETag = generateETag(resource); if (!headerValue.equals("*")) { - Set<String> eTags = EntityTag.parseEntityTag(new StringReader(headerValue), true); - if (eTags == null) { + + // RFC 7232 requires weak comparison for If-None-Match headers + // This is done by removing any weak markers before comparison + String comparisonETag; + if (resourceETag.startsWith("W/")) { + comparisonETag = resourceETag.substring(2); + } else { + comparisonETag = resourceETag; + } + + Set<String> headerETags = EntityTag.parseEntityTag(new StringReader(headerValue), true); + if (headerETags == null) { if (debug > 10) { log("DefaultServlet.checkIfNoneMatch: Invalid header value [" + headerValue + "]"); } response.sendError(HttpServletResponse.SC_BAD_REQUEST); return false; } - conditionSatisfied = eTags.contains(eTag); + conditionSatisfied = headerETags.contains(comparisonETag); } else { conditionSatisfied = true; } @@ -2433,7 +2431,7 @@ public class DefaultServlet extends HttpServlet { // back. if ("GET".equals(request.getMethod()) || "HEAD".equals(request.getMethod())) { response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - response.setHeader("ETag", eTag); + response.setHeader("ETag", resourceETag); } else { response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED); } @@ -2479,9 +2477,7 @@ public class DefaultServlet extends HttpServlet { /** * Provides the entity tag (the ETag header) for the given resource. * Intended to be over-ridden by custom DefaultServlet implementations that - * wish to use an alternative format for the entity tag. Such custom - * implementations that generate strong entity tags may also want to change - * the default value of {@link #useWeakComparisonWithIfMatch}. + * wish to use an alternative format for the entity tag. * * @param resource The resource for which an entity tag is required. * diff --git a/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java b/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java index 102fc0f..b514882 100644 --- a/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java +++ b/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java @@ -45,6 +45,8 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { private static final Integer RC_412 = Integer.valueOf(412); private static final String[] CONCAT = new String[] { ",", " ,", ", ", " , " }; + private static String resourceETagStrong; + private static String resourceETagWeak; @Parameterized.Parameters(name = "{index} resource-strong [{0}], matchHeader [{1}]") public static Collection<Object[]> parameters() { @@ -52,8 +54,8 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { // Get the length of the file used for this test // It varies by platform due to line-endings File index = new File("test/webapp/index.html"); - String resourceETagStrong = "\"" + index.length() + "-" + index.lastModified() + "\""; - String resourceETagWeak = "W/" + resourceETagStrong; + resourceETagStrong = "\"" + index.length() + "-" + index.lastModified() + "\""; + resourceETagWeak = "W/" + resourceETagStrong; String otherETagStrong = "\"123456789\""; String otherETagWeak = "\"123456789\""; @@ -88,36 +90,36 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { parameterSets.add(new Object[] { resourceWithStrongETag, otherETagWeak, RC_412, RC_200 }); // match header includes weak tag - // Results depend on whether strong or weak comparison is used - Integer rcWeak; - if (resourceWithStrongETag.booleanValue()) { - rcWeak = RC_412; - } else { - rcWeak = RC_200; - } - parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak, rcWeak, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak, RC_412, RC_304 }); for (String concat : CONCAT) { parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak + concat + otherETagWeak, - rcWeak, RC_304 }); + RC_412, RC_304 }); parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak + concat + otherETagStrong, - rcWeak, RC_304 }); + RC_412, RC_304 }); parameterSets.add(new Object[] { resourceWithStrongETag, otherETagWeak + concat + resourceETagWeak, - rcWeak, RC_304 }); + RC_412, RC_304 }); parameterSets.add(new Object[] { resourceWithStrongETag, otherETagStrong + concat + resourceETagWeak, - rcWeak, RC_304 }); + RC_412, RC_304 }); } - // match header includes strong tag - parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong, RC_200, RC_304 }); + // match header includes strong entity tag + // If-Match result depends on whether the resource has a strong entity tag + Integer rcIfMatch; + if (resourceWithStrongETag.booleanValue()) { + rcIfMatch = RC_200; + } else { + rcIfMatch = RC_412; + } + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong, rcIfMatch, RC_304 }); for (String concat : CONCAT) { parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + concat + otherETagWeak, - RC_200, RC_304 }); + rcIfMatch, RC_304 }); parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + concat + otherETagStrong, - RC_200, RC_304 }); + rcIfMatch, RC_304 }); parameterSets.add(new Object[] { resourceWithStrongETag, otherETagWeak + concat + resourceETagStrong, - RC_200, RC_304 }); + rcIfMatch, RC_304 }); parameterSets.add(new Object[] { resourceWithStrongETag, otherETagStrong + concat + resourceETagStrong, - RC_200, RC_304 }); + rcIfMatch, RC_304 }); } } @@ -138,17 +140,17 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { @Test public void testIfMatch() throws Exception { - doMatchTest("If-Match", ifMatchResponseCode); + doMatchTest("If-Match", ifMatchResponseCode, false); } @Test public void testIfNoneMatch() throws Exception { - doMatchTest("If-None-Match", ifNoneMatchResponseCode); + doMatchTest("If-None-Match", ifNoneMatchResponseCode, true); } - private void doMatchTest(String headerName, int responseCodeExpected) throws Exception { + private void doMatchTest(String headerName, int responseCodeExpected, boolean responseHasEtag) throws Exception { Tomcat tomcat = getTomcatInstance(); File appDir = new File("test/webapp"); @@ -178,6 +180,13 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { // Check the result Assert.assertEquals(responseCodeExpected, rc); + + // If-None-Match should have a real resource ETag in successful response + if (responseHasEtag && (rc == 200 || rc == 304)) { + System.out.println(responseHeaders); + String responseEtag = responseHeaders.get("ETag").get(0); + Assert.assertEquals(resourceHasStrongETag ? resourceETagStrong : resourceETagWeak, responseEtag); + } } @@ -185,10 +194,6 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { private static final long serialVersionUID = 1L; - public DefaultWithStrongETag() { - useWeakComparisonWithIfMatch = false; - } - @Override protected String generateETag(WebResource resource) { String weakETag = super.generateETag(resource); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index a368a7e..83581c4 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -73,7 +73,8 @@ requests. Requests with headers that contain invalid entity tags will be rejected with a 400 response code. Improve the matching algorithm used to compare entity tags in conditional requests with the entity tag for - the requested resource. (markt) + the requested resource. Based on a pull request by Sergey Ponomarev. + (markt) </fix> </changelog> </subsection> diff --git a/webapps/docs/default-servlet.xml b/webapps/docs/default-servlet.xml index 235a6be..f66d604 100644 --- a/webapps/docs/default-servlet.xml +++ b/webapps/docs/default-servlet.xml @@ -186,12 +186,6 @@ directory listings are disabled and debugging is turned off. <property name="sortDirectoriesFirst"> Should the server list all directories before all files. [false] </property> - <property name="useWeakComparisonWithIfMatch"> - When comparing entity tags for If-Match headers should a weak comparison - be used rather than the strong comparison required by RFC 7232? A weak - comparison is used by default since the default resources implementation - generates weak entity tags. [true] - </property> </properties> </section> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org