Merge pull request #4604 from vder/Content-Disposition-utf8-encoding

Content-Disposition utf8 encoding fix

core/src/main/scala/org/http4s/headers/Content-Disposition.scala

@@ -23,8 +23,11 @@ import org.http4s.internal.parsing.{Rfc2616, Rfc3986, Rfc7230}
 import org.http4s.util.{Renderable, Writer}
 import java.nio.charset.StandardCharsets
 import org.typelevel.ci._
+import scala.collection.immutable.TreeMap
 
 object `Content-Disposition` {
+  private val safeChars = CharPredicate.Printable -- "%\"\\"
+
   def parse(s: String): ParseResult[`Content-Disposition`] =
     ParseResult.fromParser(parser, "Invalid Content-Disposition header")(s)
 
@@ -64,10 +67,10 @@ object `Content-Disposition` {
     val parameter = for {
       tok <- Rfc7230.token <* Parser.string("=") <* Rfc7230.ows
       v <- if (tok.endsWith("*")) extValue else value
-    } yield (tok, v)
+    } yield (CIString(tok), v)
 
     (Rfc7230.token ~ (Parser.string(";") *> Rfc7230.ows *> parameter).rep0).map {
-      case (token: String, params: List[(String, String)]) =>
+      case (token: String, params: List[(CIString, String)]) =>
         `Content-Disposition`(token, params.toMap)
     }
   }
@@ -77,9 +80,28 @@ object `Content-Disposition` {
       ci"Content-Disposition",
       v =>
         new Renderable {
+          // Adapted from https://github.com/akka/akka-http/blob/b071bd67547714bd8bed2ccd8170fbbc6c2dbd77/akka-http-core/src/main/scala/akka/http/scaladsl/model/headers/headers.scala#L468-L492
           def render(writer: Writer): writer.type = {
+            val renderExtFilename =
+              v.parameters.get(ci"filename").exists(!safeChars.matchesAll(_))
+            val withExtParams =
+              if (renderExtFilename && !v.parameters.contains(ci"filename*"))
+                v.parameters + (ci"filename*" -> v.parameters(ci"filename"))
+              else v.parameters
+            val withExtParamsSorted =
+              if (withExtParams.contains(ci"filename") && withExtParams.contains(ci"filename*"))
+                TreeMap[CIString, String]() ++ withExtParams
+              else withExtParams
+
             writer.append(v.dispositionType)
-            v.parameters.foreach(p => writer << "; " << p._1 << "=\"" << p._2 << '"')
+            withExtParamsSorted.foreach {
+              case (k, v) if k == ci"filename" =>
+                writer << "; " << k << '=' << '"'
+                writer.eligibleOnly(v, keep = safeChars, placeholder = '?') << '"'
+              case (k @ ci"${_}*", v) =>
+                writer << "; " << k << '=' << "UTF-8''" << Uri.encode(v)
+              case (k, v) => writer << "; " << k << "=\"" << v << '"'
+            }
             writer
           }
         },
@@ -88,4 +110,4 @@ object `Content-Disposition` {
 }
 
 // see http://tools.ietf.org/html/rfc2183
-final case class `Content-Disposition`(dispositionType: String, parameters: Map[String, String])
+final case class `Content-Disposition`(dispositionType: String, parameters: Map[CIString, String])

core/src/main/scala/org/http4s/multipart/Part.scala

@@ -25,11 +25,12 @@ import fs2.text.utf8Encode
 import java.io.{File, InputStream}
 import java.net.URL
 import org.http4s.headers.`Content-Disposition`
+import org.typelevel.ci._
 
 final case class Part[F[_]](headers: Headers, body: Stream[F, Byte]) extends Media[F] {
-  def name: Option[String] = headers.get[`Content-Disposition`].flatMap(_.parameters.get("name"))
+  def name: Option[String] = headers.get[`Content-Disposition`].flatMap(_.parameters.get(ci"name"))
   def filename: Option[String] =
-    headers.get[`Content-Disposition`].flatMap(_.parameters.get("filename"))
+    headers.get[`Content-Disposition`].flatMap(_.parameters.get(ci"filename"))
 
   override def covary[F2[x] >: F[x]]: Part[F2] = this.asInstanceOf[Part[F2]]
 }
@@ -48,7 +49,7 @@ object Part {
 
   def formData[F[_]](name: String, value: String, headers: Header.ToRaw*): Part[F] =
     Part(
-      Headers(`Content-Disposition`("form-data", Map("name" -> name))).put(headers: _*),
+      Headers(`Content-Disposition`("form-data", Map(ci"name" -> name))).put(headers: _*),
       Stream.emit(value).through(utf8Encode))
 
   def fileData[F[_]: Sync: ContextShift](
@@ -72,7 +73,7 @@ object Part {
       headers: Header.ToRaw*): Part[F] =
     Part(
       Headers(
-        `Content-Disposition`("form-data", Map("name" -> name, "filename" -> filename)),
+        `Content-Disposition`("form-data", Map(ci"name" -> name, ci"filename" -> filename)),
         "Content-Transfer-Encoding" -> "binary"
       ).put(headers: _*),
       entityBody

core/src/main/scala/org/http4s/util/Renderable.scala

@@ -27,6 +27,7 @@ import org.typelevel.ci.CIString
 import scala.annotation.tailrec
 import scala.collection.immutable.BitSet
 import scala.concurrent.duration.FiniteDuration
+import org.http4s.internal.CharPredicate
 
 /** A type class that describes how to efficiently render a type
   * @tparam T the type which will be rendered
@@ -160,6 +161,17 @@ trait Writer {
     go(0)
     this << '"'
   }
+  //Adapted from https://github.com/akka/akka-http/blob/b071bd67547714bd8bed2ccd8170fbbc6c2dbd77/akka-http-core/src/main/scala/akka/http/impl/util/Rendering.scala#L219-L229
+  def eligibleOnly(s: String, keep: CharPredicate, placeholder: Char): this.type = {
+    @tailrec def rec(ix: Int = 0): this.type =
+      if (ix < s.length) {
+        val c = s.charAt(ix)
+        if (keep(c)) this << c
+        else this << placeholder
+        rec(ix + 1)
+      } else this
+    rec()
+  }
 
   def addStrings(
       s: collection.Seq[String],

tests/src/test/scala/org/http4s/multipart/MultipartParserSuite.scala

@@ -23,6 +23,7 @@ import fs2._
 import org.http4s._
 import org.http4s.headers._
 import org.http4s.util._
+import org.typelevel.ci._
 
 import java.nio.charset.StandardCharsets
 
@@ -92,7 +93,7 @@ class MultipartParserSuite extends Http4sSuite {
         val expectedHeaders = Headers(
           `Content-Disposition`(
             "form-data",
-            Map("name" -> "upload", "filename" -> "integration.txt")),
+            Map(ci"name" -> "upload", ci"filename" -> "integration.txt")),
           `Content-Type`(MediaType.application.`octet-stream`),
           "Content-Transfer-Encoding" -> "binary"
         )
@@ -144,7 +145,7 @@ class MultipartParserSuite extends Http4sSuite {
       val expectedHeaders = Headers(
         `Content-Disposition`(
           "form-data",
-          Map("name" -> "upload", "filename" -> "integration.txt")),
+          Map(ci"name" -> "upload", ci"filename" -> "integration.txt")),
         `Content-Type`(MediaType.application.`octet-stream`),
         "Content-Transfer-Encoding" -> "binary"
       )
@@ -189,7 +190,7 @@ class MultipartParserSuite extends Http4sSuite {
           .through(multipartPipe(boundary))
 
       val expectedHeaders = Headers(
-        `Content-Disposition`("form-data", Map("name*" -> "http4s很棒", "filename*" -> "我老婆太漂亮.txt")),
+        "Content-Disposition" -> """form-data; name*="http4s很棒"; filename*="我老婆太漂亮.txt"""",
         `Content-Type`(MediaType.application.`octet-stream`),
         "Content-Transfer-Encoding" -> "binary"
       )
@@ -210,6 +211,7 @@ class MultipartParserSuite extends Http4sSuite {
             .foldMonoid
         result <- bodies.attempt
       } yield {
+
         assertEquals(headers, expectedHeaders)
         assertEquals(result, Right(expected))
       }
@@ -218,7 +220,7 @@ class MultipartParserSuite extends Http4sSuite {
     test(s"$testNamePrefix: parse characterset encoded headers properly") {
       val unprocessedInput =
         """--_5PHqf8_Pl1FCzBuT5o_mVZg36k67UYI
-            |Content-Disposition: form-data; name*=UTF-8''http4s%20withspace; filename*="我老婆太漂亮.txt"
+            |Content-Disposition: form-data; name*=UTF-8''http4s%20withspace; filename*=UTF-8''%E6%88%91%E8%80%81%E5%A9%86%E5%A4%AA%E6%BC%82%E4%BA%AE.txt
             |Content-Type: application/octet-stream
             |Content-Transfer-Encoding: binary
             |
@@ -235,7 +237,9 @@ class MultipartParserSuite extends Http4sSuite {
 
       val expectedHeaders = Headers(
         // #4513 for why this isn't a modeled header
-        "Content-Disposition" -> """form-data; name*=UTF-8''http4s%20withspace; filename*="我老婆太漂亮.txt"""",
+        `Content-Disposition`(
+          "form-data",
+          Map(ci"name*" -> "http4s withspace", ci"filename*" -> "我老婆太漂亮.txt")),
         `Content-Type`(MediaType.application.`octet-stream`),
         "Content-Transfer-Encoding" -> "binary"
       )
@@ -279,7 +283,7 @@ class MultipartParserSuite extends Http4sSuite {
       val expectedHeaders = Headers(
         `Content-Disposition`(
           "form-data",
-          Map("name" -> "upload", "filename" -> "integration.txt")),
+          Map(ci"name" -> "upload", ci"filename" -> "integration.txt")),
         `Content-Type`(MediaType.application.`octet-stream`),
         "Content-Transfer-Encoding" -> "binary"
       )
@@ -373,7 +377,7 @@ class MultipartParserSuite extends Http4sSuite {
       val expectedHeaders = Headers(
         `Content-Disposition`(
           "form-data",
-          Map("name" -> "upload", "filename" -> "integration.txt")),
+          Map(ci"name" -> "upload", ci"filename" -> "integration.txt")),
         `Content-Type`(MediaType.application.`octet-stream`),
         "Content-Transfer-Encoding" -> "binary"
       )
@@ -420,7 +424,7 @@ class MultipartParserSuite extends Http4sSuite {
       val expectedHeaders = Headers(
         `Content-Disposition`(
           "form-data",
-          Map("name" -> "upload", "filename" -> "integration.txt")),
+          Map(ci"name" -> "upload", ci"filename" -> "integration.txt")),
         `Content-Type`(MediaType.application.`octet-stream`),
         "Content-Transfer-Encoding" -> "binary"
       )
@@ -574,11 +578,11 @@ class MultipartParserSuite extends Http4sSuite {
         .assertEquals(
           List(
             Headers(
-              `Content-Disposition`("form-data", Map("name" -> "field1")),
+              `Content-Disposition`("form-data", Map(ci"name" -> "field1")),
               `Content-Type`(MediaType.text.plain)
             ),
             Headers(
-              `Content-Disposition`("form-data", Map("name" -> "field2"))
+              `Content-Disposition`("form-data", Map(ci"name" -> "field2"))
             )
           )
         )
@@ -616,7 +620,7 @@ class MultipartParserSuite extends Http4sSuite {
         assertEquals(
           firstPart.headers,
           Headers(
-            `Content-Disposition`("form-data", Map("name" -> "field1")),
+            `Content-Disposition`("form-data", Map(ci"name" -> "field1")),
             `Content-Type`(MediaType.text.plain)))
         assert(confirmedError.isInstanceOf[Left[_, _]])
         assert(
@@ -711,7 +715,7 @@ class MultipartParserSuite extends Http4sSuite {
       .assertEquals(
         List(
           Headers(
-            `Content-Disposition`("form-data", Map("name" -> "field1")),
+            `Content-Disposition`("form-data", Map(ci"name" -> "field1")),
             `Content-Type`(MediaType.text.plain)
           )
         ))

tests/src/test/scala/org/http4s/multipart/MultipartSuite.scala

@@ -25,6 +25,7 @@ import java.io.File
 import org.http4s.headers._
 import org.http4s.syntax.literals._
 import org.http4s.EntityEncoder._
+import org.typelevel.ci._
 
 class MultipartSuite extends Http4sSuite {
   implicit val contextShift: ContextShift[IO] = Http4sSuite.TestContextShift
@@ -162,15 +163,17 @@ I am a big moose
 
       test(s"Multipart form data $name should extract name properly if it is present") {
         val part = Part(
-          Headers(`Content-Disposition`("form-data", Map("name" -> "Rich Homie Quan"))),
+          Headers(`Content-Disposition`("form-data", Map(ci"name" -> "Rich Homie Quan"))),
           Stream.empty.covary[IO])
         assertEquals(part.name, Some("Rich Homie Quan"))
       }
 
       test(s"Multipart form data $name should extract filename property if it is present") {
         val part = Part(
           Headers(
-            `Content-Disposition`("form-data", Map("name" -> "file", "filename" -> "file.txt"))),
+            `Content-Disposition`(
+              "form-data",
+              Map(ci"name" -> "file", ci"filename" -> "file.txt"))),
           Stream.empty.covary[IO]
         )
         assertEquals(part.filename, Some("file.txt"))

tests/src/test/scala/org/http4s/parser/ContentDispositionHeaderSuite.scala

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2013 http4s.org
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.http4s
+package parser
+
+import org.http4s.headers.`Content-Disposition`
+import org.http4s.syntax.header._
+import org.typelevel.ci._
+
+class ContentDispositionHeaderSuite extends Http4sSuite {
+  def parse(value: String): ParseResult[`Content-Disposition`] =
+    `Content-Disposition`.parse(value)
+
+  test("ContentDisposition Header should render the correct values") {
+
+    val wrongEncoding = `Content-Disposition`("form-data", Map(ci"filename" -> "http4s łł"))
+    val correctOrder =
+      `Content-Disposition`("form-data", Map(ci"filename*" -> "value1", ci"filename" -> "value2"))
+
+    assertEquals(
+      wrongEncoding.renderString,
+      """Content-Disposition: form-data; filename="http4s ??"; filename*=UTF-8''http4s%20%C5%82%C5%82""")
+    assertEquals(
+      correctOrder.renderString,
+      """Content-Disposition: form-data; filename="value2"; filename*=UTF-8''value1""")
+  }
+
+}

tests/src/test/scala/org/http4s/parser/SimpleHeadersSpec.scala

@@ -89,7 +89,7 @@ class SimpleHeadersSpec extends Http4sSuite {
   }
 
   test("SimpleHeaders should parse Content-Disposition") {
-    val header = `Content-Disposition`("foo", Map("one" -> "two", "three" -> "four"))
+    val header = `Content-Disposition`("foo", Map(ci"one" -> "two", ci"three" -> "four"))
     val parse = `Content-Disposition`.parse(_)
     assertEquals(parse(header.value), Right(header))