diff --git a/core/play-integration-test/src/it/scala/play/it/libs/JavaWSSpec.scala b/core/play-integration-test/src/it/scala/play/it/libs/JavaWSSpec.scala index 024bad79bd0..bf3f171dbf3 100644 --- a/core/play-integration-test/src/it/scala/play/it/libs/JavaWSSpec.scala +++ b/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") diff --git a/core/play-integration-test/src/it/scala/play/it/libs/ScalaWSSpec.scala b/core/play-integration-test/src/it/scala/play/it/libs/ScalaWSSpec.scala index 39bbf8e2bb2..b6ef9f68d3e 100644 --- a/core/play-integration-test/src/it/scala/play/it/libs/ScalaWSSpec.scala +++ b/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 diff --git a/core/play-java/src/test/java/play/mvc/RequestBuilderTest.java b/core/play-java/src/test/java/play/mvc/RequestBuilderTest.java index 03a077aa8f3..1ceb0b38120 100644 --- a/core/play-java/src/test/java/play/mvc/RequestBuilderTest.java +++ b/core/play-java/src/test/java/play/mvc/RequestBuilderTest.java @@ -4,9 +4,15 @@ package play.mvc; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.*; +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 java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -15,6 +21,7 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.ExecutionException; +import org.hamcrest.CoreMatchers; import org.junit.Test; import play.api.Application; import play.api.Play; @@ -384,16 +391,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> 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(List.of(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 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 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)); @@ -411,10 +461,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 @@ -426,7 +476,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 @@ -437,6 +487,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"); } } diff --git a/core/play-java/src/test/resources/testassets/foo.txt b/core/play-java/src/test/resources/testassets/foo.txt new file mode 100644 index 00000000000..f2ba8f84ab5 --- /dev/null +++ b/core/play-java/src/test/resources/testassets/foo.txt @@ -0,0 +1 @@ +abc \ No newline at end of file diff --git a/core/play/src/main/java/play/mvc/Http.java b/core/play/src/main/java/play/mvc/Http.java index a77903022bf..66cedec72d4 100644 --- a/core/play/src/main/java/play/mvc/Http.java +++ b/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; @@ -731,9 +733,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" diff --git a/core/play/src/main/scala/play/api/http/Writeable.scala b/core/play/src/main/scala/play/api/http/Writeable.scala index fffe6de587c..bd7d5a94375 100644 --- a/core/play/src/main/scala/play/api/http/Writeable.scala +++ b/core/play/src/main/scala/play/api/http/Writeable.scala @@ -161,7 +161,8 @@ trait DefaultWriteables extends LowPriorityWriteables { .flatMap { case (name, values) => values.map { value => - s"""--$resolvedBoundary\r\n${HeaderNames.CONTENT_DISPOSITION}: form-data; name="$name"\r\n\r\n$value\r\n""" + s"""--$resolvedBoundary\r\n${HeaderNames.CONTENT_DISPOSITION}: form-data; name="${Multipart + .escapeParamWithHTML5Strategy(name)}"\r\n\r\n$value\r\n""" } } .mkString("") @@ -169,8 +170,8 @@ trait DefaultWriteables extends LowPriorityWriteables { } 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("") diff --git a/core/play/src/main/scala/play/core/formatters/Multipart.scala b/core/play/src/main/scala/play/core/formatters/Multipart.scala index d4e45e744a3..4859b676d8b 100644 --- a/core/play/src/main/scala/play/core/formatters/Multipart.scala +++ b/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 @@ -212,8 +227,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 } diff --git a/core/play/src/test/scala/play/api/http/WriteableSpec.scala b/core/play/src/test/scala/play/api/http/WriteableSpec.scala index 29cefe226b8..16e0a70fa22 100644 --- a/core/play/src/test/scala/play/api/http/WriteableSpec.scala +++ b/core/play/src/test/scala/play/api/http/WriteableSpec.scala @@ -51,6 +51,28 @@ class WriteableSpec extends Specification { transformed.utf8String must contain("file part value") } + "escape 'name' and 'filename' params" in { + val multipartFormData = + createMultipartFormData[String]( + "file part value", + data => Some(ByteString.fromString(data)), + 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](None)(codec) + 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 codec = Codec.utf_8 val writeable = Writeable.writeableOf_MultipartFormData(None)(codec) @@ -70,15 +92,21 @@ class WriteableSpec extends Specification { } } - def createMultipartFormData[A](ref: A, refToBytes: A => Option[ByteString] = (a: A) => None): MultipartFormData[A] = { + def createMultipartFormData[A]( + ref: A, + refToBytes: A => Option[ByteString] = (a: A) => None, + 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, refToBytes = refToBytes diff --git a/transport/client/play-ahc-ws/src/test/resources/testassets/foo.txt b/transport/client/play-ahc-ws/src/test/resources/testassets/foo.txt new file mode 100644 index 00000000000..f2ba8f84ab5 --- /dev/null +++ b/transport/client/play-ahc-ws/src/test/resources/testassets/foo.txt @@ -0,0 +1 @@ +abc \ No newline at end of file diff --git a/transport/client/play-ahc-ws/src/test/scala/play/api/libs/ws/ahc/AhcWSSpec.scala b/transport/client/play-ahc-ws/src/test/scala/play/api/libs/ws/ahc/AhcWSSpec.scala index 5f25c1027bd..19ca7412314 100644 --- a/transport/client/play-ahc-ws/src/test/scala/play/api/libs/ws/ahc/AhcWSSpec.scala +++ b/transport/client/play-ahc-ws/src/test/scala/play/api/libs/ws/ahc/AhcWSSpec.scala @@ -8,6 +8,8 @@ import java.util import java.nio.charset.StandardCharsets 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 @@ -412,6 +414,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]