Escape quotes in filename

Closes gh-24220

Author: rstoyanchev

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

@@ -458,7 +458,11 @@ public interface Builder {
 		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);
 
@@ -539,10 +543,24 @@ public Builder name(String name) {
 
 		@Override
 		public Builder filename(String filename) {
-			this.filename = filename;
+			Assert.hasText(filename, "No filename");
+			this.filename = escapeQuotationMarks(filename);
 			return this;
 		}
 
+		private static String escapeQuotationMarks(String filename) {
+			if (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 == '\\');
+			}
+			return sb.toString();
+		}
+
 		@Override
 		public Builder filename(String filename, Charset charset) {
 			this.filename = filename;

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

@@ -19,11 +19,13 @@
 import java.nio.charset.StandardCharsets;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
+import java.util.function.BiConsumer;
 
 import org.junit.jupiter.api.Test;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+import static org.springframework.http.ContentDisposition.builder;
 
 /**
  * Unit tests for {@link ContentDisposition}
@@ -38,7 +40,7 @@ public class ContentDispositionTests {
 	@Test
 	public void parse() {
 		assertThat(parse("form-data; name=\"foo\"; filename=\"foo.txt\"; size=123"))
-				.isEqualTo(ContentDisposition.builder("form-data")
+				.isEqualTo(builder("form-data")
 						.name("foo")
 						.filename("foo.txt")
 						.size(123L)
@@ -48,23 +50,23 @@ public void parse() {
 	@Test
 	public void parseFilenameUnquoted() {
 		assertThat(parse("form-data; filename=unquoted"))
-				.isEqualTo(ContentDisposition.builder("form-data")
+				.isEqualTo(builder("form-data")
 						.filename("unquoted")
 						.build());
 	}
 
 	@Test  // SPR-16091
 	public void parseFilenameWithSemicolon() {
 		assertThat(parse("attachment; filename=\"filename with ; semicolon.txt\""))
-				.isEqualTo(ContentDisposition.builder("attachment")
+				.isEqualTo(builder("attachment")
 						.filename("filename with ; semicolon.txt")
 						.build());
 	}
 
 	@Test
 	public void parseEncodedFilename() {
 		assertThat(parse("form-data; name=\"name\"; filename*=UTF-8''%E4%B8%AD%E6%96%87.txt"))
-				.isEqualTo(ContentDisposition.builder("form-data")
+				.isEqualTo(builder("form-data")
 						.name("name")
 						.filename("中文.txt", StandardCharsets.UTF_8)
 						.build());
@@ -73,15 +75,15 @@ public void parseEncodedFilename() {
 	@Test // gh-24112
 	public void parseEncodedFilenameWithPaddedCharset() {
 		assertThat(parse("attachment; filename*= UTF-8''some-file.zip"))
-				.isEqualTo(ContentDisposition.builder("attachment")
+				.isEqualTo(builder("attachment")
 						.filename("some-file.zip", StandardCharsets.UTF_8)
 						.build());
 	}
 
 	@Test
 	public void parseEncodedFilenameWithoutCharset() {
 		assertThat(parse("form-data; name=\"name\"; filename*=test.txt"))
-				.isEqualTo(ContentDisposition.builder("form-data")
+				.isEqualTo(builder("form-data")
 						.name("name")
 						.filename("test.txt")
 						.build());
@@ -104,18 +106,30 @@ public void parseEncodedFilenameWithInvalidName() {
 
 	@Test // gh-23077
 	public void parseWithEscapedQuote() {
-		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());
+
+		BiConsumer<String, String> tester = (description, filename) -> {
+			assertThat(parse("form-data; name=\"file\"; filename=\"" + filename + "\"; size=123"))
+					.as(description)
+					.isEqualTo(builder("form-data").name("file").filename(filename).size(123L).build());
+		};
+
+		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() {
 		assertThat(parse("form-data; name=\"foo\";; ; filename=\"foo.txt\"; size=123"))
-				.isEqualTo(ContentDisposition.builder("form-data")
+				.isEqualTo(builder("form-data")
 						.name("foo")
 						.filename("foo.txt")
 						.size(123L)
@@ -133,7 +147,7 @@ public void parseDates() {
 						"creation-date=\"" + creationTime.format(formatter) + "\"; " +
 						"modification-date=\"" + modificationTime.format(formatter) + "\"; " +
 						"read-date=\"" + readTime.format(formatter) + "\"")).isEqualTo(
-				ContentDisposition.builder("attachment")
+				builder("attachment")
 						.creationDate(creationTime)
 						.modificationDate(modificationTime)
 						.readDate(readTime)
@@ -149,7 +163,7 @@ public void parseIgnoresInvalidDates() {
 						"creation-date=\"-1\"; " +
 						"modification-date=\"-1\"; " +
 						"read-date=\"" + readTime.format(formatter) + "\"")).isEqualTo(
-				ContentDisposition.builder("attachment")
+				builder("attachment")
 						.readDate(readTime)
 						.build());
 	}
@@ -177,7 +191,7 @@ private static ContentDisposition parse(String input) {
 	@Test
 	public void format() {
 		assertThat(
-				ContentDisposition.builder("form-data")
+				builder("form-data")
 						.name("foo")
 						.filename("foo.txt")
 						.size(123L)
@@ -188,7 +202,7 @@ public void format() {
 	@Test
 	public void formatWithEncodedFilename() {
 		assertThat(
-				ContentDisposition.builder("form-data")
+				builder("form-data")
 						.name("name")
 						.filename("中文.txt", StandardCharsets.UTF_8)
 						.build().toString())
@@ -198,18 +212,45 @@ public void formatWithEncodedFilename() {
 	@Test
 	public void formatWithEncodedFilenameUsingUsAscii() {
 		assertThat(
-				ContentDisposition.builder("form-data")
+				builder("form-data")
 						.name("name")
 						.filename("test.txt", StandardCharsets.US_ASCII)
 						.build()
 						.toString())
 				.isEqualTo("form-data; name=\"name\"; filename=\"test.txt\"");
 	}
 
+	@Test // gh-24220
+	public void formatWithFilenameWithQuotes() {
+
+		BiConsumer<String, String> tester = (input, output) -> {
+			assertThat(builder("form-data").filename(input).build().toString())
+					.isEqualTo("form-data; filename=\"" + output + "\"");
+		};
+
+		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);
+
+		filename = "\\\\\\\\\"foo.txt";
+		tester.accept(filename, "\\" + filename);
+
+		tester.accept("\"\"foo.txt", "\\\"\\\"foo.txt");
+		tester.accept("\"\"\"foo.txt", "\\\"\\\"\\\"foo.txt");
+	}
+
 	@Test
 	public void formatWithEncodedFilenameUsingInvalidCharset() {
 		assertThatIllegalArgumentException().isThrownBy(() ->
-				ContentDisposition.builder("form-data")
+				builder("form-data")
 						.name("name")
 						.filename("test.txt", StandardCharsets.UTF_16)
 						.build()