From cbe92cd6d57835ab873db83f9cf472a5fa610b9b Mon Sep 17 00:00:00 2001 From: nikosk Date: Fri, 27 Dec 2024 09:56:51 +0200 Subject: [PATCH] Fixes form submissions with empty file fields throwing exceptions (#150) Given a form with a file filed i.e. `` if the user does not include a file to upload when the form is submitted the browser will send a request with the following multipart body: ``` -----------------------------101075975012956453003083734809 Content-Disposition: form-data; name="some-field" -----------------------------101075975012956453003083734809 Content-Disposition: form-data; name="a-file"; filename="" Content-Type: application/octet-stream -----------------------------101075975012956453003083734809-- ``` Since there is no file but there is a file field, the browser will send a part with empty filename and body and octet-stream as the content type to signify a submission without a file to upload. In cask to handle such form submissions you create an endpoint like the following: ``` @cask.postForm("/post") def post(image: cask.FormFile) = s"Image filename: ${image.fileName}" ``` The `postForm` decorator upon receiving the submission it will create an Undertow `MultiPartParserDefinition` and try to parse the request. After parsing the boundary and the headers of the part Undertow will then try to parse the body which is empty and because there is nothing to parse it will never create a file for the `FormValue` representing the empty file field (see `MultiPartParserDefinition` line 329): ![image](https://github.com/user-attachments/assets/cbfa167f-a870-413a-9f48-9890595ca042) This results to `FormData` with a `FormValue` where the `fileitem` field is `null`. ![image](https://github.com/user-attachments/assets/6fc890ba-6ae9-40be-9ab3-a5c7fd7e74fc) So when cask tries to convert the Undertow `FormValue`s to cask `FormEntry` by calling `FormEntry.fromUndertow` on each FormValue the `FormValue.isFile` condition returns false and cask tries to convert the Undertow `FormValue` to a cask `FormValue` and an exception is thrown when calling `getValue()` on a binary `FormValue`. ![image](https://github.com/user-attachments/assets/0f7715a4-c166-4dd2-b8f1-a2c3efb0bd10) ![image](https://github.com/user-attachments/assets/c69f804a-e54e-49d8-adf8-ce550a2f96e0) ![image](https://github.com/user-attachments/assets/e00b0328-c1b9-4037-98d5-2747c1ae2211) In essence if your endpoint is using the `postForm` decorator and is expecting a `FormField` cask will throw an exception if the user doesn't submit a file and there's no chance for the developer to do any validation. This PR is preventing the exception from being thrown by detecting the empty binary field and replicating the Undertow behaviour of passing a `FormValue` with null values (it's converted to Option[Path] in cask land) so that the code has a chance to perform validation and accept or reject the form submission. ``` def fromUndertow(from: io.undertow.server.handlers.form.FormData.FormValue): FormEntry = { val isOctetStream = Option(from.getHeaders) .flatMap(headers => Option(headers.get(HttpString.tryFromString("Content-Type")))) .exists(h => h.asScala.exists(v => v == "application/octet-stream")) // browsers will set empty file fields to content type: octet-stream if (isOctetStream || from.isFileItem) FormFile(from.getFileName, Try(from.getFileItem.getFile).toOption, from.getHeaders) else FormValue(from.getValue, from.getHeaders) } ``` Closes #149 --------- Co-authored-by: nk --- build.mill | 1 + cask/src/cask/model/Params.scala | 18 +++++++--- .../app/resources/example.txt | 0 .../app/src/MultipartFormSubmission.scala | 26 ++++++++++++++ .../app/test/src/ExampleTests.scala | 34 +++++++++++++++++++ example/multipartFormSubmission/package.mill | 18 ++++++++++ 6 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 example/multipartFormSubmission/app/resources/example.txt create mode 100644 example/multipartFormSubmission/app/src/MultipartFormSubmission.scala create mode 100644 example/multipartFormSubmission/app/test/src/ExampleTests.scala create mode 100644 example/multipartFormSubmission/package.mill diff --git a/build.mill b/build.mill index 88544e365d..a02f4b502f 100644 --- a/build.mill +++ b/build.mill @@ -125,6 +125,7 @@ def zippedExamples = T { build.example.websockets2.millSourcePath, build.example.websockets3.millSourcePath, build.example.websockets4.millSourcePath, + build.example.multipartFormSubmission.millSourcePath, ) for (example <- examples) yield { diff --git a/cask/src/cask/model/Params.scala b/cask/src/cask/model/Params.scala index 09871e57b4..6f3fba610c 100644 --- a/cask/src/cask/model/Params.scala +++ b/cask/src/cask/model/Params.scala @@ -1,10 +1,12 @@ package cask.model import java.io.{ByteArrayOutputStream, InputStream} - import cask.internal.Util import io.undertow.server.HttpServerExchange import io.undertow.server.handlers.CookieImpl +import io.undertow.util.HttpString +import scala.util.Try +import scala.collection.JavaConverters.collectionAsScalaIterableConverter case class QueryParams(value: Map[String, collection.Seq[String]]) case class RemainingPathSegments(value: Seq[String]) @@ -98,9 +100,13 @@ sealed trait FormEntry{ } } object FormEntry{ - def fromUndertow(from: io.undertow.server.handlers.form.FormData.FormValue) = { - if (!from.isFile) FormValue(from.getValue, from.getHeaders) - else FormFile(from.getFileName, from.getPath, from.getHeaders) + def fromUndertow(from: io.undertow.server.handlers.form.FormData.FormValue): FormEntry = { + val isOctetStream = Option(from.getHeaders) + .flatMap(headers => Option(headers.get(HttpString.tryFromString("Content-Type")))) + .exists(h => h.asScala.exists(v => v == "application/octet-stream")) + // browsers will set empty file fields to content type: octet-stream + if (isOctetStream || from.isFileItem) FormFile(from.getFileName, Try(from.getFileItem.getFile).toOption, from.getHeaders) + else FormValue(from.getValue, from.getHeaders) } } @@ -110,7 +116,9 @@ case class FormValue(value: String, } case class FormFile(fileName: String, - filePath: java.nio.file.Path, + filePath: Option[java.nio.file.Path], headers: io.undertow.util.HeaderMap) extends FormEntry{ def valueOrFileName = fileName } + +case class EmptyFormEntry() diff --git a/example/multipartFormSubmission/app/resources/example.txt b/example/multipartFormSubmission/app/resources/example.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/example/multipartFormSubmission/app/src/MultipartFormSubmission.scala b/example/multipartFormSubmission/app/src/MultipartFormSubmission.scala new file mode 100644 index 0000000000..f67ded041c --- /dev/null +++ b/example/multipartFormSubmission/app/src/MultipartFormSubmission.scala @@ -0,0 +1,26 @@ +package app + +object MultipartFormSubmission extends cask.MainRoutes { + + @cask.get("/") + def index() = + cask.model.Response( + """ + + + + +
+ + +
+ + + """, 200, Seq(("Content-Type", "text/html"))) + + @cask.postForm("/post") + def post(somefile: cask.FormFile) = + s"filename: ${somefile.fileName}" + + initialize() +} diff --git a/example/multipartFormSubmission/app/test/src/ExampleTests.scala b/example/multipartFormSubmission/app/test/src/ExampleTests.scala new file mode 100644 index 0000000000..9c065ead09 --- /dev/null +++ b/example/multipartFormSubmission/app/test/src/ExampleTests.scala @@ -0,0 +1,34 @@ +package app +import io.undertow.Undertow + +import utest._ + +object ExampleTests extends TestSuite{ + def withServer[T](example: cask.main.Main)(f: String => T): T = { + val server = Undertow.builder + .addHttpListener(8081, "localhost") + .setHandler(example.defaultHandler) + .build + server.start() + val res = + try f("http://localhost:8081") + finally server.stop() + res + } + + val tests = Tests { + test("MultipartFormSubmission") - withServer(MultipartFormSubmission) { host => + val withFile = requests.post(s"$host/post", data = requests.MultiPart( + requests.MultiItem("somefile", Array[Byte](1,2,3,4,5) , "example.txt"), + )) + withFile.text() ==> s"filename: example.txt" + withFile.statusCode ==> 200 + + val withoutFile = requests.post(s"$host/post", data = requests.MultiPart( + requests.MultiItem("somefile", Array[Byte]()), + )) + withoutFile.text() ==> s"filename: null" + withoutFile.statusCode ==> 200 + } + } +} diff --git a/example/multipartFormSubmission/package.mill b/example/multipartFormSubmission/package.mill new file mode 100644 index 0000000000..1908ce5236 --- /dev/null +++ b/example/multipartFormSubmission/package.mill @@ -0,0 +1,18 @@ +package build.example.multipartFormSubmission +import mill._, scalalib._ + +object app extends Cross[AppModule](build.scalaVersions) +trait AppModule extends CrossScalaModule{ + + def moduleDeps = Seq(build.cask(crossScalaVersion)) + + def ivyDeps = Agg[Dep]( + ) + object test extends ScalaTests with TestModule.Utest{ + + def ivyDeps = Agg( + ivy"com.lihaoyi::utest::0.8.4", + ivy"com.lihaoyi::requests::0.9.0", + ) + } +}