This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 7.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/7.0.x by this push: new 52e2b0a Improve entity tag handling 52e2b0a is described below commit 52e2b0ab3a7ed13835b1db955f2ebbb44d56edf9 Author: Mark Thomas <ma...@apache.org> AuthorDate: Tue Aug 11 15:27:45 2020 +0100 Improve entity tag handling --- conf/web.xml | 8 ++ .../apache/catalina/servlets/DefaultServlet.java | 116 +++++++++-------- .../apache/tomcat/util/http/parser/EntityTag.java | 82 ++++++++++++ .../apache/tomcat/util/http/parser/HttpParser.java | 4 +- .../TestDefaultServletIfMatchRequests.java | 143 +++++++++++++++++---- webapps/docs/changelog.xml | 7 + webapps/docs/default-servlet.xml | 8 +- 7 files changed, 285 insertions(+), 83 deletions(-) diff --git a/conf/web.xml b/conf/web.xml index dccac75..5eeb229 100644 --- a/conf/web.xml +++ b/conf/web.xml @@ -99,6 +99,14 @@ <!-- showServerInfo Should server information be presented in the --> <!-- response sent to clients when directory --> <!-- listings is enabled? [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 2c8ca97..103214d 100644 --- a/java/org/apache/catalina/servlets/DefaultServlet.java +++ b/java/org/apache/catalina/servlets/DefaultServlet.java @@ -35,6 +35,7 @@ import java.security.AccessController; import java.util.ArrayList; import java.util.Iterator; import java.util.Locale; +import java.util.Set; import java.util.StringTokenizer; import javax.naming.InitialContext; @@ -75,6 +76,7 @@ import org.apache.naming.resources.CacheEntry; import org.apache.naming.resources.ProxyDirContext; import org.apache.naming.resources.Resource; import org.apache.naming.resources.ResourceAttributes; +import org.apache.tomcat.util.http.parser.EntityTag; import org.apache.tomcat.util.res.StringManager; import org.apache.tomcat.util.security.PrivilegedGetTccl; import org.apache.tomcat.util.security.PrivilegedSetTccl; @@ -265,6 +267,8 @@ public class DefaultServlet extends HttpServlet { */ protected boolean showServerInfo = true; + protected boolean useWeakComparisonWithIfMatch = true; + // --------------------------------------------------------- Public Methods @@ -283,23 +287,27 @@ public class DefaultServlet extends HttpServlet { @Override public void init() throws ServletException { - if (getServletConfig().getInitParameter("debug") != null) + if (getServletConfig().getInitParameter("debug") != null) { debug = Integer.parseInt(getServletConfig().getInitParameter("debug")); + } - if (getServletConfig().getInitParameter("input") != null) + if (getServletConfig().getInitParameter("input") != null) { input = Integer.parseInt(getServletConfig().getInitParameter("input")); + } - if (getServletConfig().getInitParameter("output") != null) + if (getServletConfig().getInitParameter("output") != null) { output = Integer.parseInt(getServletConfig().getInitParameter("output")); + } listings = Boolean.parseBoolean(getServletConfig().getInitParameter("listings")); - if (getServletConfig().getInitParameter("readonly") != null) + if (getServletConfig().getInitParameter("readonly") != null) { readOnly = Boolean.parseBoolean(getServletConfig().getInitParameter("readonly")); + } - if (getServletConfig().getInitParameter("sendfileSize") != null) - sendfileSize = - Integer.parseInt(getServletConfig().getInitParameter("sendfileSize")) * 1024; + if (getServletConfig().getInitParameter("sendfileSize") != null) { + sendfileSize = Integer.parseInt(getServletConfig().getInitParameter("sendfileSize")) * 1024; + } fileEncoding = getServletConfig().getInitParameter("fileEncoding"); @@ -308,28 +316,32 @@ public class DefaultServlet extends HttpServlet { localXsltFile = getServletConfig().getInitParameter("localXsltFile"); readmeFile = getServletConfig().getInitParameter("readmeFile"); - if (getServletConfig().getInitParameter("useAcceptRanges") != null) + if (getServletConfig().getInitParameter("useAcceptRanges") != null) { 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) + if (input < 256) { input = 256; - if (output < 256) + } + if (output < 256) { output = 256; + } if (debug > 0) { - log("DefaultServlet.init: input buffer size=" + input + - ", output buffer size=" + output); + log("DefaultServlet.init: input buffer size=" + input + ", output buffer size=" + output); } // Load the proxy dir context. - resources = (ProxyDirContext) getServletContext() - .getAttribute(Globals.RESOURCES_ATTR); + resources = (ProxyDirContext) getServletContext().getAttribute(Globals.RESOURCES_ATTR); if (resources == null) { try { - resources = - (ProxyDirContext) new InitialContext() - .lookup(RESOURCES_JNDI_NAME); + resources = (ProxyDirContext) new InitialContext().lookup(RESOURCES_JNDI_NAME); } catch (NamingException e) { // Failed throw new ServletException("No resources", e); @@ -1917,30 +1929,22 @@ public class DefaultServlet extends HttpServlet { eTag = eTag.substring(2); } String headerValue = request.getHeader("If-Match"); - if (headerValue != null) { - if (headerValue.indexOf('*') == -1) { - - StringTokenizer commaTokenizer = new StringTokenizer(headerValue, ","); - boolean conditionSatisfied = false; + if (headerValue != null && !headerValue.equals("*")) { - while (!conditionSatisfied && commaTokenizer.hasMoreTokens()) { - String currentToken = commaTokenizer.nextToken(); - currentToken = currentToken.trim(); - if (currentToken.startsWith("W/")) { - currentToken = currentToken.substring(2); - } - if (currentToken.equals(eTag)) - conditionSatisfied = true; - } - - // If none of the given ETags match, 412 Precondition failed is - // sent back - if (!conditionSatisfied) { - response.sendError - (HttpServletResponse.SC_PRECONDITION_FAILED); - return false; + Set<String> eTags = EntityTag.parseEntityTag(new StringReader(headerValue), useWeakComparisonWithIfMatch); + if (eTags == null) { + if (debug > 10) { + log("DefaultServlet.checkIfMatch: Invalid header value [" + headerValue + "]"); } + response.sendError(HttpServletResponse.SC_BAD_REQUEST); + return false; + } + // If none of the given ETags match, 412 Precondition failed is + // sent back + if (!eTags.contains(eTag)) { + response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED); + return false; } } return true; @@ -1999,46 +2003,48 @@ public class DefaultServlet extends HttpServlet { throws IOException { String eTag = generateETag(resourceAttributes); + // 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; if (!headerValue.equals("*")) { - - StringTokenizer commaTokenizer = - new StringTokenizer(headerValue, ","); - - while (!conditionSatisfied && commaTokenizer.hasMoreTokens()) { - String currentToken = commaTokenizer.nextToken(); - if (currentToken.trim().equals(eTag)) - conditionSatisfied = true; + Set<String> eTags = EntityTag.parseEntityTag(new StringReader(headerValue), true); + if (eTags == null) { + if (debug > 10) { + log("DefaultServlet.checkIfNoneMatch: Invalid header value [" + headerValue + "]"); + } + response.sendError(HttpServletResponse.SC_BAD_REQUEST); + return false; } - + conditionSatisfied = eTags.contains(eTag); } else { conditionSatisfied = true; } if (conditionSatisfied) { - // For GET and HEAD, we should respond with // 304 Not Modified. // For every other method, 412 Precondition Failed is sent // back. - if ( ("GET".equals(request.getMethod())) - || ("HEAD".equals(request.getMethod())) ) { + if ("GET".equals(request.getMethod()) || "HEAD".equals(request.getMethod())) { response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); response.setHeader("ETag", eTag); - - return false; + } else { + response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED); } - response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED); return false; } } return true; } + /** * Check if the if-unmodified-since condition is satisfied. * @@ -2075,7 +2081,9 @@ public class DefaultServlet extends HttpServlet { * Provides the entity tag (the ETag header) for the given resource * attributes. Intended to be over-ridden by custom DefaultServlet * implementations that wish to use an alternative format for the entity - * tag. + * tag. Such custom implementations that generate strong entity tags may + * also want to change the default value of + * {@link #useWeakComparisonWithIfMatch}. * * @param resourceAttributes The resource attributes for which an entity * tag is required. diff --git a/java/org/apache/tomcat/util/http/parser/EntityTag.java b/java/org/apache/tomcat/util/http/parser/EntityTag.java new file mode 100644 index 0000000..abab6b7 --- /dev/null +++ b/java/org/apache/tomcat/util/http/parser/EntityTag.java @@ -0,0 +1,82 @@ +/* + * 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 org.apache.tomcat.util.http.parser; + +import java.io.IOException; +import java.io.StringReader; +import java.util.HashSet; +import java.util.Set; + +public class EntityTag { + + /** + * + * @param input + * @param includeWeak + * + * @return The set of parsed entity tags or {@code null} if the header is + * invalid + * + * @throws IOException + */ + public static Set<String> parseEntityTag(StringReader input, boolean includeWeak) throws IOException { + + HashSet<String> result = new HashSet<>(); + + while (true) { + boolean strong = false; + HttpParser.skipLws(input); + + switch (HttpParser.skipConstant(input, "W/")) { + case EOF: + // Empty values are invalid + return null; + case NOT_FOUND: + strong = true; + break; + case FOUND: + strong = false; + break; + } + + // Note: RFC 2616 allowed quoted string + // RFC 7232 does not allow " in the entity-tag + String value = HttpParser.readQuotedString(input, true); + if (value == null) { + // Not a quoted string so the header is invalid + return null; + } + + if (strong || includeWeak) { + result.add(value); + } + + HttpParser.skipLws(input); + + switch (HttpParser.skipConstant(input, ",")) { + case EOF: + return result; + case NOT_FOUND: + // Not EOF and not "," so must be invalid + return null; + case FOUND: + // Parse next entry + break; + } + } + } +} diff --git a/java/org/apache/tomcat/util/http/parser/HttpParser.java b/java/org/apache/tomcat/util/http/parser/HttpParser.java index 962b5f7..32a8272 100644 --- a/java/org/apache/tomcat/util/http/parser/HttpParser.java +++ b/java/org/apache/tomcat/util/http/parser/HttpParser.java @@ -519,7 +519,7 @@ public class HttpParser { // Skip any LWS and position to read the next character. The next character // is returned as being able to 'peek()' it allows a small optimisation in // some cases. - private static int skipLws(Reader input) throws IOException { + static int skipLws(Reader input) throws IOException { input.mark(1); int c = input.read(); @@ -588,7 +588,7 @@ public class HttpParser { * quoted string was found or null if the end of data was reached * before the quoted string was terminated */ - private static String readQuotedString(Reader input, boolean returnQuoted) throws IOException { + static String readQuotedString(Reader input, boolean returnQuoted) throws IOException { skipLws(input); int c = input.read(); diff --git a/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java b/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java index 362ca8e..baf9eb4 100644 --- a/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java +++ b/test/org/apache/catalina/servlets/TestDefaultServletIfMatchRequests.java @@ -19,6 +19,7 @@ package org.apache.catalina.servlets; import java.io.File; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -32,57 +33,132 @@ import org.junit.runners.Parameterized.Parameter; import org.apache.catalina.Context; import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.TomcatBaseTest; +import org.apache.naming.resources.ResourceAttributes; import org.apache.tomcat.util.buf.ByteChunk; @RunWith(Parameterized.class) public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { - @Parameterized.Parameters(name = "{index} ifMatchHeader [{0}]") + private static final Integer RC_200 = Integer.valueOf(200); + private static final Integer RC_304 = Integer.valueOf(304); + private static final Integer RC_400 = Integer.valueOf(400); + private static final Integer RC_412 = Integer.valueOf(412); + + private static final String[] CONCAT = new String[] { ",", " ,", ", ", " , " }; + + @Parameterized.Parameters(name = "{index} resource-strong [{0}], matchHeader [{1}]") public static Collection<Object[]> parameters() { // 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 strongETag = "\"" + index.length() + "-" + index.lastModified() + "\""; - String weakETag = "W/" + strongETag; + String resourceETagStrong = "\"" + index.length() + "-" + index.lastModified() + "\""; + String resourceETagWeak = "W/" + resourceETagStrong; + + String otherETagStrong = "\"123456789\""; + String otherETagWeak = "\"123456789\""; List<Object[]> parameterSets = new ArrayList<Object[]>(); - parameterSets.add(new Object[] { null, Integer.valueOf(200) }); - parameterSets.add(new Object[] { "*", Integer.valueOf(200) }); - parameterSets.add(new Object[] { weakETag, Integer.valueOf(200) }); - parameterSets.add(new Object[] { strongETag, Integer.valueOf(200) }); - parameterSets.add(new Object[] { weakETag + ",123456789", Integer.valueOf(200) }); - parameterSets.add(new Object[] { strongETag + ",123456789", Integer.valueOf(200) }); - parameterSets.add(new Object[] { weakETag + " ,123456789", Integer.valueOf(200) }); - parameterSets.add(new Object[] { strongETag + " ,123456789", Integer.valueOf(200) }); - parameterSets.add(new Object[] { "123456789," + weakETag, Integer.valueOf(200) }); - parameterSets.add(new Object[] { "123456789," + strongETag, Integer.valueOf(200) }); - parameterSets.add(new Object[] { "123456789, " + weakETag, Integer.valueOf(200) }); - parameterSets.add(new Object[] { "123456789, " + strongETag, Integer.valueOf(200) }); - parameterSets.add(new Object[] { "123456789", Integer.valueOf(412) }); - parameterSets.add(new Object[] { "W/123456789", Integer.valueOf(412) }); - parameterSets.add(new Object[] { "W/", Integer.valueOf(412) }); + for (Boolean resourceWithStrongETag : booleans) { + // No match header + parameterSets.add(new Object[] { resourceWithStrongETag, null, RC_200, RC_200 }); + + // match header is invalid + parameterSets.add(new Object[] { resourceWithStrongETag, "", RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, "W", RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, "W/", RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, "w/" + resourceETagStrong, RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + " x", RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + "x", RC_400, RC_400 }); + for (String concat : CONCAT) { + parameterSets.add(new Object[] { resourceWithStrongETag, concat + resourceETagStrong, RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, concat + resourceETagWeak, RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + concat, RC_400, RC_400 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak + concat, RC_400, RC_400 }); + } + + // match header always matches resource (leading and trailing space should be ignored) + parameterSets.add(new Object[] { resourceWithStrongETag, "*", RC_200, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, " *", RC_200, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, "* ", RC_200, RC_304 }); + + // match header never matches resource + parameterSets.add(new Object[] { resourceWithStrongETag, otherETagStrong, RC_412, RC_200 }); + 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 }); + for (String concat : CONCAT) { + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak + concat + otherETagWeak, + rcWeak, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagWeak + concat + otherETagStrong, + rcWeak, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, otherETagWeak + concat + resourceETagWeak, + rcWeak, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, otherETagStrong + concat + resourceETagWeak, + rcWeak, RC_304 }); + } + + // match header includes strong tag + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong, RC_200, RC_304 }); + for (String concat : CONCAT) { + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + concat + otherETagWeak, + RC_200, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, resourceETagStrong + concat + otherETagStrong, + RC_200, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, otherETagWeak + concat + resourceETagStrong, + RC_200, RC_304 }); + parameterSets.add(new Object[] { resourceWithStrongETag, otherETagStrong + concat + resourceETagStrong, + RC_200, RC_304 }); + } + } return parameterSets; } @Parameter(0) - public String ifMatchHeader; + public boolean resourceHasStrongETag; @Parameter(1) - public int responseCodeExpected; + public String matchHeader; + + @Parameter(2) + public int ifMatchResponseCode; + @Parameter(3) + public int ifNoneMatchResponseCode; @Test public void testIfMatch() throws Exception { + doMatchTest("If-Match", ifMatchResponseCode); + } + + + @Test + public void testIfNoneMatch() throws Exception { + doMatchTest("If-None-Match", ifNoneMatchResponseCode); + } + + private void doMatchTest(String headerName, int responseCodeExpected) throws Exception { Tomcat tomcat = getTomcatInstance(); File appDir = new File("test/webapp"); Context ctxt = tomcat.addContext("", appDir.getAbsolutePath()); - Tomcat.addServlet(ctxt, "default", DefaultServlet.class.getName()); + if (resourceHasStrongETag) { + Tomcat.addServlet(ctxt, "default", DefaultWithStrongETag.class.getName()); + } else { + Tomcat.addServlet(ctxt, "default", DefaultServlet.class.getName()); + } ctxt.addServletMapping("/", "default"); tomcat.start(); @@ -93,11 +169,9 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { Map<String,List<String>> responseHeaders = new HashMap<String,List<String>>(); Map<String,List<String>> requestHeaders = null; - if (ifMatchHeader != null) { - requestHeaders = new HashMap<String,List<String>>(); - List<String> values = new ArrayList<String>(1); - values.add(ifMatchHeader); - requestHeaders.put("If-Match", values); + if (matchHeader != null) { + List<String> values = Collections.singletonList(matchHeader); + requestHeaders = Collections.singletonMap(headerName, values); } int rc = getUrl(path, responseBody, requestHeaders, responseHeaders); @@ -105,4 +179,21 @@ public class TestDefaultServletIfMatchRequests extends TomcatBaseTest { // Check the result Assert.assertEquals(responseCodeExpected, rc); } + + + public static class DefaultWithStrongETag extends DefaultServlet { + + private static final long serialVersionUID = 1L; + + public DefaultWithStrongETag() { + useWeakComparisonWithIfMatch = false; + } + + @Override + protected String generateETag(ResourceAttributes resource) { + String weakETag = super.generateETag(resource); + // Make it a strong ETag + return weakETag.substring(2); + } + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 8e71230..9d5dc7d 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -72,6 +72,13 @@ overridden (<code>generateETag()</code>) should a custom entity tag format be required. (markt) </add> + <fix> + Improve the validation of entity tags provided with conditional + 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) + </fix> </changelog> </subsection> <subsection name="Coyote"> diff --git a/webapps/docs/default-servlet.xml b/webapps/docs/default-servlet.xml index a705bac..9f95c23 100644 --- a/webapps/docs/default-servlet.xml +++ b/webapps/docs/default-servlet.xml @@ -81,7 +81,7 @@ directory listings are disabled and debugging is turned off. <property name="debug"> Debugging level. It is not very useful unless you are a tomcat developer. As - of this writing, useful values are 0, 1, 11, 1000. [0] + of this writing, useful values are 0, 1, 11. [0] </property> <property name="listings"> If no welcome file is present, can a directory listing be @@ -160,6 +160,12 @@ directory listings are disabled and debugging is turned off. Should server information be presented in the response sent to clients when directory listing is enabled. [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: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org