Merge pull request #11603 from playframework/mergify/bp/2.8.x/pr-11571

[2.8.x] Escape Content-Disposition params according to WHATWG HTML living standard (backport #11571) by @mkurz

core/play-integration-test/src/it/scala/play/it/libs/JavaWSSpec.scala

@@ -170,6 +170,26 @@ trait JavaWSSpec
       body.path("file").textValue() must_== "This is a test asset."
     }
 
+    "sending a multipart form body with escaped 'name' and 'filename' params" in withEchoServer { ws =>
+      val file = new File(this.getClass.getResource("/testassets/bar.txt").toURI).toPath
+      val dp   = new Http.MultipartFormData.DataPart("f\ni\re\"l\nd1", "world")
+      val fp = new Http.MultipartFormData.FilePart(
+        "f\"i\rl\nef\"ie\nld\r1",
+        "f\rir\"s\ntf\ril\"e\n.txt",
+        "text/plain",
+        FileIO.fromPath(file).asJava
+      )
+      val source = akka.stream.javadsl.Source.from(util.Arrays.asList(dp, fp))
+
+      val res  = ws.url("/post").post(source)
+      val body = res.toCompletableFuture.get().getBody()
+
+      body must contain("Content-Disposition: form-data; name=\"f%0Ai%0De%22l%0Ad1\"")
+      body must contain(
+        "Content-Disposition: form-data; name=\"f%22i%0Dl%0Aef%22ie%0Ald%0D1\"; filename=\"f%0Dir%22s%0Atf%0Dil%22e%0A.txt\""
+      )
+    }
+
     "send a multipart request body via multipartBody()" in withServer { ws =>
       val file = new File(this.getClass.getResource("/testassets/bar.txt").toURI)
       val dp   = new Http.MultipartFormData.DataPart("hello", "world")

core/play-integration-test/src/it/scala/play/it/libs/ScalaWSSpec.scala

@@ -113,6 +113,25 @@ trait ScalaWSSpec
       (body \ "file").toOption must beSome(JsString("This is a test asset."))
     }
 
+    "send a multipart request body with escaped 'name' and 'filename' params" in withEchoServer { ws =>
+      val file = new File(this.getClass.getResource("/testassets/foo.txt").toURI)
+      val dp   = MultipartFormData.DataPart("f\ni\re\"l\nd1", "world")
+      val fp = MultipartFormData.FilePart(
+        "f\"i\rl\nef\"ie\nld\r1",
+        "f\rir\"s\ntf\ril\"e\n.txt",
+        None,
+        FileIO.fromPath(file.toPath)
+      )
+      val source = Source(List(dp, fp))
+      val res    = ws.url("/post").withBody(source).withMethod("POST").execute()
+      val body   = await(res).body
+
+      body must contain("""Content-Disposition: form-data; name="f%0Ai%0De%22l%0Ad1"""")
+      body must contain(
+        """Content-Disposition: form-data; name="f%22i%0Dl%0Aef%22ie%0Ald%0D1"; filename="f%0Dir%22s%0Atf%0Dil%22e%0A.txt""""
+      )
+    }
+
     "not throw an exception while signing requests" >> {
       val calc = new CustomSigner
 

core/play-java/src/test/java/play/mvc/RequestBuilderTest.java

@@ -4,7 +4,15 @@
 
 package play.mvc;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import akka.stream.javadsl.FileIO;
 import akka.stream.javadsl.Source;
+import akka.util.ByteString;
+import java.io.File;
+import java.io.IOException;
+import java.net.URISyntaxException;
+import org.hamcrest.CoreMatchers;
 import org.junit.Test;
 import play.api.Application;
 import play.api.Play;
@@ -19,6 +27,7 @@
 import play.test.Helpers;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -385,16 +394,59 @@ public void multipartForm() throws ExecutionException, InterruptedException {
     Play.stop(app);
   }
 
+  @Test
+  public void multipartForm_bodyRaw_correctEscapedParams() throws URISyntaxException, IOException {
+    Application app = new GuiceApplicationBuilder().build();
+    Play.start(app);
+
+    File file = new File(this.getClass().getResource("/testassets/foo.txt").toURI());
+    Http.MultipartFormData.Part<Source<ByteString, ?>> filePart =
+        new Http.MultipartFormData.FilePart<>(
+            "f\"i\rl\nef\"ie\nld\r1",
+            "f\rir\"s\ntf\ril\"e\n.txt",
+            "text/plain",
+            FileIO.fromPath(file.toPath()),
+            java.nio.file.Files.size(file.toPath()));
+
+    Http.MultipartFormData.DataPart dataPart =
+        new Http.MultipartFormData.DataPart("f\ni\re\"l\nd1", "value1");
+
+    TemporaryFileCreator temporaryFileCreator =
+        app.injector().instanceOf(TemporaryFileCreator.class);
+    final Request request =
+        new RequestBuilder()
+            .uri("http://playframework.com/")
+            // bodyRaw, as its name tells us, saves the body in raw bytes.
+            // To do that it needs to render the body, so bodyRaw(...) goes through
+            // play.mvc.MultipartFormatter#transform(...), so eventually
+            // play.core.formatters.Multipart, which renders the multipart/form-data elements and
+            // escapes params, will be used
+            .bodyRaw(Arrays.asList(dataPart, filePart), temporaryFileCreator, app.materializer())
+            .build();
+
+    String body =
+        request.body().asBytes().utf8String(); // Let's get the text representation of the bytes
+    assertThat(
+        body,
+        CoreMatchers.containsString("Content-Disposition: form-data; name=\"f%0Ai%0De%22l%0Ad1\""));
+    assertThat(
+        body,
+        CoreMatchers.containsString(
+            "Content-Disposition: form-data; name=\"f%22i%0Dl%0Aef%22ie%0Ald%0D1\"; filename=\"f%0Dir%22s%0Atf%0Dil%22e%0A.txt\""));
+
+    Play.stop(app);
+  }
+
   @Test
   public void multipartFormContentLength() {
     final Map<String, String[]> dataParts = new HashMap<>();
-    dataParts.put("field1", new String[] {"value1"});
+    dataParts.put("f\ni\re\"l\nd1", new String[] {"value1"});
     dataParts.put("field2", new String[] {"value2-1", "value2.2"});
 
     final List<Http.MultipartFormData.FilePart> fileParts = new ArrayList<>();
     fileParts.add(
         new Http.MultipartFormData.FilePart<>(
-            "filefield1", "firstfile.txt", "text/plain", "abc", 3));
+            "f\"i\rl\nef\"ie\nld\r1", "f\rir\"s\ntf\ril\"e\n.txt", "text/plain", "abc", 3));
     fileParts.add(
         new Http.MultipartFormData.FilePart<>(
             "file_field_2", "secondfile.txt", "text/plain", "hello world", 11));
@@ -412,10 +464,10 @@ public void multipartFormContentLength() {
     // Now let's check the calculated Content-Length. The request body should look like this when
     // stringified:
     // (You can copy the lines, save it with an editor with UTF-8 encoding and Windows line endings
-    // (\r\n) and the file size should be 542 bytes
+    // (\r\n) and the file size should be 590 bytes
     /*
     --somerandomboundary
-    Content-Disposition: form-data; name="field1"
+    Content-Disposition: form-data; name="f%0Ai%0De%22l%0Ad1"
 
     value1
     --somerandomboundary
@@ -427,7 +479,7 @@ public void multipartFormContentLength() {
 
     value2.2
     --somerandomboundary
-    Content-Disposition: form-data; name="filefield1"; filename="firstfile.txt"
+    Content-Disposition: form-data; name="f%22i%0Dl%0Aef%22ie%0Ald%0D1"; filename="f%0Dir%22s%0Atf%0Dil%22e%0A.txt"
     Content-Type: text/plain
 
     abc
@@ -438,6 +490,6 @@ public void multipartFormContentLength() {
     hello world
     --somerandomboundary--
     */
-    assertEquals(request.header(Http.HeaderNames.CONTENT_LENGTH).get(), "542");
+    assertEquals(request.header(Http.HeaderNames.CONTENT_LENGTH).get(), "590");
   }
 }

core/play-java/src/test/resources/testassets/foo.txt

@@ -0,0 +1 @@
+abc

core/play/src/main/java/play/mvc/Http.java

@@ -4,6 +4,8 @@
 
 package play.mvc;
 
+import static play.core.formatters.Multipart.escapeParamWithHTML5Strategy;
+
 import akka.stream.Materializer;
 import akka.stream.javadsl.Sink;
 import akka.stream.javadsl.Source;
@@ -681,9 +683,11 @@ private int partLength(
               + "Content-Disposition: "
               + dispositionType
               + "; name=\""
-              + name
+              + escapeParamWithHTML5Strategy(name)
               + "\""
-              + (filename != null ? "; filename=\"" + filename + "\"" : "")
+              + (filename != null
+                  ? "; filename=\"" + escapeParamWithHTML5Strategy(filename) + "\""
+                  : "")
               + "\r\n"
               + (contentType != null ? "Content-Type: " + contentType + "\r\n" : "")
               + "\r\n"

core/play/src/main/scala/play/api/http/Writeable.scala

@@ -9,6 +9,7 @@ import play.api.libs.Files.TemporaryFile
 import play.api.mvc._
 import play.api.libs.json._
 import play.api.mvc.MultipartFormData.FilePart
+import play.core.formatters.Multipart
 
 import scala.annotation._
 
@@ -145,16 +146,17 @@ trait DefaultWriteables extends LowPriorityWriteables {
         .flatMap {
           case (name, values) =>
             values.map { value =>
-              s"--$boundary\r\n${HeaderNames.CONTENT_DISPOSITION}: form-data; name=$name\r\n\r\n$value\r\n"
+              s"--$boundary\r\n${HeaderNames.CONTENT_DISPOSITION}: form-data; name=${Multipart
+                .escapeParamWithHTML5Strategy(name)}\r\n\r\n$value\r\n"
             }
         }
         .mkString("")
       codec.encode(dataParts)
     }
 
     def filePartHeader(file: FilePart[A]) = {
-      val name     = s""""${file.key}""""
-      val filename = s""""${file.filename}""""
+      val name     = s""""${Multipart.escapeParamWithHTML5Strategy(file.key)}""""
+      val filename = s""""${Multipart.escapeParamWithHTML5Strategy(file.filename)}""""
       val contentType = file.contentType
         .map { ct => s"${HeaderNames.CONTENT_TYPE}: $ct\r\n" }
         .getOrElse("")

core/play/src/main/scala/play/core/formatters/Multipart.scala

@@ -62,6 +62,21 @@ object Multipart {
     new String(bytes.toArray, US_ASCII)
   }
 
+  /**
+   * Helper function to escape a single header parameter using the HTML5 strategy.
+   * (The alternative would be the strategy defined by RFC5987)
+   * Particularly useful for Content-Disposition header parameters which might contain
+   * non-ASCII values, like file names.
+   * This follows the "WHATWG HTML living standard" section 4.10.21.8 and matches
+   * the behavior of curl and modern browsers.
+   * See https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart-form-data
+   */
+  def escapeParamWithHTML5Strategy(value: String) =
+    value
+      .replace("\"", "%22")
+      .replace("\r", "%0D")
+      .replace("\n", "%0A")
+
   private sealed trait Formatter {
     def ~~(ch: Char): this.type
 
@@ -204,8 +219,10 @@ object Multipart {
       contentDisposition: String,
       filename: Option[String]
   ): Unit = {
-    f ~~ "Content-Disposition: " ~~ dispositionType ~~ "; name=" ~~ '"' ~~ contentDisposition ~~ '"'
-    filename.foreach { name => f ~~ "; filename=" ~~ '"' ~~ name ~~ '"' }
+    f ~~ "Content-Disposition: " ~~ dispositionType ~~ "; name=" ~~ '"' ~~ escapeParamWithHTML5Strategy(
+      contentDisposition
+    ) ~~ '"'
+    filename.foreach { name => f ~~ "; filename=" ~~ '"' ~~ escapeParamWithHTML5Strategy(name) ~~ '"' }
     f ~~ CrLf
   }
 

core/play/src/test/scala/play/api/http/WriteableSpec.scala

@@ -55,6 +55,30 @@ class WriteableSpec extends Specification {
         transformed.utf8String must contain("file part value")
       }
 
+      "escape 'name' and 'filename' params" in {
+        val multipartFormData =
+          createMultipartFormData[String](
+            "file part value",
+            dataPartKey = "ab\"cd\nef\rgh\"ij\rk\nl",
+            filePartKey = "mn\"op\nqr\rst\"uv\rw\nx",
+            filePartFilename = "fo\"o\no\rb\"a\ra\nar.p\"df"
+          )
+        val codec = Codec.utf_8
+
+        val writeable = Writeable.writeableOf_MultipartFormData[String](
+          codec,
+          Writeable[FilePart[String]]((f: FilePart[String]) => codec.encode(f.ref), None)
+        )
+        val transformed: ByteString = writeable.transform(multipartFormData)
+
+        transformed.utf8String must contain("""Content-Disposition: form-data; name=ab%22cd%0Aef%0Dgh%22ij%0Dk%0Al""")
+        transformed.utf8String must contain(
+          """Content-Disposition: form-data; name="mn%22op%0Aqr%0Dst%22uv%0Dw%0Ax"; filename="fo%22o%0Ao%0Db%22a%0Da%0Aar.p%22df""""
+        )
+        transformed.utf8String must contain("Content-Type: text/plain")
+        transformed.utf8String must contain("file part value")
+      }
+
       "use multipart/form-data content-type" in {
         val contentType = Some("text/plain")
         val codec       = Codec.utf_8
@@ -78,15 +102,20 @@ class WriteableSpec extends Specification {
     }
   }
 
-  def createMultipartFormData[A](ref: A): MultipartFormData[A] = {
+  def createMultipartFormData[A](
+      ref: A,
+      dataPartKey: String = "name",
+      filePartKey: String = "thefile",
+      filePartFilename: String = "something.text"
+  ): MultipartFormData[A] = {
     MultipartFormData[A](
       dataParts = Map(
-        "name" -> Seq("value")
+        dataPartKey -> Seq("value")
       ),
       files = Seq(
         FilePart[A](
-          key = "thefile",
-          filename = "something.text",
+          key = filePartKey,
+          filename = filePartFilename,
           contentType = Some("text/plain"),
           ref = ref
         )

transport/client/play-ahc-ws/src/test/resources/testassets/foo.txt

@@ -0,0 +1 @@
+abc

transport/client/play-ahc-ws/src/test/scala/play/api/libs/ws/ahc/AhcWSSpec.scala

@@ -7,6 +7,8 @@ package play.api.libs.ws.ahc
 import java.util
 
 import akka.stream.Materializer
+import akka.stream.scaladsl.FileIO
+import akka.stream.scaladsl.Source
 import akka.util.ByteString
 import akka.util.Timeout
 import org.specs2.concurrent.ExecutionEnv
@@ -440,6 +442,49 @@ class AhcWSSpec(implicit ee: ExecutionEnv)
     rep.body must ===("gziped response")
   }
 
+  def multipartFormDataFakeApp = {
+    val routes: (Application) => PartialFunction[(String, String), Handler] = { (app: Application) =>
+      {
+        case ("POST", "/") =>
+          val action = app.injector.instanceOf(classOf[DefaultActionBuilder])
+          action { request =>
+            Results.Ok(
+              request.body.asMultipartFormData
+                .map(mpf => {
+                  "dataPart name: " + mpf.dataParts.keys.mkString(",") + "\n" +
+                    "filePart names: " + mpf.files.map(_.key).mkString(",") + "\n" +
+                    "filePart filenames: " + mpf.files.map(_.filename).mkString(",")
+                })
+                .getOrElse("")
+            )
+          }
+      }
+    }
+
+    GuiceApplicationBuilder().appRoutes(routes).build()
+  }
+
+  "escape 'name' and 'filename' params of a multipart form body" in new WithServer(multipartFormDataFakeApp) {
+    {
+      val wsClient = app.injector.instanceOf(classOf[play.api.libs.ws.WSClient])
+      val file     = new java.io.File(this.getClass.getResource("/testassets/foo.txt").toURI)
+      val dp       = MultipartFormData.DataPart("h\"e\rl\nl\"o\rwo\nrld", "world")
+      val fp =
+        MultipartFormData.FilePart("u\"p\rl\no\"a\rd", "f\"o\ro\n_\"b\ra\nr.txt", None, FileIO.fromPath(file.toPath))
+      val source         = Source(List(dp, fp))
+      val futureResponse = wsClient.url(s"http://localhost:${Helpers.testServerPort}/").post(source)
+
+      // This test could experience CI timeouts. Give it more time.
+      val reallyLongTimeout = Timeout(defaultAwaitTimeout.duration * 3)
+      val rep               = await(futureResponse)(reallyLongTimeout)
+
+      rep.status must ===(200)
+      rep.body must be_==("""dataPart name: h%22e%0Dl%0Al%22o%0Dwo%0Arld
+                            |filePart names: u%22p%0Dl%0Ao%22a%0Dd
+                            |filePart filenames: f%22o%0Do%0A_%22b%0Da%0Ar.txt""".stripMargin)
+    }
+  }
+
   "Ahc WS Response" should {
     "get cookies from an AHC response" in {
       val ahcResponse: AHCResponse = mock[AHCResponse]