Skip to content

Commit

Permalink
Add HttpErrorInfo request attribute to track origins of errors
Browse files Browse the repository at this point in the history
  • Loading branch information
mkurz committed May 20, 2020
1 parent 3fe877c commit 510d4ec
Show file tree
Hide file tree
Showing 17 changed files with 213 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import akka.stream.scaladsl.Sink
import akka.util.ByteString
import play.api.Configuration
import play.api.Mode
import play.api.http.HttpErrorHandler
import play.api.inject.guice.GuiceApplicationBuilder
import play.api.libs.streams.Accumulator
import play.api.mvc._
Expand All @@ -19,6 +20,7 @@ import play.core.server.ServerConfig
import play.it._

import scala.concurrent.ExecutionContext.Implicits._
import scala.concurrent.Future
import scala.util.Random

class NettyRequestBodyHandlingSpec extends RequestBodyHandlingSpec with NettyIntegrationSpecification
Expand All @@ -33,9 +35,10 @@ trait RequestBodyHandlingSpec extends PlaySpecification with ServerIntegrationSp
)(action: (DefaultActionBuilder, PlayBodyParsers) => EssentialAction)(block: Port => T) = {
val port = testServerPort

val config = Configuration(configuration: _*)
val serverConfig: ServerConfig = {
val c = ServerConfig(port = Some(testServerPort), mode = Mode.Test)
c.copy(configuration = Configuration(configuration: _*).withFallback(c.configuration))
c.copy(configuration = config.withFallback(c.configuration))
}
running(
play.api.test.TestServer(
Expand All @@ -46,6 +49,7 @@ trait RequestBodyHandlingSpec extends PlaySpecification with ServerIntegrationSp
val parse = app.injector.instanceOf[PlayBodyParsers]
({ case _ => action(Action, parse) })
}
.configure(config)
.build(),
Some(integrationServerProvider)
)
Expand Down Expand Up @@ -133,7 +137,8 @@ trait RequestBodyHandlingSpec extends PlaySpecification with ServerIntegrationSp
}

"handle a big http request and fail with HTTP Error '413 request entity too large'" in withServerAndConfig(
"play.server.max-content-length" -> "21b"
"play.server.max-content-length" -> "21b",
"play.http.errorHandler" -> classOf[CustomErrorHandler].getName,
)((Action, parse) =>
Action(parse.default(Some(Long.MaxValue))) { rh =>
Results.Ok(rh.body.asText.getOrElse(""))
Expand All @@ -145,6 +150,7 @@ trait RequestBodyHandlingSpec extends PlaySpecification with ServerIntegrationSp
)
responses.length must_== 1
responses(0).status must_== 413
responses(0).body.left.getOrElse("") must_=== "Origin: server-backend / Request Entity Too Large"
}

"handle a big http request with exact amount of allowed Content-Length" in withServerAndConfig(
Expand All @@ -163,3 +169,17 @@ trait RequestBodyHandlingSpec extends PlaySpecification with ServerIntegrationSp
}
}
}

class CustomErrorHandler extends HttpErrorHandler {
def onClientError(request: RequestHeader, statusCode: Int, message: String) =
Future.successful(
Results.Status(statusCode)(
"Origin: " + request.attrs
.get(HttpErrorHandler.Attrs.HttpErrorInfo)
.map(_.origin)
.getOrElse("<not set>") + " / " + message
)
)
def onServerError(request: RequestHeader, exception: Throwable) =
Future.successful(Results.BadRequest)
}
8 changes: 8 additions & 0 deletions core/play/src/main/java/play/http/HttpErrorHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

package play.http;

import play.api.http.HttpErrorInfo;
import play.libs.typedmap.TypedKey;
import play.mvc.Http.RequestHeader;
import play.mvc.Result;

Expand Down Expand Up @@ -34,4 +36,10 @@ public interface HttpErrorHandler {
* @return a CompletionStage with the Result.
*/
CompletionStage<Result> onServerError(RequestHeader request, Throwable exception);

/** Request attributes used by the error handler. */
class Attrs {
public static final TypedKey<HttpErrorInfo> HTTP_ERROR_INFO =
new TypedKey<>(play.api.http.HttpErrorHandler.Attrs$.MODULE$.HttpErrorInfo());
}
}
9 changes: 9 additions & 0 deletions core/play/src/main/java/play/http/HttpErrorInfo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright (C) Lightbend Inc. <https://www.lightbend.com>
*/

package play.http;

public abstract class HttpErrorInfo {
public abstract String origin();
}
8 changes: 8 additions & 0 deletions core/play/src/main/scala/play/api/http/HttpErrorHandler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import play.api._
import play.api.http.Status._
import play.api.inject.Binding
import play.api.libs.json._
import play.api.libs.typedmap.TypedKey
import play.api.mvc.Results._
import play.api.mvc._
import play.api.routing.Router
Expand Down Expand Up @@ -109,6 +110,13 @@ object HttpErrorHandler {
DefaultHttpErrorHandler
](environment, configuration, "play.http.errorHandler", "ErrorHandler")
}

/**
* Request attributes used by the error handler.
*/
object Attrs {
val HttpErrorInfo: TypedKey[HttpErrorInfo] = TypedKey("HttpErrorInfo")
}
}

case class HttpErrorConfig(showDevErrors: Boolean = false, playEditor: Option[String] = None)
Expand Down
9 changes: 9 additions & 0 deletions core/play/src/main/scala/play/api/http/HttpErrorInfo.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright (C) Lightbend Inc. <https://www.lightbend.com>
*/

package play.api.http

case class HttpErrorInfo(
origin: String
) extends play.http.HttpErrorInfo
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import play.api._
import play.api.http.DefaultHttpErrorHandler
import play.api.http.HeaderNames
import play.api.http.HttpErrorHandler
import play.api.http.HttpErrorInfo
import play.api.http.{ HttpProtocol => PlayHttpProtocol }
import play.api.http.Status
import play.api.internal.libs.concurrent.CoordinatedShutdownSupport
Expand Down Expand Up @@ -412,7 +413,11 @@ class AkkaHttpServer(context: AkkaHttpServer.Context) extends Server {
}
.recoverWith {
case _: EntityStreamSizeException =>
errorHandler.onClientError(taggedRequestHeader, Status.REQUEST_ENTITY_TOO_LARGE, "Request Entity Too Large")
errorHandler.onClientError(
taggedRequestHeader.addAttr(HttpErrorHandler.Attrs.HttpErrorInfo, HttpErrorInfo("server-backend")),
Status.REQUEST_ENTITY_TOO_LARGE,
"Request Entity Too Large"
)
case e: Throwable =>
errorHandler.onServerError(taggedRequestHeader, e)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,11 @@ private[play] class PlayRequestHandler(
val unparsedTarget = modelConversion(tryApp).createUnparsedRequestTarget(request)
val requestHeader = modelConversion(tryApp).createRequestHeader(channel, request, unparsedTarget)
val debugHeader = attachDebugInfo(requestHeader)
val result = errorHandler(tryApp).onClientError(debugHeader, statusCode, if (message == null) "" else message)
val result = errorHandler(tryApp).onClientError(
debugHeader.addAttr(HttpErrorHandler.Attrs.HttpErrorInfo, HttpErrorInfo("server-backend")),
statusCode,
if (message == null) "" else message
)
// If there's a problem in parsing the request, then we should close the connection, once done with it
debugHeader -> Server.actionForResult(result.map(_.withHeaders(HeaderNames.CONNECTION -> "close")))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

import javax.inject.Inject;

import play.api.http.HttpErrorHandler;
import play.api.http.HttpErrorInfo;
import play.api.http.SessionConfiguration;
import play.api.libs.crypto.CSRFTokenSigner;
import play.api.mvc.RequestHeader;
Expand Down Expand Up @@ -128,7 +130,13 @@ private CompletionStage<Result> handleTokenError(
Http.Request req, RequestHeader taggedRequest, String msg) {
CSRFErrorHandler handler = configurator.apply(this.configuration);
return handler
.handle(taggedRequest.asJava(), msg)
.handle(
taggedRequest
.addAttr(
HttpErrorHandler.Attrs$.MODULE$.HttpErrorInfo(),
new HttpErrorInfo("csrf-filter"))
.asJava(),
msg)
.thenApply(
result -> {
if (CSRF.getToken(taggedRequest).isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import java.util.Locale
import akka.util.ByteString
import play.api.mvc._
import javax.inject._
import play.api.http.HttpErrorHandler
import play.api.http.HttpErrorInfo
import play.api.http.Status
import play.api.libs.json._
import play.api.libs.functional.syntax._
Expand Down Expand Up @@ -100,7 +102,13 @@ class DefaultCSPReportBodyParser @Inject() (parsers: PlayBodyParsers)(implicit e
@deprecated("Will be removed in an upcoming Play release", "2.9.0")
protected def createBadResult(msg: String, statusCode: Int = Status.BAD_REQUEST): RequestHeader => Future[Result] = {
request =>
parsers.errorHandler.onClientError(request, statusCode, msg).map(_.as("application/problem+json"))
parsers.errorHandler
.onClientError(
request.addAttr(HttpErrorHandler.Attrs.HttpErrorInfo, HttpErrorInfo("csp-filter")),
statusCode,
msg
)
.map(_.as("application/problem+json"))
}

private def createErrorResult(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ import akka.stream.stage._
import akka.util.ByteString
import play.api.MarkerContexts.SecurityMarkerContext
import play.api.http.HttpEntity
import play.api.http.HttpErrorHandler
import play.api.http.HttpErrorInfo
import play.api.http.HeaderNames._
import play.api.http.HttpErrorHandler.Attrs
import play.api.http.SessionConfiguration
import play.api.libs.crypto.CSRFTokenSigner
import play.api.libs.streams.Accumulator
Expand Down Expand Up @@ -547,7 +550,7 @@ class CSRFActionHelper(
def clearTokenIfInvalid(request: RequestHeader, errorHandler: ErrorHandler, msg: String): Future[Result] = {
import play.core.Execution.Implicits.trampoline

errorHandler.handle(request, msg).map { result =>
errorHandler.handle(request.addAttr(Attrs.HttpErrorInfo, HttpErrorInfo("csrf-filter")), msg).map { result =>
CSRF
.getToken(request)
.fold(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,9 @@ object CSRF {

class CSRFHttpErrorHandler @Inject() (httpErrorHandler: HttpErrorHandler) extends ErrorHandler {
import play.api.http.Status.FORBIDDEN

// The req param already has HttpErrorInfo("csrf-filter") in its attributes, no need to add them here.
// (It gets set when calling the CSRF ErrorHandler's handle(...) method)
def handle(req: RequestHeader, msg: String) = httpErrorHandler.onClientError(req, FORBIDDEN, msg)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import play.api.MarkerContexts.SecurityMarkerContext
import play.api.Configuration
import play.api.Logger
import play.api.http.HttpErrorHandler
import play.api.http.HttpErrorInfo
import play.api.http.Status
import play.api.inject._
import play.api.libs.streams.Accumulator
Expand Down Expand Up @@ -52,7 +53,13 @@ case class AllowedHostsFilter @Inject() (config: AllowedHostsConfig, errorHandle
next(req)
} else {
logger.warn(s"Host not allowed: ${req.host}")(SecurityMarkerContext)
Accumulator.done(errorHandler.onClientError(req, Status.BAD_REQUEST, s"Host not allowed: ${req.host}"))
Accumulator.done(
errorHandler.onClientError(
req.addAttr(HttpErrorHandler.Attrs.HttpErrorInfo, HttpErrorInfo("allowed-hosts-filter")),
Status.BAD_REQUEST,
s"Host not allowed: ${req.host}"
)
)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,32 +106,57 @@ trait CSRFCommonSpecs extends Specification with PlaySpecification {
_.withCookies("foo" -> "bar")
.addHttpHeaders(HeaderName -> "nocheck")
.post(Map("foo" -> "bar"))
)(_.status must_== errorStatusCode)
)(response => {
if (errorStatusCode == UNAUTHORIZED) {
response.body must startingWith("Origin: csrf-filter /")
}
response.status must_== errorStatusCode
})
}
"reject requests with ajax header" in {
csrfCheckRequest(
_.withCookies("foo" -> "bar")
.addHttpHeaders("X-Requested-With" -> "a spoon")
.post(Map("foo" -> "bar"))
)(_.status must_== errorStatusCode)
)(response => {
if (errorStatusCode == UNAUTHORIZED) {
response.body must startingWith("Origin: csrf-filter /")
}
response.status must_== errorStatusCode
})
}
"reject requests with different token in body" in {
csrfCheckRequest(req =>
addToken(req, generate)
.post(Map("foo" -> "bar", TokenName -> generate))
)(_.status must_== errorStatusCode)
)(response => {
if (errorStatusCode == UNAUTHORIZED) {
response.body must startingWith("Origin: csrf-filter /")
}
response.status must_== errorStatusCode
})
}
"reject requests with token in session but none elsewhere" in {
csrfCheckRequest(req =>
addToken(req, generate)
.post(Map("foo" -> "bar"))
)(_.status must_== errorStatusCode)
)(response => {
if (errorStatusCode == UNAUTHORIZED) {
response.body must startingWith("Origin: csrf-filter /")
}
response.status must_== errorStatusCode
})
}
"reject requests with token in body but not in session" in {
csrfCheckRequest(
_.withSession("foo" -> "bar")
.post(Map("foo" -> "bar", TokenName -> generate))
)(_.status must_== errorStatusCode)
)(response => {
if (errorStatusCode == UNAUTHORIZED) {
response.body must startingWith("Origin: csrf-filter /")
}
response.status must_== errorStatusCode
})
}

// add to response
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import akka.util.ByteString
import org.specs2.specification.core.Fragment
import play.api.ApplicationLoader.Context
import play.api.http.HttpEntity
import play.api.http.HttpErrorHandler
import play.api.http.HttpFilters
import play.api.inject.guice.GuiceApplicationBuilder
import play.api.inject.guice.GuiceApplicationLoader
Expand All @@ -26,6 +27,7 @@ import play.api.Environment
import play.api.Mode
import play.mvc.Http

import scala.compat.java8.OptionConverters._
import scala.concurrent.Future
import scala.util.Random

Expand Down Expand Up @@ -395,11 +397,28 @@ class CSRFFilterSpec extends CSRFCommonSpecs {

class CustomErrorHandler extends CSRF.ErrorHandler {
import play.api.mvc.Results.Unauthorized
def handle(req: RequestHeader, msg: String) = Future.successful(Unauthorized(msg))
def handle(req: RequestHeader, msg: String) =
Future.successful(
Unauthorized(
"Origin: " + req.attrs
.get(HttpErrorHandler.Attrs.HttpErrorInfo)
.map(_.origin)
.getOrElse("<not set>") + " / " + msg
)
)
}

class JavaErrorHandler extends CSRFErrorHandler {
def handle(req: Http.RequestHeader, msg: String) = CompletableFuture.completedFuture(play.mvc.Results.unauthorized())
def handle(req: Http.RequestHeader, msg: String) =
CompletableFuture.completedFuture(
play.mvc.Results.unauthorized(
"Origin: " + req.attrs
.getOptional(play.http.HttpErrorHandler.Attrs.HTTP_ERROR_INFO)
.asScala
.map(_.origin)
.getOrElse("<not set>") + " / " + msg
)
)
}

class CsrfFilters @Inject() (filter: CSRFFilter) extends HttpFilters {
Expand Down

0 comments on commit 510d4ec

Please sign in to comment.