[Play 2.5.1] CSRF RuntimException after migration from Play 2.4.6

[Thibault Meyer]

Hi,

We have just upgraded our project from Play 2.4.6 to Play 2.5.1; But All forms doesnt work properly: If user enter bad value, we return the same page with the submitted form to print error message on the right input. But in this case, we get a RuntimeException: No CSRF token present!

public class ExampleController extends Controller {


 @AddCSRFToken
 public Result GET_ShowForm() {
   final Form<DemoForm> demoForm = Form.form(DemoForm);
   return ok(MyFormView.render(demoForm));
 }
 
 @RequireCSRFCheck
 public Result POST_ProcessForm() {
   final Form<DemoForm> demoForm = Form
            .form(DemoForm)
            .bindFromRequest();
   if (demoForm.hasErrors()) {
     return badRequest(MyFormView.render(demoForm));  // On Play 2.5.1 : BOOM / RuntimeException
   }
   //Do something with form data
   return redirect(routes.ExampleController.GET_ShowForm());
 }
}

java.util.concurrent.CompletionException: java.lang.RuntimeException: No CSRF token present!
at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:292)
at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:308)
at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:593)
at java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:577)
at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:474)
at java.util.concurrent.CompletableFuture.completeExceptionally(CompletableFuture.java:1977)
at scala.concurrent.java8.FuturesConvertersImpl$CF.apply(FutureConvertersImpl.scala:21)
at scala.concurrent.java8.FuturesConvertersImpl$CF.apply(FutureConvertersImpl.scala:18)
at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:32)
at scala.concurrent.BatchingExecutor$Batch$$anonfun$run$1.processBatch$1(BatchingExecutor.scala:63)
Caused by: java.lang.RuntimeException: No CSRF token present!
at scala.sys.package$.error(package.scala:27)
at views.html.helper.CSRF$$anonfun$2.apply(CSRF.scala:29)
at views.html.helper.CSRF$$anonfun$2.apply(CSRF.scala:29)
at scala.Option.getOrElse(Option.scala:121)
at views.html.helper.CSRF$.formField(CSRF.scala:29)

[Thibault Meyer]

Enclosed, a demo project to reproduce the issue

[Will Sargent]

This is coming from the helper:

https://github.com/playframework/playframework/blob/master/framework/src/play-filters-helpers/src/main/scala/views/html/helper/CSRF.scala#L18

which is calling out to CSRF.getToken:

https://github.com/playframework/playframework/blob/master/framework/src/play-filters-helpers/src/main/scala/play/filters/csrf/csrf.scala#L175

So it's not finding the request tag with the CSRF check.

I'd turn on tracing in logback.xml:

<logger name="play" value="TRACE"/> 

and check that the CSRF token is being set in that request before it gets to the helper...

--
You received this message because you are subscribed to the Google Groups "play-framework" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framewor...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/d8e84e29-9798-4ca2-a581-ffa2cb2b6383%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mariot Chauvin]

I think the helper code need some enhancement. If the CRSF token is missing the server should not through a runtime exception but returns a 403 like the default error handler in the CSRF filter: 

In general:

• CRSF.getToken should be avoided and the crsf instance should be used to get the token
   
•  getOrElse(sys.error(...)) is a bad practice. It prevents the compiler to check the safety of the code and does not allow one to recover properly.
  This will result in the server returning 500 on the else condition

To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/CAJmgB60dDtRGE9vJMV7NXkep6AQpifmjXwNmdFHt3d0jUX1%2BFg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

---

This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way.  Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software.

 

Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP.  Registered in England Number 908396

[Greg Methvin]

Actually, I think a 500 error is okay, though it might be better if we threw a more useful type of exception. CSRF.getToken gets the server-generated token that should be displayed to the user. If the token is somehow not being generated on the server, then something is not configured properly. It should get a new token regardless of the input from the user.

I'm actually not sure if we have any tests to test rendering a form in the action with the CSRF check, so we should probably test that. I think our tests generally assume you are using the post-redirect-get pattern and only rendering the form in the GET request and doing the check in the POST.

To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/CAGHe3P%3Dg8%3DU1r%3DKHjFbHbAdwoSkYujbkZ3iSm6ipycbzESm7NQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Greg Methvin

Senior Software Engineer

Lightbend, Inc.

[Igmar Palsenberg]

 

Actually, I think a 500 error is okay, though it might be better if we threw a more useful type of exception. CSRF.getToken gets the server-generated token that should be displayed to the user. If the token is somehow not being generated on the server, then something is not configured properly. It should get a new token regardless of the input from the user.

A 500 error is NOT OK. Not getting a piece of user input is a user / script error, unless we're certain that it is an internal issue. In this case, it is probably not.

500 errors open up a loophole that allows malicious users to take a whole serverfarm offline, since usually, loadbalancer will remove backend servers that give 5xx errors above a certain threshold. At least the AWS ELB does it this way, and this also applies to most nginx proxy configs out there.

Igmar

[Greg Methvin]

In this case it is your job as a developer to ensure that a token is present on the server. If you are rendering a form, you should use @CSRFAddToken to ensure a token is generated. The @RequireCSRFCheck just requires a check, so I actually think the OPs code is buggy. But I'm not sure why the behavior changed.

The use case we expected is a POST-redirect-GET, where the check is on the POST and the form render is on the GET. I guess if you wanted to render a form in the POST request you should probably just apply both annotations.

I'm not sure why there was a change in behavior from 2.4 though.

--

You received this message because you are subscribed to the Google Groups "play-framework" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framewor...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/b76c732a-92b5-42c6-97f0-839d308db0ba%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Igmar Palsenberg]

Op donderdag 7 april 2016 13:07:47 UTC+2 schreef Greg Methvin:

In this case it is your job as a developer to ensure that a token is present on the server. If you are rendering a form, you should use @CSRFAddToken to ensure a token is generated. The @RequireCSRFCheck just requires a check, so I actually think the OPs code is buggy. But I'm not sure why the behavior changed.

True that it might be a user error. But user errors don't warrant an ISE. ISE's should be throws in case the server encounters a fatal error, and is unable to recover. Throwing a 500 opens up a lot of loopholes.

Igmar

[Greg Methvin]

Based on RFC2616, HTTP 500 means "The server encountered an unexpected condition which prevented it from fulfilling the request." I don't think there's a better status code you could return. If the server is rendering a form, it's an "unexpected condition" to not have added that token to the session.

What we really should do is change how the token is obtained for the form. One solution is to use the CSRF.getToken method that returns an Option, and pass the token to the form if it exists. Also, we could make the Scala helper CSRFAddToken accept a Token => Action[A], so you could easily pass the token down to the template. I'm not sure what the best solution is for Java.

--

You received this message because you are subscribed to the Google Groups "play-framework" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framewor...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/d651a672-5ba0-44d4-9e17-caa15fcf4b73%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Igmar Palsenberg]

 

Based on RFC2616, HTTP 500 means "The server encountered an unexpected condition which prevented it from fulfilling the request." I don't think there's a better status code you could return. If the server is rendering a form, it's an "unexpected condition" to not have added that token to the session.

It's a bad request, and it's a client error : It should have send the CSRF token. It's not unexpected : Input can be wrong, or missing stuff. That is expected on the HTTP world.

So, in this case, go with a 400. A 500 "unexpected error" is that we screwed up on serverside, because something really bad happened.

Like said : When a client can trigger a 500 error, you open up a possible DOS. That is not something we should want.

Igmar

[Greg Methvin]

It's a bad request, and it's a client error : It should have send the CSRF token. It's not unexpected : Input can be wrong, or missing stuff. That is expected on the HTTP world.

So, in this case, go with a 400. A 500 "unexpected error" is that we screwed up on serverside, because something really bad happened.

 

No. The token it's getting in the form helper is the token that the server has generated. If you're using the AddCSRFToken helper, a new session token will be generated so it's available in the request, regardless of what the client has sent. If you're getting that exception, there's either a bug/misconfiguration in your code, or a bug in Play.

In this case it's also possible the CSRF check is not actually being performed. @RequireCSRFCheck only "requires" a check if the CSRF filter configuration tells it to do so. So this exception could happen on a bare request if no check is performed, and it's possible that is happening because of the CSRF configuration changes in 2.5.

[Igmar Palsenberg]

It's a bad request, and it's a client error : It should have send the CSRF token. It's not unexpected : Input can be wrong, or missing stuff. That is expected on the HTTP world.

So, in this case, go with a 400. A 500 "unexpected error" is that we screwed up on serverside, because something really bad happened.

 

No. The token it's getting in the form helper is the token that the server has generated. If you're using the AddCSRFToken helper, a new session token will be generated so it's available in the request, regardless of what the client has sent. If you're getting that exception, there's either a bug/misconfiguration in your code, or a bug in Play.

Clear. Yeah, in that case, the 5xx makes sense.

 

In this case it's also possible the CSRF check is not actually being performed. @RequireCSRFCheck only "requires" a check if the CSRF filter configuration tells it to do so. So this exception could happen on a bare request if no check is performed, and it's possible that is happening because of the CSRF configuration changes in 2.5.

I believe we're also using this. If yes, I'll check what it does with 2.5.

Igmar