Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes form submissions with empty file fields throwing exceptions #150

Merged

Conversation

nikosk
Copy link
Contributor

@nikosk nikosk commented Nov 12, 2024

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

This results to FormData with a FormValue where the fileitem field is null.

image

So when cask tries to convert the Undertow FormValues 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

image

image

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

@lihaoyi
Copy link
Member

lihaoyi commented Nov 14, 2024

@nikosk can you update the PR description with an explanation of the problem and how your solution fixes it

@nikosk
Copy link
Contributor Author

nikosk commented Nov 17, 2024

@lihaoyi I did my best to document the problem. lmk if there's anything else I can do.

PS: I've added a test case which passes but the checks are failing at the generate docs step, no idea what that is about.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 27, 2024

Thanks!

@lihaoyi lihaoyi merged commit cbe92cd into com-lihaoyi:master Dec 27, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Submitting a form with an empty file field throws exception
2 participants