Skip to content

Commit

Permalink
Fixes form submissions with empty file fields throwing exceptions (#150)
Browse files Browse the repository at this point in the history
Given a form with a file filed i.e. `<input type="file"/>` 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 <nk>
  • Loading branch information
nikosk authored Dec 27, 2024
1 parent 0b14e42 commit cbe92cd
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 5 deletions.
1 change: 1 addition & 0 deletions build.mill
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 13 additions & 5 deletions cask/src/cask/model/Params.scala
Original file line number Diff line number Diff line change
@@ -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])
Expand Down Expand Up @@ -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)
}

}
Expand All @@ -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()
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package app

object MultipartFormSubmission extends cask.MainRoutes {

@cask.get("/")
def index() =
cask.model.Response(
"""
<!DOCTYPE html>
<html lang="en">
<head></head>
<body>
<form action="/post" method="post" enctype="multipart/form-data">
<input type="file" id="somefile" name="somefile">
<button type="submit">Submit</button>
</form>
</body>
</html>
""", 200, Seq(("Content-Type", "text/html")))

@cask.postForm("/post")
def post(somefile: cask.FormFile) =
s"filename: ${somefile.fileName}"

initialize()
}
34 changes: 34 additions & 0 deletions example/multipartFormSubmission/app/test/src/ExampleTests.scala
Original file line number Diff line number Diff line change
@@ -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
}
}
}
18 changes: 18 additions & 0 deletions example/multipartFormSubmission/package.mill
Original file line number Diff line number Diff line change
@@ -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",
)
}
}

0 comments on commit cbe92cd

Please sign in to comment.