Filter is called but not applied when endpoint throws an exception

[JFCote]

Play Version

2.8.2, also tried in 2.7.X, same problem

API

Java

Operating System

Windows 10

JDK

java version "1.8.0_191" Java(TM) SE Runtime Environment (build 1.8.0_191-b12) Java HotSpot(TM) 64-Bit Server VM (build 25.191-b12, mixed mode)

Expected Behavior

1. If I have a filter that adds a specific header for each response.
2. I call GET http://localhost:9000/message-exception.
3. In the process of the call, the system throws an exception.
4. I receive a response which contains the exception details + the header from the filter.

Actual Behavior

1. If I have a filter that adds a specific header for each response.
2. I call GET http://localhost:9000/message-exception.
3. In the process of the call, the system throws an exception.
4. I receive a response which contains the exception details but NOT the specific header.

Reproducible Test Case

I have a project to reproduce the bug here: https://github.com/JFCote/play-framework-filter-problem

API call that works: http://localhost:9000/message API call that throw an exception: http://localhost:9000/message-exception

Check for the header X-ExampleFilter. It should be in both call but it only is in the first one.

[JFCote]

Is there anyone working on this project anymore? @gmethvin Thanks!

[chipz]

Is this something that can be handled with recover inside the filter though?

    @Override
    public EssentialAction apply(EssentialAction next) {
        System.out.println("the filter was called!!");
        return EssentialAction.of(request ->
            next.apply(request).map(result ->
                 result.withHeader("X-ExampleFilter", "foo"), exec)
                    .recover(x -> Results.ok().withHeader("X-ExampleFilter", "FOOO"), exec)
        );
    }

Or in Scala:

  override def apply(next: EssentialAction) = EssentialAction { request =>
    next(request).map { result =>
      result.withHeaders("X-ExampleFilter" -> "foo")
    }.recover {
      case error =>
        Results.Ok.withHeaders("X-ExampleFilter" -> "FOOO")
    }
  }

I've tried both and it's working in Scala and Java, and I can see the second header being returned here ("FOOO") Or is there some any bug that I'm not seeing here? Or do we need different implementation?

[JFCote]

@chipz I will definitely test this out today! Didn't know about the recover method. Will let you know here. Thanks for taking the time to answer!

[JFCote]

@chipz I tested your proposition and it worked but I still think it's a bug in the Play Framework. To be able to add security headers even when an API call throw an exception, I ended up creating this:

This is a pretty ugly hack if you ask me.

package filters;

import com.google.inject.Inject;
import com.google.inject.Singleton;
import openapitools.ErrorHandler;
import play.mvc.EssentialAction;
import play.mvc.EssentialFilter;
import play.mvc.Results;
import services.helper.interfaces.IUtilService;

import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;

@Singleton
public class ErrorHandlerFilter extends EssentialFilter {
    private final Executor exec;
    private final ErrorHandler errorHandler;

    @Inject
    public ErrorHandlerFilter(Executor exec, ErrorHandler errorHandler) {
        this.exec = exec;
        this.errorHandler = errorHandler;
    }

    @SuppressWarnings("java:S2142")
    @Override
    public EssentialAction apply(EssentialAction next) {
        return EssentialAction.of(request ->
                next.apply(request)
                        .recover(throwable -> {
                            try {
                                return errorHandler
                                        .onServerError(request, throwable)
                                        .toCompletableFuture().get();
                            } catch (InterruptedException | ExecutionException e) {
                                return Results.internalServerError();
                            }
                        }, exec)
        );
    }
}

So if for example you added this to your configuration file:

play.filters.headers {
    # The X-Frame-Options header. If null, the header is not set.
    frameOptions = "DENY"
}

With this new filter, even if you throw new RuntimeException() in your call, you will get the frame options header. I think it should be done by the framework by default.

[praphull-purohit]

@JFCote, since you're using custom error handler in your example, you could very well add the additional header within the error handler. This gives you more fine grained control over what headers you want to set during server/client errors.

return CompletableFuture.completedFuture(internalServerError("Internal server error!")
  .withHeader("X-My-Header", "something"));

This doesn't add any headers from other enabled filters though, since they are applied within map function of different filters (e.g. in apply method of SecurityHeadersFilter).

Considering the fact that filters serve different purposes (not just adding headers), forcing the application of filter logic on a failed execution might not be the most accurate thing to do. You could argue that Security Headers filter might need to be applied at all times (if enabled), but that's a decision best left for the implementation to decide. As mentioned in the snippet above, you can add any number of headers within a server error handler if you so desire and it would give you absolute control over what you want to do when an exception occurs.

My suggestion would be that this issue can be closed as the headers (both from SecurityHeadersFilter and custom filters) get added properly during successful executions. For error scenarios, Error Handler is a better place to customize the response IMHO.

P.S. This was the first issue I tried to work on in this repo since it was marked good first issue. I was trying to implement the change suggested in this issue but that didn't seem correct as I dived into the code

[JFCote]

@praphull First thing, thanks for taking the time to check this issue.

Unfortunately, I must disagree with you and argue, as you planned, that Security Headers that are in the application.conf should be applied at all time. If you think about it as a user of the Play Framework who chose to add security header when starting the deployment of the app (or just let the default). This user don't want to have the very bad surprise to learn, 6 months later when the app is in prod that when it returns a 500, the security headers are not there. And then, have to go through hard to find documentation pages/ github issues to find a weird work around of creating a "custom security header filters" or to hardcode the headers in the default ErrorHandler.

If this is the call Play Framework wants to make, then it should at least be clear in the default application.conf that is generated in a comment or something that this filter is NOT applied when the server returns an error and it should point to a clear documentation page that show what to do and explains why they choose this.

As a senior dev who worked with a lot of framework and who believes that security is very important, I think that letting the decision to add security or not on errors to the "implementer" is a bad idea. Security should always be activated by default in a framework and the implementer/user should be the one responsible to "remove" some part of it and make the call to have a less secure app.

I know you are only trying to help and I appreciate this but I would recommend asking maybe an old Play Framework developer before closing the issue as "fixed".

[FXHibon]

~Hello @JFCote~

~I've been looking at your filter problem~

~There is something interesting in the error handling on the /message-exception endpoint :~

~https://github.com/JFCote/play-framework-filter-problem/blob/master/app/controllers/AsyncController.java#L56~

~The endpoint is describes as : public CompletionStage<Result> messageException() : meaning that success must be wrapped in a CompletionStage, but this contract is true for failure as well. Therefore, any errors that occur in this endpoint must be wrapped in an CompletionStage, rather than throwing a raw exception => any error that would be throwed here, without being wrapped in a CompletionStage, would short-circuit the filters chain, resulting in the unexpected behavior you are facing here.~

~I took the liberty to open a Pull Request on your example github repository to demonstrate how errors should be handled in a CompletionStage context, without violating the contract of the function (aka it must returns a CompletionStage) The pull request can be found here => https://github.com/JFCote/play-framework-filter-problem/pull/1~

~This result in a consistent behavior, your filter is properly applied, as well as the custom ErrorHandler, see the following screenshot for illustration.~

~For what I can see, I do not think there is a bug on the Play framework here, as it behaves correctly as long as you respect the signature of your endpoint function (it must return a CompletionStage, possibly wrapping an error)~

EDIT:

it seems I read a bit to quick, or my eyes where not aligned correctly, when testing the behavior, and now I see that the header is missing, just as you described, because the filter is not applied. You can ignore all my previous comment, sorry about that 👼

[JFCote]

@FXHibon Thanks for taking the time even if it's not a solution after all. Also, I must say that the example I did was with the async API of Play Framework with CompletionStage and all but the real code in production where I discovered this bug is using the regular API and no async so I'm not breaking the contract and still have the bug.

Anyway, thanks for trying! :)

[gmethvin]

This seems like a deliberate "feature" of the design of filters, since most filters don't want to try to handle the result after an uncaught exception occurs, and in general it's hard to know whether it's even safe for the filter to do its work at that point. Basically I agree with what @praphull said (https://github.com/playframework/playframework/issues/10512#issuecomment-1004899011):

Considering the fact that filters serve different purposes (not just adding headers), forcing the application of filter logic on a failed execution might not be the most accurate thing to do. You could argue that Security Headers filter might need to be applied at all times (if enabled), but that's a decision best left for the implementation to decide. As mentioned in the snippet above, you can add any number of headers within a server error handler if you so desire and it would give you absolute control over what you want to do when an exception occurs.

The security headers situation is a possible exception to that rule, though. Indeed, if the framework user chose to output some exploitable content on their 500 error page, then the lack of those security headers could leave their application vulnerable in a way it wouldn't have been otherwise. (I should add, though, that best practice is to keep your production 500 error pages as simple as possible.)

So, what's the best way to resolve this? The way Play is currently designed, the most foolproof way for you to handle this yourself is to add the headers in HttpErrorHandler#onServerError, but that's obviously not a very satisfying answer. It would be nice if Play offered some way to do this without having to write custom code.

If we wanted to fix this for SecurityHeadersFilter specifically, one possibility is to change the implementation to something like:

def apply(next: EssentialAction) = EssentialAction { req =>
  next(req)
    .recoverWith(errorHandler.onServerError(req, _))
    .map(result => result.withHeaders(headers(req, result): _*))
}

That would handle the example where the action throws an exception, but it would only catch exceptions from filters that are applied after SecurityHeadersFilter in the list. So Play users would have to make sure to put the SecurityHeadersFilter first. The ordering of filters is something users already have to be aware of, so that's not too unreasonable I think.

Another possibility would be for Play to provide some sort of ErrorHandlerFilter like the above example provided by @JFCote, so Play users could easily decide which filters happened before and after error handling.

We're open to ideas and contributions, of course. To begin with we might want to implement something for SecurityHeadersFilter specifically, then try to come up with a more general purpose solution.