You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/07/01 20:16:50 UTC
[tomcat] 05/05: Improve parsing of Content-Range headers
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org