Add HttpErrorInfo request attribute to track origins of errors #10270

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

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

+
+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

core/play/src/main/scala/play/api/http/HttpErrorHandler.scala

+   */
+  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

core/play/src/main/java/play/http/HttpErrorHandler.java

+  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.

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

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

core/play/src/main/java/play/http/HttpErrorInfo.java

+ * 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

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

+ * 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

web/play-filters-helpers/src/main/scala/play/filters/csrf/csrf.scala

@@ -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.


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

core/play/src/main/scala/play/api/http/HttpErrorInfo.scala Show resolved Hide resolved

core/play/src/main/scala/play/api/http/HttpErrorInfo.scala Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HttpErrorInfo request attribute to track origins of errors #10270

Add HttpErrorInfo request attribute to track origins of errors #10270

mkurz commented May 11, 2020

mkurz May 11, 2020

mkurz May 11, 2020

mkurz May 11, 2020

mkurz commented May 12, 2020

wsargent left a comment •

edited

ignasi35 commented May 19, 2020

mkurz commented May 19, 2020

wsargent left a comment

wsargent May 20, 2020

wsargent May 20, 2020

wsargent May 20, 2020

mkurz commented May 20, 2020

mergify bot commented May 20, 2020

mergify bot commented May 20, 2020

	/**
	* 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());
	}

Add HttpErrorInfo request attribute to track origins of errors #10270

Add HttpErrorInfo request attribute to track origins of errors #10270

Conversation

mkurz commented May 11, 2020

mkurz May 11, 2020

Choose a reason for hiding this comment

mkurz May 11, 2020

Choose a reason for hiding this comment

mkurz May 11, 2020

Choose a reason for hiding this comment

mkurz commented May 12, 2020

wsargent left a comment • edited

Choose a reason for hiding this comment

ignasi35 commented May 19, 2020

mkurz commented May 19, 2020

wsargent left a comment

Choose a reason for hiding this comment

wsargent May 20, 2020

Choose a reason for hiding this comment

wsargent May 20, 2020

Choose a reason for hiding this comment

wsargent May 20, 2020

Choose a reason for hiding this comment

mkurz commented May 20, 2020

mergify bot commented May 20, 2020

mergify bot commented May 20, 2020

wsargent left a comment •

edited