From 36b41ec17abefe2c433a0b7164e533bedbca181e Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Wed, 25 Nov 2020 13:54:36 +0200 Subject: [PATCH] fix: optimize handling of requests containing Range header (#9484) More efficient parsing of the header value. Also, range count is capped at 16 (additional will be ignored) and overlapping ranges at 2 (request will be denied if above). --- .../vaadin/flow/internal/ResponseWriter.java | 62 ++++++++++++++++--- .../flow/internal/ResponseWriterTest.java | 49 +++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/ResponseWriter.java b/flow-server/src/main/java/com/vaadin/flow/internal/ResponseWriter.java index df8e9720b08..f27f4c5cfa4 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/ResponseWriter.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/ResponseWriter.java @@ -29,8 +29,8 @@ import java.net.MalformedURLException; import java.net.URL; import java.net.URLConnection; -import java.util.ArrayList; import java.util.List; +import java.util.Stack; import java.util.UUID; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -51,8 +51,20 @@ public class ResponseWriter implements Serializable { private static final int DEFAULT_BUFFER_SIZE = 32 * 1024; - private static final Pattern RANGE_HEADER_PATTERN = Pattern.compile("^bytes=(([0-9]*-[0-9]*,?\\s*)+)$"); - private static final Pattern BYTE_RANGE_PATTERN = Pattern.compile("([0-9]*)-([0-9]*)"); + private static final Pattern RANGE_HEADER_PATTERN = Pattern.compile( + "^bytes=((\\d*-\\d*\\s*,\\s*)*\\d*-\\d*\\s*)$"); + private static final Pattern BYTE_RANGE_PATTERN = Pattern.compile( + "(\\d*)-(\\d*)"); + + /** + * Maximum number of ranges accepted in a single Range header. Remaining ranges will be ignored. + */ + private static final int MAX_RANGE_COUNT = 16; + + /** + * Maximum number of overlapping ranges allowed. The request will be denied if above this threshold. + */ + private static final int MAX_OVERLAPPING_RANGE_COUNT = 2; private final int bufferSize; private final boolean brotliEnabled; @@ -226,13 +238,14 @@ private void writeRangeContents(String range, HttpServletResponse response, long resourceLength = connection.getContentLengthLong(); Matcher rangeMatcher = BYTE_RANGE_PATTERN.matcher(byteRanges); - List> ranges = new ArrayList<>(); - while (rangeMatcher.find()) { + Stack> ranges = new Stack<>(); + while (rangeMatcher.find() && ranges.size() < MAX_RANGE_COUNT) { String startGroup = rangeMatcher.group(1); String endGroup = rangeMatcher.group(2); if (startGroup.isEmpty() && endGroup.isEmpty()) { response.setContentLengthLong(0L); response.setStatus(416); // Range Not Satisfiable + getLogger().info("received a malformed range: '{}'", rangeMatcher.group()); return; } long start = startGroup.isEmpty() ? 0L : Long.parseLong(startGroup); @@ -241,11 +254,20 @@ private void writeRangeContents(String range, HttpServletResponse response, if (end < start || (resourceLength >= 0 && start >= resourceLength)) { // illegal range -> 416 + getLogger().info("received an illegal range '{}' for resource '{}'", + rangeMatcher.group(), resourceURL); response.setContentLengthLong(0L); response.setStatus(416); return; } - ranges.add(new Pair<>(start, end)); + ranges.push(new Pair<>(start, end)); + + if (!verifyRangeLimits(ranges)) { + ranges.pop(); + getLogger().info("serving only {} ranges for resource '{}' even though more were requested", + ranges.size(), resourceURL); + break; + } } response.setStatus(206); @@ -344,7 +366,33 @@ private void setContentLength(HttpServletResponse response, } } - private URL getResource(HttpServletRequest request, String resource ) + /** + * Returns true if the number of ranges in ranges is less than the + * upper limit and the number that overlap (= have at least one byte in common) + * with the range [start, end] are less than the upper limit. + */ + private boolean verifyRangeLimits(List> ranges) { + if (ranges.size() > MAX_RANGE_COUNT) { + getLogger().info("more than {} ranges requested", MAX_RANGE_COUNT); + return false; + } + int count = 0; + for (int i = 0; i < ranges.size(); i++) { + for (int j = i + 1; j < ranges.size(); j++) { + if (ranges.get(i).getFirst() <= ranges.get(j).getSecond() + && ranges.get(j).getFirst() <= ranges.get(i).getSecond()) { + count++; + } + } + } + if (count > MAX_OVERLAPPING_RANGE_COUNT) { + getLogger().info("more than {} overlapping ranges requested", MAX_OVERLAPPING_RANGE_COUNT); + return false; + } + return true; + } + + private URL getResource(HttpServletRequest request, String resource) throws MalformedURLException { URL url = request.getServletContext() .getResource(resource); diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/ResponseWriterTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/ResponseWriterTest.java index 7e3a828860f..7a7262cee64 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/ResponseWriterTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/internal/ResponseWriterTest.java @@ -486,6 +486,44 @@ public void writeByteRangeMultiPartOverlapping() throws IOException { assertStatus(206); } + @Test + public void writeByteRangeMultiPartTooManyRequested() throws IOException { + makePathsAvailable(PATH_JS); + mockRequestHeaders(new Pair<>("Range", "bytes=0-0, 0-0, 1-1, 2-2, 3-3, 4-4, 5-5, 6-6, 7-7, 8-8, 9-9, 10-10, 11-11, 12-12, 13-13, 14-14, 15-15, 16-16")); + // "File.js contents" + // ^0123456789ABCDEF^ + assertMultipartResponse(PATH_JS, Arrays.asList( + new Pair<>(new String[]{"Content-Range: bytes 0-0/16"}, "F".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 0-0/16"}, "F".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 1-1/16"}, "i".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 2-2/16"}, "l".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 3-3/16"}, "e".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 4-4/16"}, ".".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 5-5/16"}, "j".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 6-6/16"}, "s".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 7-7/16"}, " ".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 8-8/16"}, "c".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 9-9/16"}, "o".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 10-10/16"}, "n".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 11-11/16"}, "t".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 12-12/16"}, "e".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 13-13/16"}, "n".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 14-14/16"}, "t".getBytes()))); + assertStatus(206); + } + + @Test + public void writeByteRangeMultiPartTooManyOverlappingRequested() throws IOException { + makePathsAvailable(PATH_JS); + mockRequestHeaders(new Pair<>("Range", "bytes=2-4, 0-4, 3-14")); + // "File.js contents" + // ^0123456789ABCDEF^ + assertMultipartResponse(PATH_JS, Arrays.asList( + new Pair<>(new String[]{"Content-Range: bytes 2-4/16"}, "le.".getBytes()), + new Pair<>(new String[]{"Content-Range: bytes 0-4/16"}, "File.".getBytes()))); + assertStatus(206); + } + private void assertResponse(byte[] expectedResponse) throws IOException { assertResponse(PATH_JS, expectedResponse); } @@ -542,6 +580,17 @@ private void assertMultipartResponse(String path, byte[] bytes = outputStream.toByteArray(); Assert.assertArrayEquals(expectedBytes, bytes); } + + // check that there are no excess parts + try { + mps.readHeaders(); + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + mps.readBodyData(outputStream); + Assert.assertTrue("excess bytes in multipart response", + outputStream.toByteArray().length == 0); + } catch (IOException ioe) { + // all is well, stream ended + } } private void assertStatus(int status) {