From f084b632864685d8e65d0cdfa719a813824fc59f Mon Sep 17 00:00:00 2001 From: Jonathan Bluett-Duncan Date: Mon, 19 Aug 2019 18:19:49 +0100 Subject: [PATCH 1/3] Fix "array index out of bounds" problem reported by LGTM.com --- .../http/ContentDisposition.java | 18 ++++++++++++----- .../http/ContentDispositionTests.java | 20 ++++++++++++++++++- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/ContentDisposition.java b/spring-web/src/main/java/org/springframework/http/ContentDisposition.java index 115aea08d5..0a245314b0 100644 --- a/spring-web/src/main/java/org/springframework/http/ContentDisposition.java +++ b/spring-web/src/main/java/org/springframework/http/ContentDisposition.java @@ -42,6 +42,9 @@ import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME; */ public final class ContentDisposition { + private static final String INVALID_HEADER_FIELD_PARAMETER_FORMAT = + "Invalid header field parameter format (as defined in RFC 5987)"; + @Nullable private final String type; @@ -357,7 +360,7 @@ public final class ContentDisposition { } /** - * Decode the given header field param as describe in RFC 5987. + * Decode the given header field param as described in RFC 5987. *

Only the US-ASCII, UTF-8 and ISO-8859-1 charsets are supported. * @param input the header field param * @return the encoded header field param @@ -383,13 +386,18 @@ public final class ContentDisposition { bos.write((char) b); index++; } - else if (b == '%') { - char[] array = { (char)value[index + 1], (char)value[index + 2]}; - bos.write(Integer.parseInt(String.valueOf(array), 16)); + else if (b == '%' && index < value.length - 2) { + char[] array = new char[]{(char) value[index + 1], (char) value[index + 2]}; + try { + bos.write(Integer.parseInt(String.valueOf(array), 16)); + } + catch (NumberFormatException ex) { + throw new IllegalArgumentException(INVALID_HEADER_FIELD_PARAMETER_FORMAT, ex); + } index+=3; } else { - throw new IllegalArgumentException("Invalid header field parameter format (as defined in RFC 5987)"); + throw new IllegalArgumentException(INVALID_HEADER_FIELD_PARAMETER_FORMAT); } } return new String(bos.toByteArray(), charset); diff --git a/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java b/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java index 13acce90a0..ddf4f8867b 100644 --- a/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java +++ b/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java @@ -28,6 +28,7 @@ import org.springframework.util.ReflectionUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Unit tests for {@link ContentDisposition} @@ -36,7 +37,6 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException */ public class ContentDispositionTests { - @Test public void parseTest() { ContentDisposition disposition = ContentDisposition @@ -198,4 +198,22 @@ public class ContentDispositionTests { ReflectionUtils.invokeMethod(decode, null, "UTF-16''test")); } + @Test + public void decodeHeaderFieldParamShortInvalidEncodedFilename() { + Method decode = ReflectionUtils.findMethod(ContentDisposition.class, + "decodeHeaderFieldParam", String.class); + ReflectionUtils.makeAccessible(decode); + assertThatIllegalArgumentException().isThrownBy(() -> + ReflectionUtils.invokeMethod(decode, null, "UTF-8''%A")); + } + + @Test + public void decodeHeaderFieldParamLongerInvalidEncodedFilename() { + Method decode = ReflectionUtils.findMethod(ContentDisposition.class, + "decodeHeaderFieldParam", String.class); + ReflectionUtils.makeAccessible(decode); + assertThatIllegalArgumentException().isThrownBy(() -> + ReflectionUtils.invokeMethod(decode, null, "UTF-8''%A.txt")); + } + } From c97580035e63393d2306e2e0dcf59d890dbd8c44 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 3 Sep 2019 11:17:32 +0100 Subject: [PATCH 2/3] Polish ContentDispositionTests Closes gh-23485 --- .../http/ContentDispositionTests.java | 174 ++++++++++-------- 1 file changed, 94 insertions(+), 80 deletions(-) diff --git a/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java b/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java index ddf4f8867b..c4b397e189 100644 --- a/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java +++ b/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java @@ -28,129 +28,143 @@ import org.springframework.util.ReflectionUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; -import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Unit tests for {@link ContentDisposition} - * * @author Sebastien Deleuze + * @author Rossen Stoyanchev */ public class ContentDispositionTests { - @Test - public void parseTest() { - ContentDisposition disposition = ContentDisposition - .parse("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123"); - assertThat(disposition).isEqualTo(ContentDisposition.builder("form-data") - .name("foo").filename("foo.txt").size(123L).build()); - } + private static DateTimeFormatter formatter = DateTimeFormatter.RFC_1123_DATE_TIME; + @Test public void parse() { - ContentDisposition disposition = ContentDisposition - .parse("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123"); - assertThat(disposition).isEqualTo(ContentDisposition.builder("form-data") - .name("foo").filename("foo.txt").size(123L).build()); + assertThat(parse("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123")) + .isEqualTo(ContentDisposition.builder("form-data") + .name("foo") + .filename("foo.txt") + .size(123L) + .build()); } @Test - public void parseType() { - ContentDisposition disposition = ContentDisposition.parse("form-data"); - assertThat(disposition).isEqualTo(ContentDisposition.builder("form-data").build()); - } - - @Test - public void parseUnquotedFilename() { - ContentDisposition disposition = ContentDisposition - .parse("form-data; filename=unquoted"); - assertThat(disposition).isEqualTo(ContentDisposition.builder("form-data").filename("unquoted").build()); + public void parseFilenameUnquoted() { + assertThat(parse("form-data; filename=unquoted")) + .isEqualTo(ContentDisposition.builder("form-data") + .filename("unquoted") + .build()); } @Test // SPR-16091 public void parseFilenameWithSemicolon() { - ContentDisposition disposition = ContentDisposition - .parse("attachment; filename=\"filename with ; semicolon.txt\""); - assertThat(disposition).isEqualTo(ContentDisposition.builder("attachment") - .filename("filename with ; semicolon.txt").build()); - } - - @Test - public void parseAndIgnoreEmptyParts() { - ContentDisposition disposition = ContentDisposition - .parse("form-data; name=\"foo\";; ; filename=\"foo.txt\"; size=123"); - assertThat(disposition).isEqualTo(ContentDisposition.builder("form-data") - .name("foo").filename("foo.txt").size(123L).build()); + assertThat(parse("attachment; filename=\"filename with ; semicolon.txt\"")) + .isEqualTo(ContentDisposition.builder("attachment") + .filename("filename with ; semicolon.txt") + .build()); } @Test public void parseEncodedFilename() { - ContentDisposition disposition = ContentDisposition - .parse("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt"); - assertThat(disposition).isEqualTo(ContentDisposition.builder("form-data").name("name") - .filename("中文.txt", StandardCharsets.UTF_8).build()); + assertThat(parse("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt")) + .isEqualTo(ContentDisposition.builder("form-data") + .name("name") + .filename("中文.txt", StandardCharsets.UTF_8) + .build()); } @Test // gh-23077 public void parseWithEscapedQuote() { - ContentDisposition disposition = ContentDisposition.parse( - "form-data; name=\"file\"; filename=\"\\\"The Twilight Zone\\\".txt\"; size=123"); - assertThat(ContentDisposition.builder("form-data").name("file") - .filename("\\\"The Twilight Zone\\\".txt").size(123L).build()).isEqualTo(disposition); + assertThat(parse("form-data; name=\"file\"; filename=\"\\\"The Twilight Zone\\\".txt\"; size=123")) + .isEqualTo(ContentDisposition.builder("form-data") + .name("file") + .filename("\\\"The Twilight Zone\\\".txt") + .size(123L) + .build()); } @Test - public void parseEmpty() { - assertThatIllegalArgumentException().isThrownBy(() -> - ContentDisposition.parse("")); - } - - @Test - public void parseNoType() { - assertThatIllegalArgumentException().isThrownBy(() -> - ContentDisposition.parse(";")); - } - - @Test - public void parseInvalidParameter() { - assertThatIllegalArgumentException().isThrownBy(() -> - ContentDisposition.parse("foo;bar")); + public void parseWithExtraSemicolons() { + assertThat(parse("form-data; name=\"foo\";; ; filename=\"foo.txt\"; size=123")) + .isEqualTo(ContentDisposition.builder("form-data") + .name("foo") + .filename("foo.txt") + .size(123L) + .build()); } @Test public void parseDates() { - ContentDisposition disposition = ContentDisposition - .parse("attachment; creation-date=\"Mon, 12 Feb 2007 10:15:30 -0500\"; " + - "modification-date=\"Tue, 13 Feb 2007 10:15:30 -0500\"; " + - "read-date=\"Wed, 14 Feb 2007 10:15:30 -0500\""); - DateTimeFormatter formatter = DateTimeFormatter.RFC_1123_DATE_TIME; - assertThat(disposition).isEqualTo(ContentDisposition.builder("attachment") - .creationDate(ZonedDateTime.parse("Mon, 12 Feb 2007 10:15:30 -0500", formatter)) - .modificationDate(ZonedDateTime.parse("Tue, 13 Feb 2007 10:15:30 -0500", formatter)) - .readDate(ZonedDateTime.parse("Wed, 14 Feb 2007 10:15:30 -0500", formatter)).build()); + ZonedDateTime creationTime = ZonedDateTime.parse("Mon, 12 Feb 2007 10:15:30 -0500", formatter); + ZonedDateTime modificationTime = ZonedDateTime.parse("Tue, 13 Feb 2007 10:15:30 -0500", formatter); + ZonedDateTime readTime = ZonedDateTime.parse("Wed, 14 Feb 2007 10:15:30 -0500", formatter); + + assertThat( + parse("attachment; " + + "creation-date=\"" + creationTime.format(formatter) + "\"; " + + "modification-date=\"" + modificationTime.format(formatter) + "\"; " + + "read-date=\"" + readTime.format(formatter) + "\"")).isEqualTo( + ContentDisposition.builder("attachment") + .creationDate(creationTime) + .modificationDate(modificationTime) + .readDate(readTime) + .build()); } @Test - public void parseInvalidDates() { - ContentDisposition disposition = ContentDisposition - .parse("attachment; creation-date=\"-1\"; modification-date=\"-1\"; " + - "read-date=\"Wed, 14 Feb 2007 10:15:30 -0500\""); - DateTimeFormatter formatter = DateTimeFormatter.RFC_1123_DATE_TIME; - assertThat(disposition).isEqualTo(ContentDisposition.builder("attachment") - .readDate(ZonedDateTime.parse("Wed, 14 Feb 2007 10:15:30 -0500", formatter)).build()); + public void parseIgnoresInvalidDates() { + ZonedDateTime readTime = ZonedDateTime.parse("Wed, 14 Feb 2007 10:15:30 -0500", formatter); + + assertThat( + parse("attachment; " + + "creation-date=\"-1\"; " + + "modification-date=\"-1\"; " + + "read-date=\"" + readTime.format(formatter) + "\"")).isEqualTo( + ContentDisposition.builder("attachment") + .readDate(readTime) + .build()); } + @Test + public void parseEmpty() { + assertThatIllegalArgumentException().isThrownBy(() -> parse("")); + } + + @Test + public void parseNoType() { + assertThatIllegalArgumentException().isThrownBy(() -> parse(";")); + } + + @Test + public void parseInvalidParameter() { + assertThatIllegalArgumentException().isThrownBy(() -> parse("foo;bar")); + } + + private static ContentDisposition parse(String input) { + return ContentDisposition.parse(input); + } + + @Test public void headerValue() { - ContentDisposition disposition = ContentDisposition.builder("form-data") - .name("foo").filename("foo.txt").size(123L).build(); - assertThat(disposition.toString()).isEqualTo("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123"); + assertThat( + ContentDisposition.builder("form-data") + .name("foo") + .filename("foo.txt") + .size(123L) + .build().toString()) + .isEqualTo("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123"); } @Test public void headerValueWithEncodedFilename() { - ContentDisposition disposition = ContentDisposition.builder("form-data") - .name("name").filename("中文.txt", StandardCharsets.UTF_8).build(); - assertThat(disposition.toString()).isEqualTo("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt"); + assertThat( + ContentDisposition.builder("form-data") + .name("name") + .filename("中文.txt", StandardCharsets.UTF_8) + .build().toString()) + .isEqualTo("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt"); } @Test // SPR-14547 From d927d31e135bcbc8b4a69ac3b3b3aab354d1670b Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 3 Sep 2019 12:30:31 +0100 Subject: [PATCH 3/3] Remove reflection from ContentDispositionTests Also minor refactoring in decoding in order to tolerate the absence of a charset and treat as US_ASCII. See gh-23485 --- .../http/ContentDisposition.java | 41 +++---- .../http/ContentDispositionTests.java | 104 +++++++----------- 2 files changed, 62 insertions(+), 83 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/ContentDisposition.java b/spring-web/src/main/java/org/springframework/http/ContentDisposition.java index 0a245314b0..37d9120057 100644 --- a/spring-web/src/main/java/org/springframework/http/ContentDisposition.java +++ b/spring-web/src/main/java/org/springframework/http/ContentDisposition.java @@ -208,7 +208,7 @@ public final class ContentDisposition { } else { sb.append("; filename*="); - sb.append(encodeHeaderFieldParam(this.filename, this.charset)); + sb.append(encodeFilename(this.filename, this.charset)); } } if (this.size != null) { @@ -274,15 +274,23 @@ public final class ContentDisposition { String attribute = part.substring(0, eqIndex); String value = (part.startsWith("\"", eqIndex + 1) && part.endsWith("\"") ? part.substring(eqIndex + 2, part.length() - 1) : - part.substring(eqIndex + 1, part.length())); + part.substring(eqIndex + 1)); if (attribute.equals("name") ) { name = value; } else if (attribute.equals("filename*") ) { - filename = decodeHeaderFieldParam(value); - charset = Charset.forName(value.substring(0, value.indexOf('\''))); - Assert.isTrue(UTF_8.equals(charset) || ISO_8859_1.equals(charset), - "Charset should be UTF-8 or ISO-8859-1"); + int idx1 = value.indexOf('\''); + int idx2 = value.indexOf('\'', idx1 + 1); + if (idx1 != -1 && idx2 != -1) { + charset = Charset.forName(value.substring(0, idx1)); + Assert.isTrue(UTF_8.equals(charset) || ISO_8859_1.equals(charset), + "Charset should be UTF-8 or ISO-8859-1"); + filename = decodeFilename(value.substring(idx2 + 1), charset); + } + else { + // US ASCII + filename = decodeFilename(value, StandardCharsets.US_ASCII); + } } else if (attribute.equals("filename") && (filename == null)) { filename = value; @@ -362,22 +370,15 @@ public final class ContentDisposition { /** * Decode the given header field param as described in RFC 5987. *

Only the US-ASCII, UTF-8 and ISO-8859-1 charsets are supported. - * @param input the header field param + * @param filename the filename + * @param charset the charset for the filename * @return the encoded header field param * @see RFC 5987 */ - private static String decodeHeaderFieldParam(String input) { - Assert.notNull(input, "Input String should not be null"); - int firstQuoteIndex = input.indexOf('\''); - int secondQuoteIndex = input.indexOf('\'', firstQuoteIndex + 1); - // US_ASCII - if (firstQuoteIndex == -1 || secondQuoteIndex == -1) { - return input; - } - Charset charset = Charset.forName(input.substring(0, firstQuoteIndex)); - Assert.isTrue(UTF_8.equals(charset) || ISO_8859_1.equals(charset), - "Charset should be UTF-8 or ISO-8859-1"); - byte[] value = input.substring(secondQuoteIndex + 1, input.length()).getBytes(charset); + private static String decodeFilename(String filename, Charset charset) { + Assert.notNull(filename, "'input' String` should not be null"); + Assert.notNull(charset, "'charset' should not be null"); + byte[] value = filename.getBytes(charset); ByteArrayOutputStream bos = new ByteArrayOutputStream(); int index = 0; while (index < value.length) { @@ -417,7 +418,7 @@ public final class ContentDisposition { * @return the encoded header field param * @see RFC 5987 */ - private static String encodeHeaderFieldParam(String input, Charset charset) { + private static String encodeFilename(String input, Charset charset) { Assert.notNull(input, "Input String should not be null"); Assert.notNull(charset, "Charset should not be null"); if (StandardCharsets.US_ASCII.equals(charset)) { diff --git a/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java b/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java index c4b397e189..a647871b0e 100644 --- a/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java +++ b/spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java @@ -16,16 +16,12 @@ package org.springframework.http; -import java.lang.reflect.Method; -import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import org.junit.jupiter.api.Test; -import org.springframework.util.ReflectionUtils; - import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -74,6 +70,30 @@ public class ContentDispositionTests { .build()); } + @Test + public void parseEncodedFilenameWithoutCharset() { + assertThat(parse("form-data; name=\"name\"; filename*=test.txt")) + .isEqualTo(ContentDisposition.builder("form-data") + .name("name") + .filename("test.txt") + .build()); + } + + @Test + public void parseEncodedFilenameWithInvalidCharset() { + assertThatIllegalArgumentException() + .isThrownBy(() -> parse("form-data; name=\"name\"; filename*=UTF-16''test.txt")); + } + + @Test + public void parseEncodedFilenameWithInvalidName() { + assertThatIllegalArgumentException() + .isThrownBy(() -> parse("form-data; name=\"name\"; filename*=UTF-8''%A")); + + assertThatIllegalArgumentException() + .isThrownBy(() -> parse("form-data; name=\"name\"; filename*=UTF-8''%A.txt")); + } + @Test // gh-23077 public void parseWithEscapedQuote() { assertThat(parse("form-data; name=\"file\"; filename=\"\\\"The Twilight Zone\\\".txt\"; size=123")) @@ -147,7 +167,7 @@ public class ContentDispositionTests { @Test - public void headerValue() { + public void format() { assertThat( ContentDisposition.builder("form-data") .name("foo") @@ -158,7 +178,7 @@ public class ContentDispositionTests { } @Test - public void headerValueWithEncodedFilename() { + public void formatWithEncodedFilename() { assertThat( ContentDisposition.builder("form-data") .name("name") @@ -167,67 +187,25 @@ public class ContentDispositionTests { .isEqualTo("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt"); } - @Test // SPR-14547 - public void encodeHeaderFieldParam() { - Method encode = ReflectionUtils.findMethod(ContentDisposition.class, - "encodeHeaderFieldParam", String.class, Charset.class); - ReflectionUtils.makeAccessible(encode); - - String result = (String)ReflectionUtils.invokeMethod(encode, null, "test.txt", - StandardCharsets.US_ASCII); - assertThat(result).isEqualTo("test.txt"); - - result = (String)ReflectionUtils.invokeMethod(encode, null, "中文.txt", StandardCharsets.UTF_8); - assertThat(result).isEqualTo("UTF-8''%E4%B8%AD%E6%96%87.txt"); + @Test + public void formatWithEncodedFilenameUsingUsAscii() { + assertThat( + ContentDisposition.builder("form-data") + .name("name") + .filename("test.txt", StandardCharsets.US_ASCII) + .build() + .toString()) + .isEqualTo("form-data; name=\"name\"; filename=\"test.txt\""); } @Test - public void encodeHeaderFieldParamInvalidCharset() { - Method encode = ReflectionUtils.findMethod(ContentDisposition.class, - "encodeHeaderFieldParam", String.class, Charset.class); - ReflectionUtils.makeAccessible(encode); + public void formatWithEncodedFilenameUsingInvalidCharset() { assertThatIllegalArgumentException().isThrownBy(() -> - ReflectionUtils.invokeMethod(encode, null, "test", StandardCharsets.UTF_16)); - } - - @Test // SPR-14408 - public void decodeHeaderFieldParam() { - Method decode = ReflectionUtils.findMethod(ContentDisposition.class, - "decodeHeaderFieldParam", String.class); - ReflectionUtils.makeAccessible(decode); - - String result = (String)ReflectionUtils.invokeMethod(decode, null, "test.txt"); - assertThat(result).isEqualTo("test.txt"); - - result = (String)ReflectionUtils.invokeMethod(decode, null, "UTF-8''%E4%B8%AD%E6%96%87.txt"); - assertThat(result).isEqualTo("中文.txt"); - } - - @Test - public void decodeHeaderFieldParamInvalidCharset() { - Method decode = ReflectionUtils.findMethod(ContentDisposition.class, - "decodeHeaderFieldParam", String.class); - ReflectionUtils.makeAccessible(decode); - assertThatIllegalArgumentException().isThrownBy(() -> - ReflectionUtils.invokeMethod(decode, null, "UTF-16''test")); - } - - @Test - public void decodeHeaderFieldParamShortInvalidEncodedFilename() { - Method decode = ReflectionUtils.findMethod(ContentDisposition.class, - "decodeHeaderFieldParam", String.class); - ReflectionUtils.makeAccessible(decode); - assertThatIllegalArgumentException().isThrownBy(() -> - ReflectionUtils.invokeMethod(decode, null, "UTF-8''%A")); - } - - @Test - public void decodeHeaderFieldParamLongerInvalidEncodedFilename() { - Method decode = ReflectionUtils.findMethod(ContentDisposition.class, - "decodeHeaderFieldParam", String.class); - ReflectionUtils.makeAccessible(decode); - assertThatIllegalArgumentException().isThrownBy(() -> - ReflectionUtils.invokeMethod(decode, null, "UTF-8''%A.txt")); + ContentDisposition.builder("form-data") + .name("name") + .filename("test.txt", StandardCharsets.UTF_16) + .build() + .toString()); } }