Escape quotes in filename

Also sync up to master and 5.1.x on refactorings in ContentDisposition
and ContentDispositionTests.

Closes gh-24230

spring-web/src/main/java/org/springframework/http/ContentDisposition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,8 +28,9 @@ import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.ObjectUtils;
 
-import static java.nio.charset.StandardCharsets.*;
-import static java.time.format.DateTimeFormatter.*;
+import static java.nio.charset.StandardCharsets.ISO_8859_1;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME;
 
 /**
  * Represent the Content-Disposition type and parameters as defined in RFC 2183.
@@ -39,7 +40,11 @@ import static java.time.format.DateTimeFormatter.*;
  * @since 5.0
  * @see <a href="https://tools.ietf.org/html/rfc2183">RFC 2183</a>
  */
-public class ContentDisposition {
+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;
@@ -200,11 +205,11 @@ public class ContentDisposition {
 		if (this.filename != null) {
 			if (this.charset == null || StandardCharsets.US_ASCII.equals(this.charset)) {
 				sb.append("; filename=\"");
-				sb.append(this.filename).append('\"');
+				sb.append(escapeQuotationsInFilename(this.filename)).append('\"');
 			}
 			else {
 				sb.append("; filename*=");
-				sb.append(encodeHeaderFieldParam(this.filename, this.charset));
+				sb.append(encodeFilename(this.filename, this.charset));
 			}
 		}
 		if (this.size != null) {
@@ -270,15 +275,23 @@ public 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).trim());
+						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;
@@ -330,6 +343,7 @@ public class ContentDisposition {
 			do {
 				int nextIndex = index + 1;
 				boolean quoted = false;
+				boolean escaped = false;
 				while (nextIndex < headerValue.length()) {
 					char ch = headerValue.charAt(nextIndex);
 					if (ch == ';') {
@@ -337,9 +351,10 @@ public class ContentDisposition {
 							break;
 						}
 					}
-					else if (ch == '"') {
+					else if (!escaped && ch == '"') {
 						quoted = !quoted;
 					}
+					escaped = (!escaped && ch == '\\');
 					nextIndex++;
 				}
 				String part = headerValue.substring(index + 1, nextIndex).trim();
@@ -356,22 +371,15 @@ public class ContentDisposition {
 	/**
 	 * Decode the given header field param as describe in RFC 5987.
 	 * <p>Only the US-ASCII, UTF-8 and ISO-8859-1 charsets are supported.
-	 * @param input the header field param
+	 * @param filename the header field param
+	 * @param charset the charset to use
 	 * @return the encoded header field param
 	 * @see <a href="https://tools.ietf.org/html/rfc5987">RFC 5987</a>
 	 */
-	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) {
@@ -380,13 +388,18 @@ public 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);
@@ -398,6 +411,23 @@ public class ContentDisposition {
 				c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~';
 	}
 
+	private static String escapeQuotationsInFilename(String filename) {
+		if (filename.indexOf('"') == -1 && filename.indexOf('\\') == -1) {
+			return filename;
+		}
+		boolean escaped = false;
+		StringBuilder sb = new StringBuilder();
+		for (char c : filename.toCharArray()) {
+			sb.append((c == '"' && !escaped) ? "\\\"" : c);
+			escaped = (!escaped && c == '\\');
+		}
+		// Remove backslash at the end..
+		if (escaped) {
+			sb.deleteCharAt(sb.length() - 1);
+		}
+		return sb.toString();
+	}
+
 	/**
 	 * Encode the given header field param as describe in RFC 5987.
 	 * @param input the header field param
@@ -406,14 +436,11 @@ public class ContentDisposition {
 	 * @return the encoded header field param
 	 * @see <a href="https://tools.ietf.org/html/rfc5987">RFC 5987</a>
 	 */
-	private static String encodeHeaderFieldParam(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)) {
-			return input;
-		}
-		Assert.isTrue(UTF_8.equals(charset) || ISO_8859_1.equals(charset),
-				"Charset should be UTF-8 or ISO-8859-1");
+	private static String encodeFilename(String input, Charset charset) {
+		Assert.notNull(input, "`input` is required");
+		Assert.notNull(charset, "`charset` is required");
+		Assert.isTrue(!StandardCharsets.US_ASCII.equals(charset), "ASCII does not require encoding");
+		Assert.isTrue(UTF_8.equals(charset) || ISO_8859_1.equals(charset), "Only UTF-8 and ISO-8859-1 supported.");
 		byte[] source = input.getBytes(charset);
 		int len = source.length;
 		StringBuilder sb = new StringBuilder(len << 1);
@@ -446,7 +473,11 @@ public class ContentDisposition {
 		Builder name(String name);
 
 		/**
-		 * Set the value of the {@literal filename} parameter.
+		 * Set the value of the {@literal filename} parameter. The given
+		 * filename will be formatted as quoted-string, as defined in RFC 2616,
+		 * section 2.2, and any quote characters within the filename value will
+		 * be escaped with a backslash, e.g. {@code "foo\"bar.txt"} becomes
+		 * {@code "foo\\\"bar.txt"}.
 		 */
 		Builder filename(String filename);
 
@@ -527,12 +558,14 @@ public class ContentDisposition {
 
 		@Override
 		public Builder filename(String filename) {
+			Assert.hasText(filename, "No filename");
 			this.filename = filename;
 			return this;
 		}
 
 		@Override
 		public Builder filename(String filename, Charset charset) {
+			Assert.hasText(filename, "No filename");
 			this.filename = filename;
 			this.charset = charset;
 			return this;

spring-web/src/test/java/org/springframework/http/ContentDispositionTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,163 +16,216 @@
 
 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 java.util.function.BiConsumer;
+import java.util.function.Consumer;
 
-import static org.junit.Assert.assertEquals;
 import org.junit.Test;
 
-import org.springframework.util.ReflectionUtils;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+import static org.springframework.http.ContentDisposition.builder;
 
 /**
  * Unit tests for {@link ContentDisposition}
- *
  * @author Sebastien Deleuze
+ * @author Rossen Stoyanchev
  */
 public class ContentDispositionTests {
 
+	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");
-		assertEquals(ContentDisposition.builder("form-data")
-				.name("foo").filename("foo.txt").size(123L).build(), disposition);
+		assertEquals(builder("form-data").name("foo").filename("foo.txt").size(123L).build(),
+				parse("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123"));
 	}
 
 	@Test
-	public void parseType() {
-		ContentDisposition disposition = ContentDisposition.parse("form-data");
-		assertEquals(ContentDisposition.builder("form-data").build(), disposition);
+	public void parseFilenameUnquoted() {
+		assertEquals(builder("form-data").filename("unquoted").build(),
+				parse("form-data; filename=unquoted"));
+	}
+
+	@Test  // SPR-16091
+	public void parseFilenameWithSemicolon() {
+		assertEquals(builder("attachment").filename("filename with ; semicolon.txt").build(),
+				parse("attachment; filename=\"filename with ; semicolon.txt\""));
 	}
 
 	@Test
-	public void parseUnquotedFilename() {
-		ContentDisposition disposition = ContentDisposition
-				.parse("form-data; filename=unquoted");
-		assertEquals(ContentDisposition.builder("form-data").filename("unquoted").build(), disposition);
+	public void parseEncodedFilename() {
+		assertEquals(builder("form-data").name("name").filename("中文.txt", StandardCharsets.UTF_8).build(),
+				parse("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt"));
 	}
 
-	@Test  // SPR-16091
-	public void parseFilenameWithSemicolon() {
-		ContentDisposition disposition = ContentDisposition
-				.parse("attachment; filename=\"filename with ; semicolon.txt\"");
-		assertEquals(ContentDisposition.builder("attachment")
-				.filename("filename with ; semicolon.txt").build(), disposition);
+	@Test // gh-24112
+	public void parseEncodedFilenameWithPaddedCharset() {
+		assertEquals(builder("attachment").filename("some-file.zip", StandardCharsets.UTF_8).build(),
+				parse("attachment; filename*= UTF-8''some-file.zip"));
 	}
 
 	@Test
-	public void parseAndIgnoreEmptyParts() {
-		ContentDisposition disposition = ContentDisposition
-				.parse("form-data; name=\"foo\";; ; filename=\"foo.txt\"; size=123");
-		assertEquals(ContentDisposition.builder("form-data")
-				.name("foo").filename("foo.txt").size(123L).build(), disposition);
+	public void parseEncodedFilenameWithoutCharset() {
+		assertEquals(builder("form-data").name("name").filename("test.txt").build(),
+				parse("form-data; name=\"name\"; filename*=test.txt"));
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void parseEncodedFilenameWithInvalidCharset() {
+		parse("form-data; name=\"name\"; filename*=UTF-16''test.txt");
 	}
 
 	@Test
-	public void parseEncodedFilename() {
-		ContentDisposition disposition = ContentDisposition
-				.parse("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt");
-		assertEquals(ContentDisposition.builder("form-data").name("name")
-				.filename("中文.txt", StandardCharsets.UTF_8).build(), disposition);
+	public void parseEncodedFilenameWithInvalidName() {
+
+		Consumer<String> tester = input -> {
+			try {
+				parse(input);
+				fail();
+			}
+			catch (IllegalArgumentException ex) {
+				// expected
+			}
+		};
+
+		tester.accept("form-data; name=\"name\"; filename*=UTF-8''%A");
+		tester.accept("form-data; name=\"name\"; filename*=UTF-8''%A.txt");
+	}
+
+	@Test // gh-23077
+	public void parseWithEscapedQuote() {
+
+		BiConsumer<String, String> tester = (description, filename) ->
+				assertEquals(description,
+						builder("form-data").name("file").filename(filename).size(123L).build(),
+						parse("form-data; name=\"file\"; filename=\"" + filename + "\"; size=123"));
+
+		tester.accept("Escaped quotes should be ignored",
+				"\\\"The Twilight Zone\\\".txt");
+
+		tester.accept("Escaped quotes preceded by escaped backslashes should be ignored",
+				"\\\\\\\"The Twilight Zone\\\\\\\".txt");
+
+		tester.accept("Escaped backslashes should not suppress quote",
+				"The Twilight Zone \\\\");
+
+		tester.accept("Escaped backslashes should not suppress quote",
+				"The Twilight Zone \\\\\\\\");
+	}
+
+	@Test
+	public void parseWithExtraSemicolons() {
+		assertEquals(builder("form-data").name("foo").filename("foo.txt").size(123L).build(),
+				parse("form-data; name=\"foo\";; ; filename=\"foo.txt\"; size=123"));
+	}
+
+	@Test
+	public void parseDates() {
+		assertEquals(
+				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(),
+				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\""));
+	}
+
+	@Test
+	public void parseIgnoresInvalidDates() {
+		assertEquals(
+				builder("attachment")
+						.readDate(ZonedDateTime.parse("Wed, 14 Feb 2007 10:15:30 -0500", formatter))
+						.build(),
+				parse("attachment; creation-date=\"-1\"; " +
+						"modification-date=\"-1\"; " +
+						"read-date=\"Wed, 14 Feb 2007 10:15:30 -0500\""));
 	}
 
 	@Test(expected = IllegalArgumentException.class)
 	public void parseEmpty() {
-		ContentDisposition.parse("");
+		parse("");
 	}
 
 	@Test(expected = IllegalArgumentException.class)
 	public void parseNoType() {
-		ContentDisposition.parse(";");
+		parse(";");
 	}
 
 	@Test(expected = IllegalArgumentException.class)
 	public void parseInvalidParameter() {
-		ContentDisposition.parse("foo;bar");
+		parse("foo;bar");
 	}
 
-	@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;
-		assertEquals(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(), disposition);
+	private static ContentDisposition parse(String input) {
+		return ContentDisposition.parse(input);
 	}
 
+
 	@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;
-		assertEquals(ContentDisposition.builder("attachment")
-				.readDate(ZonedDateTime.parse("Wed, 14 Feb 2007 10:15:30 -0500", formatter)).build(), disposition);
+	public void format() {
+		assertEquals("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123",
+				builder("form-data").name("foo").filename("foo.txt").size(123L).build().toString());
 	}
 
 	@Test
-	public void headerValue() {
-		ContentDisposition disposition = ContentDisposition.builder("form-data")
-				.name("foo").filename("foo.txt").size(123L).build();
-		assertEquals("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123", disposition.toString());
+	public void formatWithEncodedFilename() {
+		assertEquals("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt",
+				builder("form-data").name("name").filename("中文.txt", StandardCharsets.UTF_8).build().toString());
 	}
 
 	@Test
-	public void headerValueWithEncodedFilename() {
-		ContentDisposition disposition = ContentDisposition.builder("form-data")
-				.name("name").filename("中文.txt", StandardCharsets.UTF_8).build();
-		assertEquals("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt",
-				disposition.toString());
+	public void formatWithEncodedFilenameUsingUsAscii() {
+		assertEquals("form-data; name=\"name\"; filename=\"test.txt\"",
+				builder("form-data")
+						.name("name")
+						.filename("test.txt", StandardCharsets.US_ASCII)
+						.build()
+						.toString());
 	}
 
-	@Test  // SPR-14547
-	public void encodeHeaderFieldParam() {
-		Method encode = ReflectionUtils.findMethod(ContentDisposition.class,
-				"encodeHeaderFieldParam", String.class, Charset.class);
-		ReflectionUtils.makeAccessible(encode);
+	@Test // gh-24220
+	public void formatWithFilenameWithQuotes() {
 
-		String result = (String)ReflectionUtils.invokeMethod(encode, null, "test.txt",
-				StandardCharsets.US_ASCII);
-		assertEquals("test.txt", result);
+		BiConsumer<String, String> tester = (input, output) -> {
 
-		result = (String)ReflectionUtils.invokeMethod(encode, null, "中文.txt", StandardCharsets.UTF_8);
-		assertEquals("UTF-8''%E4%B8%AD%E6%96%87.txt", result);
-	}
+			assertEquals("form-data; filename=\"" + output + "\"",
+					builder("form-data").filename(input).build().toString());
 
-	@Test(expected = IllegalArgumentException.class)
-	public void encodeHeaderFieldParamInvalidCharset() {
-		Method encode = ReflectionUtils.findMethod(ContentDisposition.class,
-				"encodeHeaderFieldParam", String.class, Charset.class);
-		ReflectionUtils.makeAccessible(encode);
-		ReflectionUtils.invokeMethod(encode, null, "test", StandardCharsets.UTF_16);
-	}
+			assertEquals("form-data; filename=\"" + output + "\"",
+					builder("form-data").filename(input, StandardCharsets.US_ASCII).build().toString());
+		};
+
+		String filename = "\"foo.txt";
+		tester.accept(filename, "\\" + filename);
+
+		filename = "\\\"foo.txt";
+		tester.accept(filename, filename);
+
+		filename = "\\\\\"foo.txt";
+		tester.accept(filename, "\\" + filename);
+
+		filename = "\\\\\\\"foo.txt";
+		tester.accept(filename, filename);
 
-	@Test  // SPR-14408
-	public void decodeHeaderFieldParam() {
-		Method decode = ReflectionUtils.findMethod(ContentDisposition.class,
-				"decodeHeaderFieldParam", String.class);
-		ReflectionUtils.makeAccessible(decode);
+		filename = "\\\\\\\\\"foo.txt";
+		tester.accept(filename, "\\" + filename);
 
-		String result = (String)ReflectionUtils.invokeMethod(decode, null, "test.txt");
-		assertEquals("test.txt", result);
+		tester.accept("\"\"foo.txt", "\\\"\\\"foo.txt");
+		tester.accept("\"\"\"foo.txt", "\\\"\\\"\\\"foo.txt");
 
-		result = (String)ReflectionUtils.invokeMethod(decode, null, "UTF-8''%E4%B8%AD%E6%96%87.txt");
-		assertEquals("中文.txt", result);
+		tester.accept("foo.txt\\", "foo.txt");
+		tester.accept("foo.txt\\\\", "foo.txt\\\\");
+		tester.accept("foo.txt\\\\\\", "foo.txt\\\\");
 	}
 
 	@Test(expected = IllegalArgumentException.class)
-	public void decodeHeaderFieldParamInvalidCharset() {
-		Method decode = ReflectionUtils.findMethod(ContentDisposition.class,
-				"decodeHeaderFieldParam", String.class);
-		ReflectionUtils.makeAccessible(decode);
-		ReflectionUtils.invokeMethod(decode, null, "UTF-16''test");
+	public void formatWithEncodedFilenameUsingInvalidCharset() {
+		builder("form-data").name("name").filename("test.txt", StandardCharsets.UTF_16).build().toString();
 	}
 
 }