This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new c895def Further ETag fixes
c895def is described below
commit c895deffb1fe881eaf7bd16a94b615266cda4bc0
Author: Mark Thomas <[email protected]>
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 b5e0b30..a685947 100644
--- a/conf/web.xml
+++ b/conf/web.xml
@@ -114,14 +114,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 513353f..fac8907 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -280,8 +280,6 @@ public class DefaultServlet extends HttpServlet {
*/
private boolean allowPartialPut = true;
- protected boolean useWeakComparisonWithIfMatch = true;
-
// --------------------------------------------------------- Public Methods
@@ -351,11 +349,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;
@@ -2171,31 +2164,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;
}
@@ -2251,31 +2245,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;
}
@@ -2287,7 +2285,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);
}
@@ -2333,9 +2331,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 cea1d8b..8b20ced 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -79,7 +79,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 b382895..899ce02 100644
--- a/webapps/docs/default-servlet.xml
+++ b/webapps/docs/default-servlet.xml
@@ -206,12 +206,6 @@ Tomcat.</p>
partial PUT? Note that RFC 7233 clarified that Range headers are only
valid for GET requests. [true]
</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: [email protected]
For additional commands, e-mail: [email protected]
