Add HttpErrorInfo request attribute to track origins of errors by mkurz · Pull Request #10270 · playframework/playframework

mkurz · 2020-05-11T15:37:48Z

Please read this comment, it should explain the motivation behind this pull request: #6171 (comment)

I tried to keep the HttpErrorInfo class as generic as possible, so we can extend it later for other uses cases, maybe one day we want to attach more infos to the request passed to an error handler.

With this pull request I started to add the HttpErrorInfo attribute to onClientErrors thrown by filters and the server backends for now. We can extend that in later pull requests, if someone comes up with the need for more origin tracking.
For me personally I need to know if the 413 request entity too large came from the server backend or a body parser.

mkurz · 2020-05-11T15:43:14Z

+
+case class HttpErrorInfo(
+    origin: String
+) extends play.http.HttpErrorInfo


The implementation of HttpErrorInfo is done the same we way do for HandlerDef:

https://github.com/playframework/playframework/blob/2.8.1/core/play/src/main/java/play/routing/HandlerDef.java

https://github.com/playframework/playframework/blob/2.8.1/core/play/src/main/scala/play/api/routing/HandlerDef.scala

The Scala version extends the Java version, this makes it easy to just always store the one and only instance in the request attributes, because it is, at the same time, a Java and a Scala HttpErrorInfo object.

mkurz · 2020-05-11T15:45:34Z

+   */
+  object Attrs {
+    val HttpErrorInfo: TypedKey[HttpErrorInfo] = TypedKey("HttpErrorInfo")
+  }


Again same style like for HandlerDef:

playframework/core/play/src/main/scala/play/api/routing/Router.scala

Lines 118 to 127 in 7d860b7

/**

* Request attributes used by the router.

*/

object Attrs {

/**

* Key for the [[HandlerDef]] used to handle the request.

*/

val HandlerDef: TypedKey[HandlerDef] = TypedKey("HandlerDef")

}

mkurz · 2020-05-11T15:46:14Z

+  class Attrs {
+    public static final TypedKey<HttpErrorInfo> HTTP_ERROR_INFO =
+        new TypedKey<>(play.api.http.HttpErrorHandler.Attrs$.MODULE$.HttpErrorInfo());
+  }


HandlerDef style once more:

playframework/core/play/src/main/java/play/routing/Router.java

Lines 51 to 56 in 7d860b7

/** Request attributes used by the router. */

class Attrs {

/** Key for the {@link HandlerDef} used to handle the request. */

public static final TypedKey<HandlerDef> HANDLER_DEF =

new TypedKey<>(play.api.routing.Router.Attrs$.MODULE$.HandlerDef());

}

mkurz · 2020-05-12T15:34:00Z

@wsargent @gmethvin Can you have a look at this one? Actually this was the pull request I wanted to submit in the first place... I would like to backport to 2.8.x

wsargent

I think HttpErrorInfo as a request attribute is fine as is, but we should either label it as an internal/experimental API (Akka does this occasionally using ApiMayChange) or document it fully. Leaning it marking it as experimental, so if you rely on the string to be a specific value it's specifically a risk you're taking.

ignasi35 · 2020-05-19T16:27:13Z

I think HttpErrorInfo as a request attribute is fine as is, but we should either label it as an internal/experimental API (Akka does this occassionally) or document it fully. Leaning it marking it as experimental, so if you rely on the string to be a specific value it's specifically a risk you're taking.

I like the idea of marking this @ApiMayChange.

mkurz · 2020-05-19T22:20:57Z

Thanks for the feedback, I will fix that tomorrow.

wsargent

LGTM

wsargent · 2020-05-20T15:54:06Z

+ * Used as request attribute which gets attached to the request that gets passed to an error
+ * handler. Contains additional information useful for handling an error.
+ */
+@ApiMayChange


wsargent · 2020-05-20T15:54:50Z

+ * Used as request attribute which gets attached to the request that gets passed to an error
+ * handler. Contains additional information useful for handling an error.
+ */
+@ApiMayChange


wsargent · 2020-05-20T15:55:18Z

  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.


mkurz · 2020-05-20T15:57:08Z

@Mergifyio backport 2.8.x

mergify · 2020-05-20T15:57:40Z

Command backport 2.8.x: pending

Waiting for the pull request to get merged

mergify · 2020-05-20T16:01:18Z

Command backport 2.8.x: failure

No backport have been created

Backport to branch 2.8.x failed

Cherry-pick of 510d4ec has failed:

On branch mergify/bp/2.8.x/pr-10270
Your branch is up to date with 'origin/2.8.x'.

You are currently cherry-picking commit 510d4ec2e4.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   core/play/src/main/java/play/http/HttpErrorHandler.java
	new file:   core/play/src/main/java/play/http/HttpErrorInfo.java
	modified:   core/play/src/main/scala/play/api/http/HttpErrorHandler.scala
	new file:   core/play/src/main/scala/play/api/http/HttpErrorInfo.scala
	modified:   transport/server/play-akka-http-server/src/main/scala/play/core/server/AkkaHttpServer.scala
	modified:   transport/server/play-netty-server/src/main/scala/play/core/server/netty/PlayRequestHandler.scala
	modified:   web/play-filters-helpers/src/main/java/play/filters/csrf/RequireCSRFCheckAction.java
	modified:   web/play-filters-helpers/src/main/scala/play/filters/csp/CSPReportActionBuilder.scala
	modified:   web/play-filters-helpers/src/main/scala/play/filters/csrf/CSRFActions.scala
	modified:   web/play-filters-helpers/src/main/scala/play/filters/csrf/csrf.scala
	modified:   web/play-filters-helpers/src/main/scala/play/filters/hosts/AllowedHostsFilter.scala
	modified:   web/play-filters-helpers/src/test/scala/play/filters/csrf/CSRFCommonSpecs.scala
	modified:   web/play-filters-helpers/src/test/scala/play/filters/csrf/CSRFFilterSpec.scala
	modified:   web/play-filters-helpers/src/test/scala/play/filters/csrf/JavaCSRFActionSpec.scala
	modified:   web/play-filters-helpers/src/test/scala/play/filters/csrf/ScalaCSRFActionSpec.scala
	modified:   web/play-filters-helpers/src/test/scala/play/filters/hosts/AllowedHostsFilterSpec.scala

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   core/play-integration-test/src/it/scala/play/it/http/RequestBodyHandlingSpec.scala

mkurz added this to the Play 2.8.2 milestone May 11, 2020

mkurz requested review from gmethvin and wsargent May 11, 2020 15:38

mkurz commented May 11, 2020

View reviewed changes

mkurz force-pushed the HttpErrorInfo_reqattr_origin branch 2 times, most recently from ef792e9 to f8e9b5a Compare May 12, 2020 15:06

mkurz mentioned this pull request May 14, 2020

Improve error handling (HttpErrorHandler) #6171

Open

wsargent reviewed May 19, 2020

View reviewed changes

Comment thread core/play/src/main/scala/play/api/http/HttpErrorInfo.scala

Comment thread core/play/src/main/scala/play/api/http/HttpErrorInfo.scala

mkurz added 2 commits May 20, 2020 16:28

Add HttpErrorInfo request attribute to track origins of errors

510d4ec

Add ApiMayChange and some scala/javadoc

0442e4e

mkurz force-pushed the HttpErrorInfo_reqattr_origin branch from f8e9b5a to 0442e4e Compare May 20, 2020 15:08

mkurz removed the request for review from gmethvin May 20, 2020 15:42

ignasi35 requested a review from wsargent May 20, 2020 15:50

wsargent approved these changes May 20, 2020

View reviewed changes

mergify Bot merged commit 7bf54dd into playframework:master May 20, 2020

mkurz deleted the HttpErrorInfo_reqattr_origin branch May 20, 2020 16:26

mkurz mentioned this pull request May 20, 2020

[2.8.x] Add HttpErrorInfo request attribute to track origins of errors #10296

Merged

mkurz mentioned this pull request May 11, 2023

Overhaul 2.9 Migration and Highlight docs #11649

Closed

6 tasks

mkurz mentioned this pull request Jul 15, 2025

A call to onClientError from the server backend was missing the HttpErrorInfo attr #13416

Merged

	/**
	* Request attributes used by the router.
	*/
	object Attrs {

	/**
	* Key for the [[HandlerDef]] used to handle the request.
	*/
	val HandlerDef: TypedKey[HandlerDef] = TypedKey("HandlerDef")
	}

	/** Request attributes used by the router. */
	class Attrs {
	/** Key for the {@link HandlerDef} used to handle the request. */
	public static final TypedKey<HandlerDef> HANDLER_DEF =
	new TypedKey<>(play.api.routing.Router.Attrs$.MODULE$.HandlerDef());
	}

Uh oh!

Conversation

mkurz commented May 11, 2020

Uh oh!

mkurz May 11, 2020

Choose a reason for hiding this comment

Uh oh!

mkurz May 11, 2020

Choose a reason for hiding this comment

Uh oh!

mkurz May 11, 2020

Choose a reason for hiding this comment

Uh oh!

mkurz commented May 12, 2020

Uh oh!

wsargent left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ignasi35 commented May 19, 2020

Uh oh!

mkurz commented May 19, 2020

Uh oh!

wsargent left a comment

Choose a reason for hiding this comment

Uh oh!

wsargent May 20, 2020

Choose a reason for hiding this comment

Uh oh!

wsargent May 20, 2020

Choose a reason for hiding this comment

Uh oh!

wsargent May 20, 2020

Choose a reason for hiding this comment

Uh oh!

mkurz commented May 20, 2020

Uh oh!

mergify Bot commented May 20, 2020

Uh oh!

mergify Bot commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wsargent left a comment •

edited

Loading