CSRF-Validation does not work with @View annotation

[gtudan]

After lots of debugging for #27 I found that CsrfProtected does not work in Combination with @View.

The CsrfValdiateFilter throws an exception as expected. This will skip the usual controller handling and trigger the CsrfExceptionMapper. There, the exception will be mapped to a response with corresponding Status and Reason Phrase, but without an entity. Everything is fine until here.

But then there is a special handling in the ViewResponseFilter that checks for calls with an empty entity on controller methods with the @View annotation.

https://github.com/eclipse-ee4j/krazo/blob/master/core/src/main/java/org/eclipse/krazo/core/ViewResponseFilter.java#L133

It will kick in and render the page despite any previous Exceptions, even if the controller never executed.

Consider this controller:

    @POST
    @CsrfProtected
    @View("redirect:/csrf/ok")
    public void postForm(@FormParam("greeting") String greeting) {
        
    }

This issue is not restricted to CsrfExceptions - it can happen on any ExceptionsMappers that don't return an entity in combination with the @View-Annotation.

What can we do about it?
An easy fix for CSRF-Validation would be to make the ExceptionMapper return a default-view. Compliant implementation must support JSP, so we could default to this as engine. We would need to think about how to inject our built-in-views, but it could be done.

The other approach would be to somehow detect that an exception handler kicked in inside the ViewResponseFilter and disable the view annotation processing.

I would prefer to push the handling of the @View annotation further down towards the controller. Maybe we can decorate the controller, so that it reads the annotation and writes the view from there to the entity if (and only if) it returns without an exception? A CDI-Interceptor running before the AroundControllerInterceptor could do the trick.

Here's a failing unit-test demonstrating the issue:
gtudan@c7da794

[chkal]

Hi @gtudan,

thanks a lot for helping with this! Great catch!

I don't think that the easy fix (sending a default entity in the CSRF exception mapper) would be a good fix, because (as you mentioned), it can happen with other mappers to.

The other approach would be to somehow detect that an exception handler kicked in inside the ViewResponseFilter and disable the view annotation processing.

Maybe it would also work to detect that the controller was not executed? This should be easy, because we already we have an CDI interceptor which intercepts all controller executions. The interceptor just needs to set some request scoped boolean to true and then we could access this information later on.

I would prefer to push the handling of the @View annotation further down towards the controller. Maybe we can decorate the controller, so that it reads the annotation and writes the view from there to the entity if (and only if) it returns without an exception? A CDI-Interceptor running before the AroundControllerInterceptor could do the trick.

That may even be better than what I suggested above and would also clean up the whole request processing pipeline.

Other thoughts?

[chkal]

@gtudan I just started to review #87, and I was wondering how it relates to this issue. I just want to make sure that we find the optimal solution for both issues. Any thoughts?

[gtudan]

I tried implementing the view-annotation processing in a CDI-Interceptor.
But I think this is a dead end - the interceptor is being invoked, but the entity is still null in the Response Filter. I'm not sure why. Maybe JAX-RS somewhat gets that the controller method has return type void and doesn't care about the response. To bad, this really cleaned up the code... If you still want to take a look here is the branch: https://github.com/gtudan/krazo/blob/krazo-91/core/src/main/java/org/eclipse/krazo/cdi/ViewInterceptor.java

So back to the first option:
I don't think detecting that the controller did not run will be enough. If the controller throws an exception, that may still be handled by an exception handler - the ViewResponseFilter has to detect that as well. The current implementation will still render whatever has been annotated on the controller method.

[chkal]

I tried implementing the view-annotation processing in a CDI-Interceptor.
But I think this is a dead end - the interceptor is being invoked, but the entity is still null in the Response Filter. I'm not sure why. Maybe JAX-RS somewhat gets that the controller method has return type void and doesn't care about the response. To bad, this really cleaned up the code... If you still want to take a look here is the branch: https://github.com/gtudan/krazo/blob/krazo-91/core/src/main/java/org/eclipse/krazo/cdi/ViewInterceptor.java

Too bad. 😢

Unfortunately, after thinking about this more, the behavior you are seeing makes sense from a CDI perspective. Imagine bean 1 calls a method on bean 2 with a void return type. Now even if you apply a CDI interceptor to that method, you cannot change the fact that the method signature actually doesn't return any value. So this value is lost.

Basically JAX-RS implementations will lookup a CDI resource/controller from the BeanManager and then simply invoke the corresponding method (with void return type). But the fact that some CDI interceptor tries to change the return value doesn't have any effect, because JAX-RS still sees a void method.

So back to the first option:
I don't think detecting that the controller did not run will be enough. If the controller throws an exception, that may still be handled by an exception handler - the ViewResponseFilter has to detect that as well. The current implementation will still render whatever has been annotated on the controller method.

Hmmm. Not sure what the best option is to handle this case. Let me think about this a bit more. I guess I also have to dig a it deeper into the code.

[chkal]

Hmmm. Not sure what the best option is to handle this case. Let me think about this a bit more. I guess I also have to dig a it deeper into the code.

I added some thoughts here: #104

[chkal]

Should be fixed via #106