Change Akka HTTP backend to use raw headers (#8314) (#8319)

* Change Akka HTTP backend to use raw headers

Fixes #7733, #7735, #7719, #7997, #8205.

Maybe fixed, but not verified: #7737, #8149.

Most request and response headers are now unparsed by Akka HTTP
and stored as RawHeaders. That Akka HTTP won't log warnings, won't
reject headers and won't reformat headers.

Several headers are still parsed in order for Akka HTTP to
function properly: Content-Type, Content-Encoding, WebSocket related
headers, etc. These headers are in a whitelist in the Akka HTTP
source.

* Update test to expect 431 status code for long headers

Since Akka HTTP 10.0.12 the status code has changed.

- See change to Akka HTTP:
  akka/akka-http#1800.
- Related PR in Play:
  #8162

framework/project/Dependencies.scala

@@ -9,7 +9,7 @@ import buildinfo.BuildInfo
 object Dependencies {
 
   val akkaVersion = "2.5.11"
-  val akkaHttpVersion = "10.1.0"
+  val akkaHttpVersion = "10.1.1"
   val playJsonVersion = "2.6.9"
 
   val logback = "ch.qos.logback" % "logback-classic" % "1.2.3"

framework/src/play-akka-http-server/src/main/scala/play/core/server/AkkaHttpServer.scala

@@ -60,24 +60,35 @@ class AkkaHttpServer(
 
   private val http2Enabled: Boolean = akkaServerConfig.getOptional[Boolean]("http2.enabled") getOrElse false
 
+  /**
+   * The underlying Config object used to initialize Akka HTTP. We patch in a setting to enable
+   * or disable HTTP/2.
+   */
+  private val initialConfig: Config = (Configuration(system.settings.config) ++ Configuration(
+    "akka.http.server.preview.enable-http2" -> http2Enabled
+  )).underlying
+
+  /**
+   * Parses the config setting `infinite` as `Long.MaxValue` otherwise uses Config's built-in
+   * parsing of byte values.
+   */
   private def getPossiblyInfiniteBytes(config: Config, path: String): Long = {
     config.getString(path) match {
       case "infinite" => Long.MaxValue
-      case x => config.getBytes(path)
+      case _ => config.getBytes(path)
     }
   }
 
-  private def createServerBinding(port: Int, connectionContext: ConnectionContext, secure: Boolean): Http.ServerBinding = {
-    // Listen for incoming connections and handle them with the `handleRequest` method.
-
-    val initialConfig = (Configuration(system.settings.config) ++ Configuration(
-      "akka.http.server.preview.enable-http2" -> http2Enabled
-    )).underlying
-
-    val parserSettings = ParserSettings(initialConfig)
-      .withMaxContentLength(getPossiblyInfiniteBytes(akkaServerConfig.underlying, "max-content-length"))
-      .withIncludeTlsSessionInfoHeader(akkaServerConfig.get[Boolean]("tls-session-info-header"))
+  /**
+   * Play's custom parser settings for Akka HTTP.
+   */
+  private val parserSettings: ParserSettings = ParserSettings(initialConfig)
+    .withMaxContentLength(getPossiblyInfiniteBytes(akkaServerConfig.underlying, "max-content-length"))
+    .withIncludeTlsSessionInfoHeader(akkaServerConfig.get[Boolean]("tls-session-info-header"))
+    .withModeledHeaderParsing(false) // Disable most of Akka HTTP's header parsing; use RawHeaders instead
 
+  /** Listen for incoming connections and handle them with the `handleRequest` method. */
+  private def createServerBinding(port: Int, connectionContext: ConnectionContext, secure: Boolean): Http.ServerBinding = {
     val initialSettings = ServerSettings(initialConfig)
 
     val idleTimeout = serverConfig.get[Duration](if (secure) "https.idleTimeout" else "http.idleTimeout")
@@ -169,7 +180,10 @@ class AkkaHttpServer(
       val serverResultUtils = reloadServerResultUtils(tryApp)
       val forwardedHeaderHandler = reloadForwardedHeaderHandler(tryApp)
       val illegalResponseHeaderValue = ParserSettings.IllegalResponseHeaderValueProcessingMode(akkaServerConfig.get[String]("illegal-response-header-value-processing-mode"))
-      val modelConversion = new AkkaModelConversion(serverResultUtils, forwardedHeaderHandler, illegalResponseHeaderValue)
+      val modelConversion = new AkkaModelConversion(
+        serverResultUtils,
+        forwardedHeaderHandler,
+        illegalResponseHeaderValue)
       ReloadCacheValues(
         resultUtils = serverResultUtils,
         modelConversion = modelConversion

framework/src/play-akka-http-server/src/main/scala/play/core/server/akkahttp/AkkaModelConversion.scala

@@ -6,8 +6,8 @@ package play.core.server.akkahttp
 import java.net.{ InetAddress, InetSocketAddress, URI }
 import java.security.cert.X509Certificate
 import java.util.Locale
-import javax.net.ssl.SSLPeerUnverifiedException
 
+import javax.net.ssl.SSLPeerUnverifiedException
 import akka.http.scaladsl.model.Uri.Query
 import akka.http.scaladsl.model._
 import akka.http.scaladsl.model.headers._
@@ -44,42 +44,10 @@ private[server] class AkkaModelConversion(
    * for its body.
    */
   def convertRequest(requestId: Long, remoteAddress: InetSocketAddress, secureProtocol: Boolean, request: HttpRequest)(implicit fm: Materializer): (RequestHeader, Either[ByteString, Source[ByteString, Any]]) = {
-
     (
       convertRequestHeader(requestId, remoteAddress, secureProtocol, request),
       convertRequestBody(request)
     )
-
-    //    // FIXME this is if you want to try out avoiding conversion
-    //    (
-    //      new RequestHeaderImpl(
-    //        forwardedHeaderHandler.forwardedConnection(
-    //          new RemoteConnection {
-    //            override def remoteAddress: InetAddress = InetAddress.getLocalHost
-    //            override def secure: Boolean = secureProtocol
-    //            // TODO - Akka does not yet expose the SSLEngine used for the request
-    //            override lazy val clientCertificateChain = None
-    //          },
-    //          Headers()),
-    //        request.method.name,
-    //        new RequestTarget {
-    //          override lazy val uri: URI = new URI(uriString)
-    //          override lazy val uriString: String = request.header[`Raw-Request-URI`] match {
-    //            case None =>
-    //              logger.warn("Can't get raw request URI.")
-    //              request.uri.toString
-    //            case Some(rawUri) =>
-    //              rawUri.uri
-    //          }
-    //          override lazy val path: String = request.uri.path.toString
-    //          override lazy val queryMap: Map[String, Seq[String]] = request.uri.query().toMultiMap
-    //        },
-    //        request.protocol.value,
-    //        Headers(),
-    //        TypedMap.empty
-    //      ),
-    //      None
-    //    )
   }
 
   /**
@@ -300,39 +268,52 @@ private[server] class AkkaModelConversion(
     }
   }
 
+  // These headers are listed in the Akka HTTP's HttpResponseRenderer class as being invalid when given as RawHeaders
+  private val mustParseHeaders: Set[String] = Set(
+    HeaderNames.CONTENT_TYPE, HeaderNames.CONTENT_LENGTH, HeaderNames.TRANSFER_ENCODING, HeaderNames.DATE,
+    HeaderNames.SERVER, HeaderNames.CONNECTION
+  ).map(_.toLowerCase(Locale.ROOT))
+
   private def convertHeaders(headers: Iterable[(String, String)]): immutable.Seq[HttpHeader] = {
     headers.flatMap {
-      case (HeaderNames.SET_COOKIE, value) =>
-        resultUtils.splitSetCookieHeaderValue(value).map(RawHeader(HeaderNames.SET_COOKIE, _))
-      case (HeaderNames.CONTENT_DISPOSITION, value) =>
-        RawHeader(HeaderNames.CONTENT_DISPOSITION, value) :: Nil
-      case (name, value) if name != HeaderNames.TRANSFER_ENCODING =>
-        HttpHeader.parse(name, value) match {
-          case HttpHeader.ParsingResult.Ok(header, errors /* errors are ignored if Ok */ ) =>
-            if (!header.renderInResponses()) {
-              // since play did not enforce the http spec when it came to headers
-              // we actually relax it by converting the parsed header to a RawHeader
-              // This will still fail on content-type, content-length, transfer-encoding, date, server and connection headers.
-              illegalResponseHeaderValue match {
-                case ParserSettings.IllegalResponseHeaderValueProcessingMode.Warn =>
-                  logger.warn(s"HTTP Header '$header' is not allowed in responses, you can turn off this warning by setting `play.server.akka.illegal-response-header-value-processing-mode = ignore`")
-                  RawHeader(name, value) :: Nil
-                case ParserSettings.IllegalResponseHeaderValueProcessingMode.Ignore =>
-                  RawHeader(name, value) :: Nil
-                case ParserSettings.IllegalResponseHeaderValueProcessingMode.Error =>
-                  logger.error(s"HTTP Header '$header' is not allowed in responses")
-                  Nil
-              }
-            } else {
-              header :: Nil
-            }
-          case HttpHeader.ParsingResult.Error(error) =>
-            sys.error(s"Error parsing header: $error")
+      case (name, value) =>
+        val lowerName = name.toLowerCase(Locale.ROOT)
+        if (lowerName == "set-cookie") {
+          resultUtils.splitSetCookieHeaderValue(value).map(RawHeader(HeaderNames.SET_COOKIE, _))
+        } else if (mustParseHeaders.contains(lowerName)) {
+          parseHeader(name, value)
+        } else {
+          resultUtils.validateHeaderNameChars(name)
+          resultUtils.validateHeaderValueChars(value)
+          RawHeader(name, value) :: Nil
         }
-
-      case _ => Nil
     }(collection.breakOut): Vector[HttpHeader]
   }
+
+  private def parseHeader(name: String, value: String): Seq[HttpHeader] = {
+    HttpHeader.parse(name, value) match {
+      case HttpHeader.ParsingResult.Ok(header, errors /* errors are ignored if Ok */ ) =>
+        if (!header.renderInResponses()) {
+          // since play did not enforce the http spec when it came to headers
+          // we actually relax it by converting the parsed header to a RawHeader
+          // This will still fail on content-type, content-length, transfer-encoding, date, server and connection headers.
+          illegalResponseHeaderValue match {
+            case ParserSettings.IllegalResponseHeaderValueProcessingMode.Warn =>
+              logger.warn(s"HTTP Header '$header' is not allowed in responses, you can turn off this warning by setting `play.server.akka.illegal-response-header-value-processing-mode = ignore`")
+              RawHeader(name, value) :: Nil
+            case ParserSettings.IllegalResponseHeaderValueProcessingMode.Ignore =>
+              RawHeader(name, value) :: Nil
+            case ParserSettings.IllegalResponseHeaderValueProcessingMode.Error =>
+              logger.error(s"HTTP Header '$header' is not allowed in responses")
+              Nil
+          }
+        } else {
+          header :: Nil
+        }
+      case HttpHeader.ParsingResult.Error(error) =>
+        sys.error(s"Error parsing header: $error")
+    }
+  }
 }
 
 final case class AkkaHeadersWrapper(

framework/src/play-filters-helpers/src/test/scala/play/filters/csrf/CSRFCommonSpecs.scala

@@ -49,7 +49,7 @@ trait CSRFCommonSpecs extends Specification with PlaySpecification {
       |Content-Disposition: form-data; name="$tokenName"
       |
       |$tokenValue
-      |--$Boundary--""".stripMargin.replaceAll(System.lineSeparator, "\r\n")
+      |--$Boundary--""".stripMargin.replaceAll("\r?\n", "\r\n")
   }
 
   // This extracts the tests out into different configurations

framework/src/play-integration-test/src/test/scala/play/it/LogTester.scala

@@ -3,56 +3,45 @@
  */
 package play.it
 
-import org.slf4j.LoggerFactory
-import ch.qos.logback.core.AppenderBase
+import ch.qos.logback.classic.Logger
 import ch.qos.logback.classic.spi.ILoggingEvent
-import scala.collection.mutable.ListBuffer
-import ch.qos.logback.classic.{ Logger, LoggerContext, Level }
+import ch.qos.logback.core.AppenderBase
+import org.slf4j.LoggerFactory
+
+import scala.collection.immutable
+import scala.collection.mutable.ArrayBuffer
 
 /**
  * Test utility for testing Play logs
  */
 object LogTester {
 
-  def withLogBuffer[T](block: LogBuffer => T) = {
-    val ctx = LoggerFactory.getILoggerFactory.asInstanceOf[LoggerContext]
-    val root = ctx.getLogger("ROOT")
-    val rootLevel = root.getLevel
-    val playLogger = play.api.Logger(this.getClass).asInstanceOf[Logger]
-    val playLevel = playLogger.getLevel
-    val appender = new LogBuffer()
-    appender.start()
-    try {
-      root.addAppender(appender)
-      root.setLevel(Level.ALL)
-      playLogger.addAppender(appender)
-      playLogger.setLevel(Level.ALL)
-      block(appender)
-    } finally {
-      root.detachAppender(appender)
-      root.setLevel(rootLevel)
-      playLogger.detachAppender(appender)
-      playLogger.setLevel(playLevel)
+  /** Record log events and return them for analysis. */
+  def recordLogEvents[T](block: => T): (T, immutable.Seq[ILoggingEvent]) = {
 
-    }
-  }
+    /** Collects all log events that occur */
+    class RecordingAppender extends AppenderBase[ILoggingEvent] {
+      private val eventBuffer = ArrayBuffer[ILoggingEvent]()
 
-}
+      override def append(e: ILoggingEvent): Unit = synchronized {
+        eventBuffer += e
+      }
 
-class LogBuffer extends AppenderBase[ILoggingEvent] {
-  private val buffer = ListBuffer.empty[ILoggingEvent]
+      def events: immutable.Seq[ILoggingEvent] = synchronized {
+        eventBuffer.to[immutable.Seq]
+      }
+    }
 
-  def append(eventObject: ILoggingEvent) = buffer.synchronized {
-    buffer.append(eventObject)
+    // Get the Logback root logger and attach a RecordingAppender
+    val rootLogger = LoggerFactory.getLogger(org.slf4j.Logger.ROOT_LOGGER_NAME).asInstanceOf[Logger]
+    val appender = new RecordingAppender()
+    appender.setContext(rootLogger.getLoggerContext)
+    appender.start()
+    rootLogger.addAppender(appender)
+    val result: T = block
+    rootLogger.detachAppender(appender)
+    appender.stop()
+    (result, appender.events)
   }
 
-  def find(
-    level: Option[Level] = None,
-    logger: Option[String] = None,
-    messageContains: Option[String] = None): List[ILoggingEvent] = buffer.synchronized {
-    val byLevel = level.fold(buffer) { l => buffer.filter(_.getLevel == l) }
-    val byLogger = logger.fold(byLevel) { l => byLevel.filter(_.getLoggerName == l) }
-    val byMessageContains = logger.fold(byLogger) { m => byLogger.filter(_.getMessage.contains(m)) }
-    byMessageContains.toList
-  }
-}
+}

framework/src/play-integration-test/src/test/scala/play/it/http/AkkaResponseHeaderHandlingSpec.scala

@@ -6,7 +6,7 @@ package play.it.http
 import play.api.inject.guice.GuiceApplicationBuilder
 import play.api.mvc._
 import play.api.test.{ PlaySpecification, Port }
-import play.it.AkkaHttpIntegrationSpecification
+import play.it.{ AkkaHttpIntegrationSpecification, LogTester }
 
 class AkkaResponseHeaderHandlingSpec extends PlaySpecification with AkkaHttpIntegrationSpecification {
 
@@ -36,6 +36,32 @@ class AkkaResponseHeaderHandlingSpec extends PlaySpecification with AkkaHttpInte
       responses(0).headers.get("Authorization") must_== Some("invalid")
     }
 
+    "don't strip quotes from Link header" in withServer((Action, _) => Action { rh =>
+      // Test the header reported in https://github.com/playframework/playframework/issues/7733
+      Results.Ok.withHeaders("Link" -> """<http://example.com/some/url>; rel="next"""")
+    }) { port =>
+      val responses = BasicHttpClient.makeRequests(port)(
+        BasicRequest("GET", "/", "HTTP/1.1", Map(), "")
+      )
+      responses(0).headers.get("Link") must_== Some("""<http://example.com/some/url>; rel="next"""")
+    }
+
+    "don't log a warning for Set-Cookie headers with negative ages" in {
+      val problemHeaderValue = "PLAY_FLASH=; Max-Age=-86400; Expires=Tue, 30 Jan 2018 06:29:53 GMT; Path=/; HTTPOnly"
+      withServer((Action, _) => Action { rh =>
+        // Test the header reported in https://github.com/playframework/playframework/issues/8205
+        Results.Ok.withHeaders("Set-Cookie" -> problemHeaderValue)
+      }) { port =>
+        val (Seq(response), logMessages) = LogTester.recordLogEvents {
+          BasicHttpClient.makeRequests(port)(
+            BasicRequest("GET", "/", "HTTP/1.1", Map(), "")
+          )
+        }
+        response.status must_== 200
+        logMessages.map(_.getFormattedMessage) must not contain (contain(problemHeaderValue))
+      }
+    }
+
   }
 
 }