Improve decoding of form fields in multipart requets. [fixes #83]

fileupload/src/main/scala/org/scalatra/fileupload/FileUploadSupport.scala

@@ -3,7 +3,7 @@ package fileupload
 
 import org.apache.commons.fileupload.servlet.ServletFileUpload
 import org.apache.commons.fileupload.{FileItemFactory, FileItem}
-import org.apache.commons.fileupload.disk.DiskFileItemFactory
+import org.apache.commons.fileupload.disk.{DiskFileItem, DiskFileItemFactory}
 import collection.JavaConversions._
 import scala.util.DynamicVariable
 import java.util.{List => JList, HashMap => JHashMap, Map => JMap}
@@ -49,14 +49,36 @@ trait FileUploadSupport extends ScalatraKernel {
         val items = upload.parseRequest(req).asInstanceOf[JList[FileItem]]
         val bodyParams = items.foldRight(BodyParams(Map.empty, Map.empty)) { (item, params) =>
           if (item.isFormField)
-            BodyParams(params.fileParams, params.formParams + ((item.getFieldName, item.getString :: params.formParams.getOrElse(item.getFieldName, List[String]()))))
+            BodyParams(params.fileParams, params.formParams + ((item.getFieldName, fileItemToString(req, item) :: params.formParams.getOrElse(item.getFieldName, List[String]()))))
           else
             BodyParams(params.fileParams + ((item.getFieldName, item :: params.fileParams.getOrElse(item.getFieldName, List[FileItem]()))), params.formParams)
           }
         req(BodyParamsKey) = bodyParams
         bodyParams
     }
 
+  /**
+   * Converts a file item to a string.
+   *
+   * Browsers tend to be sloppy about providing content type headers with
+   * charset information to form-data parts.  Worse, browsers are
+   * inconsistent about how they encode these parameters. 
+   *
+   * The default implementation attempts to use the charset specified on
+   * the request.  If that is unspecified, and it usually isn't, then it
+   * falls back to the kernel's charset.
+   */
+  protected def fileItemToString(req: HttpServletRequest, item: FileItem): String = {
+    val charset = item match {
+      case diskItem: DiskFileItem =>
+        // Why doesn't FileItem have this method???
+        Option(diskItem.getCharSet())
+      case _ => 
+        None
+    }
+    item.getString(charset getOrElse defaultCharacterEncoding)
+  }
+
   private def wrapRequest(req: HttpServletRequest, formMap: Map[String, Seq[String]]) =
     new HttpServletRequestWrapper(req) {
       override def getParameter(name: String) = formMap.get(name) map { _.head } getOrElse null

fileupload/src/test/resources/org/scalatra/fileupload/multipart_request.txt

@@ -1,32 +1,36 @@
 POST ${PATH} HTTP/1.0
-Content-Type: multipart/form-data; boundary=---------------------------768277191987740441310253794
-Content-Length: 816
+Content-Type: multipart/form-data; boundary=---------------------------3924013385056820061124200860
+Content-Length: 944
 
------------------------------768277191987740441310253794
+-----------------------------3924013385056820061124200860
 Content-Disposition: form-data; name="string"
 
 foo
------------------------------768277191987740441310253794
+-----------------------------3924013385056820061124200860
+Content-Disposition: form-data; name="utf8-string"
+
+föo
+-----------------------------3924013385056820061124200860
 Content-Disposition: form-data; name="file"; filename="1.txt"
 Content-Type: text/plain
 
 one
 
------------------------------768277191987740441310253794
+-----------------------------3924013385056820061124200860
 Content-Disposition: form-data; name="file-none"; filename=""
 Content-Type: application/octet-stream
 
 
------------------------------768277191987740441310253794
+-----------------------------3924013385056820061124200860
 Content-Disposition: form-data; name="file-multi"; filename="2.txt"
 Content-Type: text/plain
 
 two
 
------------------------------768277191987740441310253794
+-----------------------------3924013385056820061124200860
 Content-Disposition: form-data; name="file-multi"; filename="3.txt"
 Content-Type: text/plain
 
 three
 
------------------------------768277191987740441310253794--
+-----------------------------3924013385056820061124200860--

fileupload/src/test/scala/org/scalatra/fileupload/FileUploadSupportTest.scala

@@ -1,6 +1,7 @@
 package org.scalatra
 package fileupload
 
+import java.net.{URLDecoder, URLEncoder}
 import org.scalatest.FunSuite
 import org.eclipse.jetty.testing.{ServletTester, HttpTester}
 import org.apache.commons.io.IOUtils
@@ -18,10 +19,10 @@ class FileUploadSupportTestServlet extends ScalatraServlet with FileUploadSuppor
       response.setHeader("file-multi-all", fis.foldLeft(""){ (acc, fi) => acc + new String(fi.get) })
     }
     params.get("file") foreach { response.setHeader("file-as-param", _) }
+    params("utf8-string")
   }
 
   post("/multipart-pass") {
-    println("PASSING")
     pass()
   }
 
@@ -42,8 +43,11 @@ class FileUploadSupportTest extends ScalatraFunSuite {
   addServlet(classOf[FileUploadSupportTestServlet], "/")
 
   def multipartResponse(path: String = "/multipart") = {
-    val req = IOUtils.toString(getClass.getResourceAsStream("multipart_request.txt"))
-      .replace("${PATH}", path)
+    // TODO We've had problems with the tester not running as iso-8859-1, even if the
+    // request really isn't iso-8859-1.  This is a hack, but this hack passes iff the
+    // browser behavior is correct.
+    val req = new String(IOUtils.toString(getClass.getResourceAsStream("multipart_request.txt"))
+      .replace("${PATH}", path).getBytes, "iso-8859-1")
     val res = new HttpTester("iso-8859-1")
     res.parse(tester.getResponses(req))
     res
@@ -53,6 +57,10 @@ class FileUploadSupportTest extends ScalatraFunSuite {
     multipartResponse().getHeader("string") should equal ("foo")
   }
 
+  test("decodes input parameters according to request encoding") {
+    multipartResponse().getContent() should equal ("föo")
+  }
+
   test("sets file params") {
     multipartResponse().getHeader("file") should equal ("one")
   }

notes/2.0.0.M5.markdown

@@ -9,6 +9,7 @@
 
 ## scalatra-fileupload
 * Keep query parameters available on multipart requests. [(GH-80)](http://github.com/scalatra/scalatra/issues/80)
+* Improve character decoding of form fields in multipart requests. [(GH-83)](http://github.com/scalatra/scalatra/issues/83)
 
 ## scalatra-socketio
 * Change interface to socket io to something that allows to keep state per client. [(GH-72)](http://github.com/scalatra/scalatra/issues/72)