scotty-web / scotty

Haskell web framework inspired by Ruby's Sinatra, using WAI and Warp (Official Repository)
http://hackage.haskell.org/package/scotty
BSD 3-Clause "New" or "Revised" License
1.72k stars 134 forks source link

`files` function capturing empty input-file field. #396

Open tusharad opened 5 months ago

tusharad commented 5 months ago

Situation

In my scotty application. I have a HTML form that takes an input file, which is a optional field. I am using files function to capture the file. I assumed that if user doesn't upload a file, the files function would return an empty list. But it is returning a list containing FileInfo type with fileName as "\"\"".

Question:

Is this the correct behaviour? My workaround is if the fileContent is empty, I will reject the file or can there be any better solution for files?

When the list is empty:

Screenshot from 2024-04-25 18-48-48

Checkout full project code:

  1. files function code: https://github.com/tusharad/HaskRead/blob/25cdbe72fe2e8ba7297526de7644d0a503b22e3b/src/ScottyCrud/Handler.hs#L45

  2. HTML Form code: https://github.com/tusharad/HaskRead/blob/25cdbe72fe2e8ba7297526de7644d0a503b22e3b/src/ScottyCrud/HTML/Core.hs#L93

ocramz commented 5 months ago

Looks like a bug, but could you share first what scotty version is running?

tusharad commented 5 months ago

Hi, it's 0.22

ocramz commented 5 months ago

@tusharad could you please check whether previous versions of scotty have the same behaviour? I suspect not because the changes introduced in 0.22 move the files parsing behaviour to wai-extra.

We don't even have a test for the "no files" case, fwiw.

tusharad commented 5 months ago

Hi @ocramz , I used this example. Even after using the older version (0.12.1). On submitting without uploading file, It is given us list containing elements with filename as "\"\"". image

post upload code:

    post "/upload" $ do
      fileList <- files
      liftIO $ print fileList
      text **"done"**
ocramz commented 5 months ago

Thanks @tusharad for checking! This is odd and unintuitive behaviour, but I'm at least glad that it didn't appear in 0.22 .

Question now is, what's the cause of this, and how do we fix it?

tusharad commented 5 months ago

Hi @ocramz,

Upon investigating the files and filesOpts functions, I discovered that both rely on the formParamsAndFilesWith function. This function, in turn, utilizes getFormParamsAndFilesAction, which ultimately utilizes sinkRequestBodyEx from wai-extra. Location of sinkRequestBodyEx function.

I've provided two images illustrating the output of wholebody and fs at this location:

It appears that sinkRequestBodyEx parses the wholebody bytestring, extracting the filename and file content. However, I don't believe the issue lies with sinkRequestBodyEx itself, as it simply performs parsing.

To address this, I propose a simple solution: apply a filter function after sinkRequestBodyEx to exclude files with an empty filename ("\"\""). This solution has worked effectively in my testing.

Here's the proposed modification within the getFormParamsAndFilesAction function:

  bs <- getBodyAction bodyInfo opts
  let
    wholeBody = BL.toChunks bs
  (formparams, fs) <- parseRequestBodyExBS istate prbo wholeBody (W.getRequestBodyType req) `catches` handleWaiParseSafeExceptions
  -- changed here
  let fs' = filter emptyFile fs
  return (convertBoth <$> formparams, convertKey <$> fs')
    where emptyFile (_,fInfo) = ("\"\"" /= (W.fileName fInfo)) 

Please review and let me know if this approach aligns with your expectations. If approved, I'll proceed with creating a PR.

Thank you!

ocramz commented 5 months ago

@tusharad Yes, this fix seems sensible but it's also a breaking change that should be documented as such.

Happy to review your PR!