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
commit d1f58003a97af79df452cdbe5e94052acc4b7188 Author: Mark Thomas <ma...@apache.org> AuthorDate: Mon Jul 1 21:16:15 2019 +0100 Improve parsing of Content-Range headers --- .../apache/catalina/servlets/DefaultServlet.java | 57 +++---- .../tomcat/util/http/parser/ContentRange.java | 108 ++++++++++++ .../org/apache/tomcat/util/http/parser/Ranges.java | 2 +- .../catalina/servlets/TestDefaultServletPut.java | 185 +++++++++++++++++++++ .../apache/catalina/startup/SimpleHttpClient.java | 10 ++ webapps/docs/changelog.xml | 9 +- 6 files changed, 339 insertions(+), 32 deletions(-) diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java b/java/org/apache/catalina/servlets/DefaultServlet.java index a87f4ce..c217cf6 100644 --- a/java/org/apache/catalina/servlets/DefaultServlet.java +++ b/java/org/apache/catalina/servlets/DefaultServlet.java @@ -80,6 +80,7 @@ import org.apache.catalina.util.URLEncoder; import org.apache.catalina.webresources.CachedResource; import org.apache.tomcat.util.buf.B2CConverter; import org.apache.tomcat.util.http.ResponseUtil; +import org.apache.tomcat.util.http.parser.ContentRange; import org.apache.tomcat.util.http.parser.Ranges; import org.apache.tomcat.util.res.StringManager; import org.apache.tomcat.util.security.Escape; @@ -151,6 +152,8 @@ public class DefaultServlet extends HttpServlet { */ protected static final ArrayList<Range> FULL = new ArrayList<>(); + private static final Range IGNORE = new Range(); + /** * MIME multipart separation string */ @@ -612,6 +615,11 @@ public class DefaultServlet extends HttpServlet { Range range = parseContentRange(req, resp); + if (range == null) { + // Processing error. parseContentRange() set the error code + return; + } + InputStream resourceInputStream = null; try { @@ -619,11 +627,11 @@ public class DefaultServlet extends HttpServlet { // resource - create a temp. file on the local filesystem to // perform this operation // Assume just one range is specified for now - if (range != null) { + if (range == IGNORE) { + resourceInputStream = req.getInputStream(); + } else { File contentFile = executePartialPut(req, range, path); resourceInputStream = new FileInputStream(contentFile); - } else { - resourceInputStream = req.getInputStream(); } if (resources.write(path, resourceInputStream, true)) { @@ -1383,7 +1391,9 @@ public class DefaultServlet extends HttpServlet { * * @param request The servlet request we are processing * @param response The servlet response we are creating - * @return the range object + * @return the partial content-range, {@code null} if the content-range + * header was invalid or {@code #IGNORE} if there is no header to + * process * @throws IOException an IO error occurred */ protected Range parseContentRange(HttpServletRequest request, @@ -1391,45 +1401,37 @@ public class DefaultServlet extends HttpServlet { throws IOException { // Retrieving the content-range header (if any is specified - String rangeHeader = request.getHeader("Content-Range"); + String contentRangeHeader = request.getHeader("Content-Range"); - if (rangeHeader == null || !allowPartialPut) { - return null; + if (contentRangeHeader == null) { + return IGNORE; } - // bytes is the only range unit supported - if (!rangeHeader.startsWith("bytes")) { + if (!allowPartialPut) { response.sendError(HttpServletResponse.SC_BAD_REQUEST); return null; } - rangeHeader = rangeHeader.substring(6).trim(); - - int dashPos = rangeHeader.indexOf('-'); - int slashPos = rangeHeader.indexOf('/'); + ContentRange contentRange = ContentRange.parse(new StringReader(contentRangeHeader)); - if (dashPos == -1) { + if (contentRange == null) { response.sendError(HttpServletResponse.SC_BAD_REQUEST); return null; } - if (slashPos == -1) { + + // bytes is the only range unit supported + if (!contentRange.getUnits().equals("bytes")) { response.sendError(HttpServletResponse.SC_BAD_REQUEST); return null; } + // TODO: Remove the internal representation and use Ranges + // Convert to internal representation Range range = new Range(); - - try { - range.start = Long.parseLong(rangeHeader.substring(0, dashPos)); - range.end = - Long.parseLong(rangeHeader.substring(dashPos + 1, slashPos)); - range.length = Long.parseLong - (rangeHeader.substring(slashPos + 1, rangeHeader.length())); - } catch (NumberFormatException e) { - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return null; - } + range.start = contentRange.getStart(); + range.end = contentRange.getEnd(); + range.length = contentRange.getLength(); if (!range.validate()) { response.sendError(HttpServletResponse.SC_BAD_REQUEST); @@ -1437,7 +1439,6 @@ public class DefaultServlet extends HttpServlet { } return range; - } @@ -1506,7 +1507,7 @@ public class DefaultServlet extends HttpServlet { return FULL; } - Ranges ranges = Ranges.parseRanges(new StringReader(rangeHeader)); + Ranges ranges = Ranges.parse(new StringReader(rangeHeader)); if (ranges == null) { // The Range header is present but not formatted correctly. diff --git a/java/org/apache/tomcat/util/http/parser/ContentRange.java b/java/org/apache/tomcat/util/http/parser/ContentRange.java new file mode 100644 index 0000000..59bf071 --- /dev/null +++ b/java/org/apache/tomcat/util/http/parser/ContentRange.java @@ -0,0 +1,108 @@ +/* + * 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; + +public class ContentRange { + + private final String units; + private final long start; + private final long end; + private final long length; + + + public ContentRange(String units, long start, long end, long length) { + this.units = units; + this.start = start; + this.end = end; + this.length = length; + } + + + public String getUnits() { + return units; + } + + + public long getStart() { + return start; + } + + + public long getEnd() { + return end; + } + + + public long getLength() { + return length; + } + + + /** + * Parses a Content-Range header from an HTTP header. + * + * @param input a reader over the header text + * + * @return the range parsed from the input, or null if not valid + * + * @throws IOException if there was a problem reading the input + */ + public static ContentRange parse(StringReader input) throws IOException { + // Units (required) + String units = HttpParser.readToken(input); + if (units == null || units.length() == 0) { + return null; + } + + // Must be followed by '=' + if (HttpParser.skipConstant(input, "=") == SkipResult.NOT_FOUND) { + return null; + } + + // Start + long start = HttpParser.readLong(input); + + // Must be followed by '-' + if (HttpParser.skipConstant(input, "-") == SkipResult.NOT_FOUND) { + return null; + } + + // End + long end = HttpParser.readLong(input); + + // Must be followed by '/' + if (HttpParser.skipConstant(input, "/") == SkipResult.NOT_FOUND) { + return null; + } + + // Length + long length = HttpParser.readLong(input); + + // Doesn't matter what we look for, result should be EOF + SkipResult skipResult = HttpParser.skipConstant(input, "X"); + + if (skipResult != SkipResult.EOF) { + // Invalid range + return null; + } + + return new ContentRange(units, start, end, length); + } +} diff --git a/java/org/apache/tomcat/util/http/parser/Ranges.java b/java/org/apache/tomcat/util/http/parser/Ranges.java index 1921d87..c937eb5 100644 --- a/java/org/apache/tomcat/util/http/parser/Ranges.java +++ b/java/org/apache/tomcat/util/http/parser/Ranges.java @@ -75,7 +75,7 @@ public class Ranges { * * @throws IOException if there was a problem reading the input */ - public static Ranges parseRanges(StringReader input) throws IOException { + public static Ranges parse(StringReader input) throws IOException { // Units (required) String units = HttpParser.readToken(input); diff --git a/test/org/apache/catalina/servlets/TestDefaultServletPut.java b/test/org/apache/catalina/servlets/TestDefaultServletPut.java new file mode 100644 index 0000000..915c448 --- /dev/null +++ b/test/org/apache/catalina/servlets/TestDefaultServletPut.java @@ -0,0 +1,185 @@ +/* + * 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.catalina.servlets; + +import java.io.File; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; + +import static org.apache.catalina.startup.SimpleHttpClient.CRLF; +import org.apache.catalina.Context; +import org.apache.catalina.Wrapper; +import org.apache.catalina.startup.ExpandWar; +import org.apache.catalina.startup.SimpleHttpClient; +import org.apache.catalina.startup.Tomcat; +import org.apache.catalina.startup.TomcatBaseTest; +import org.apache.tomcat.util.buf.ByteChunk; + +@RunWith(Parameterized.class) +public class TestDefaultServletPut extends TomcatBaseTest { + + private static final String START_TEXT= "Starting text"; + private static final String START_LEN = Integer.toString(START_TEXT.length()); + private static final String PATCH_TEXT= "Ending *"; + private static final String PATCH_LEN = Integer.toString(PATCH_TEXT.length()); + private static final String END_TEXT= "Ending * text"; + + @Parameterized.Parameters(name = "{index} rangeHeader [{0}]") + public static Collection<Object[]> parameters() { + List<Object[]> parameterSets = new ArrayList<>(); + + // Valid partial PUT + parameterSets.add(new Object[] { + "Content-Range: bytes=0-" + PATCH_LEN + "/" + START_LEN + CRLF, Boolean.TRUE, END_TEXT }); + // Full PUT + parameterSets.add(new Object[] { + "", null, PATCH_TEXT }); + // Invalid range + parameterSets.add(new Object[] { + "Content-Range: apples=0-" + PATCH_LEN + "/" + START_LEN + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes00-" + PATCH_LEN + "/" + START_LEN + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes=9-7/" + START_LEN + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes=-7/" + START_LEN + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes=9-/" + START_LEN + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes=9-X/" + START_LEN + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes=0-5/" + CRLF, Boolean.FALSE, START_TEXT }); + parameterSets.add(new Object[] { + "Content-Range: bytes=0-5/0x5" + CRLF, Boolean.FALSE, START_TEXT }); + + return parameterSets; + } + + + private File tempDocBase; + + @Parameter(0) + public String contentRangeHeader; + + @Parameter(1) + public Boolean contentRangeHeaderValid; + + @Parameter(2) + public String expectedEndText; + + @Override + public void setUp() throws Exception { + super.setUp(); + tempDocBase = Files.createTempDirectory(getTemporaryDirectory().toPath(), "put").toFile(); + } + + + /* + * Replaces the text at the start of START_TEXT with PATCH_TEXT. + */ + @Test + public void testPut() throws Exception { + // Configure a web app with a read/write default servlet + Tomcat tomcat = getTomcatInstance(); + Context ctxt = tomcat.addContext("", tempDocBase.getAbsolutePath()); + + Wrapper w = Tomcat.addServlet(ctxt, "default", DefaultServlet.class.getName()); + w.addInitParameter("readonly", "false"); + ctxt.addServletMappingDecoded("/", "default"); + + tomcat.start(); + + // Disable caching + ctxt.getResources().setCachingAllowed(false); + + // Full PUT + PutClient putClient = new PutClient(getPort()); + + putClient.setRequest(new String[] { + "PUT /test.txt HTTP/1.1" + CRLF + + "Host: localhost:" + getPort() + CRLF + + "Content-Length: " + START_LEN + CRLF + + CRLF + + START_TEXT + }); + putClient.connect(); + putClient.processRequest(false); + Assert.assertTrue(putClient.isResponse201()); + putClient.disconnect(); + + putClient.reset(); + + // Partial PUT + putClient.connect(); + putClient.setRequest(new String[] { + "PUT /test.txt HTTP/1.1" + CRLF + + "Host: localhost:" + getPort() + CRLF + + contentRangeHeader + + "Content-Length: " + PATCH_LEN + CRLF + + CRLF + + PATCH_TEXT + }); + putClient.processRequest(false); + if (contentRangeHeaderValid == null) { + // Not present (so will do a full PUT, replacing the existing) + Assert.assertTrue(putClient.isResponse204()); + } else if (contentRangeHeaderValid.booleanValue()) { + // Valid + Assert.assertTrue(putClient.isResponse204()); + } else { + // Not valid + Assert.assertTrue(putClient.isResponse400()); + } + + // Check for the final resource + String path = "http://localhost:" + getPort() + "/test.txt"; + ByteChunk responseBody = new ByteChunk(); + + int rc = getUrl(path, responseBody, null); + + Assert.assertEquals(200, rc); + Assert.assertEquals(expectedEndText, responseBody.toString()); + } + + + @Override + public void tearDown() { + ExpandWar.deleteDir(tempDocBase, false); + } + + + private static class PutClient extends SimpleHttpClient { + + public PutClient(int port) { + setPort(port); + } + + + @Override + public boolean isResponseBodyOK() { + return false; + } + } +} diff --git a/test/org/apache/catalina/startup/SimpleHttpClient.java b/test/org/apache/catalina/startup/SimpleHttpClient.java index af2c69d..774d955 100644 --- a/test/org/apache/catalina/startup/SimpleHttpClient.java +++ b/test/org/apache/catalina/startup/SimpleHttpClient.java @@ -50,6 +50,8 @@ public abstract class SimpleHttpClient { public static final String INFO_100 = "HTTP/1.1 100 "; public static final String OK_200 = "HTTP/1.1 200 "; + public static final String CREATED_201 = "HTTP/1.1 201 "; + public static final String NOCONTENT_204 = "HTTP/1.1 204 "; public static final String REDIRECT_302 = "HTTP/1.1 302 "; public static final String REDIRECT_303 = "HTTP/1.1 303 "; public static final String FAIL_400 = "HTTP/1.1 400 "; @@ -414,6 +416,14 @@ public abstract class SimpleHttpClient { return responseLineStartsWith(OK_200); } + public boolean isResponse201() { + return responseLineStartsWith(CREATED_201); + } + + public boolean isResponse204() { + return responseLineStartsWith(NOCONTENT_204); + } + public boolean isResponse302() { return responseLineStartsWith(REDIRECT_302); } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 14ab5f8..e8a582c 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -61,9 +61,12 @@ </fix> <fix> Add an option to the default servlet to disable processing of PUT - requests with Range headers as partial PUTs. The default behaviour - (processing as partial PUT) is unchanged. Based on a pull request by - zhanhb. (markt) + requests with Content-Range headers as partial PUTs. The default + behaviour (processing as partial PUT) is unchanged. Based on a pull + request by zhanhb. (markt) + </fix> + <fix> + Improve parsing of Content-Range headers. (markt) </fix> </changelog> </subsection> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org