Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom CSRF exception mappers are always processed by Krazo #34

Closed
chkal opened this issue Jan 18, 2019 · 6 comments
Closed

Custom CSRF exception mappers are always processed by Krazo #34

chkal opened this issue Jan 18, 2019 · 6 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@chkal
Copy link
Contributor

chkal commented Jan 18, 2019

MVC allows to define custom exception mappers for CsrfValidationExceptions:

@Provider
public class MyMapper implements ExceptionMapper<CsrfValidationException> {

    @Override
    public Response toResponse(CsrfValidationException exception) {
        return Response.status(403)
                .type(MediaType.TEXT_PLAIN)
                .entity("Invalid CSRF token")
                .build();
    }

}

However, using this mapper currently fails with:

javax.ws.rs.ServerErrorException: Unable to find suitable view engine for 'org.eclipse.krazo.engine.Viewable@5f01e840'
	at org.eclipse.krazo.core.ViewableWriter.writeTo(ViewableWriter.java:135)
	at org.eclipse.krazo.core.ViewableWriter.writeTo(ViewableWriter.java:77)
	at org.glassfish.jersey.message.internal.WriterInterceptorExecutor$TerminalWriterInterceptor.invokeWriteTo(WriterInterceptorExecutor.java:266)
	at org.glassfish.jersey.message.internal.WriterInterceptorExecutor$TerminalWriterInterceptor.aroundWriteTo(WriterInterceptorExecutor.java:251)
	at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:163)
	at org.glassfish.jersey.server.internal.JsonWithPaddingInterceptor.aroundWriteTo(JsonWithPaddingInterceptor.java:109)
	at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:163)
	at org.glassfish.jersey.server.internal.MappableExceptionWrapperInterceptor.aroundWriteTo(MappableExceptionWrapperInterceptor.java:85)
	at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:163)
	at org.glassfish.jersey.message.internal.MessageBodyFactory.writeTo(MessageBodyFactory.java:1135)
	at org.glassfish.jersey.server.ServerRuntime$Responder.writeResponse(ServerRuntime.java:662)
	at org.glassfish.jersey.server.ServerRuntime$Responder.processResponse(ServerRuntime.java:395)
	at org.glassfish.jersey.server.ServerRuntime$Responder.process(ServerRuntime.java:441)
	at org.glassfish.jersey.server.ServerRuntime$1.run(ServerRuntime.java:285)
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:272)
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:268)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:316)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:298)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:268)
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:289)
	at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:256)
	at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:703)
	at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:416)
	at org.glassfish.jersey.servlet.WebComponent.service(WebComponent.java:370)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:389)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:342)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:229)
	at org.apache.catalina.core.StandardWrapper.service(StandardWrapper.java:1540)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:217)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:119)
	at org.apache.catalina.core.StandardPipeline.doInvoke(StandardPipeline.java:611)
	at org.apache.catalina.core.StandardPipeline.invoke(StandardPipeline.java:550)
	at com.sun.enterprise.web.WebPipeline.invoke(WebPipeline.java:75)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:114)
	at org.apache.catalina.connector.CoyoteAdapter.doService(CoyoteAdapter.java:332)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:199)
	at com.sun.enterprise.v3.services.impl.ContainerMapper$HttpHandlerCallable.call(ContainerMapper.java:439)
	at com.sun.enterprise.v3.services.impl.ContainerMapper.service(ContainerMapper.java:144)
	at org.glassfish.grizzly.http.server.HttpHandler.runService(HttpHandler.java:206)
	at org.glassfish.grizzly.http.server.HttpHandler.doHandle(HttpHandler.java:180)
	at org.glassfish.grizzly.http.server.HttpServerFilter.handleRead(HttpServerFilter.java:242)
	at org.glassfish.grizzly.filterchain.ExecutorResolver$9.execute(ExecutorResolver.java:119)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeFilter(DefaultFilterChain.java:284)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeChainPart(DefaultFilterChain.java:201)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.execute(DefaultFilterChain.java:133)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.process(DefaultFilterChain.java:112)
	at org.glassfish.grizzly.ProcessorExecutor.execute(ProcessorExecutor.java:77)
	at org.glassfish.grizzly.nio.transport.TCPNIOTransport.fireIOEvent(TCPNIOTransport.java:539)
	at org.glassfish.grizzly.strategies.AbstractIOStrategy.fireIOEvent(AbstractIOStrategy.java:112)
	at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.run0(WorkerThreadIOStrategy.java:117)
	at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.access$100(WorkerThreadIOStrategy.java:56)
	at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy$WorkerThreadRunnable.run(WorkerThreadIOStrategy.java:137)
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:593)
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:573)
	at java.lang.Thread.run(Thread.java:748)

You can reproduce this issue by modifying the mapper by adding a response entity in this TCK test:

https://github.com/mvc-spec/mvc-tck/tree/master/tests/src/main/java/org/mvcspec/tck/tests/security/csrf/exception

@chkal chkal added the help wanted Extra attention is needed label Jan 18, 2019
@chkal chkal added this to the 1.0.0-m06 milestone Jan 19, 2019
@chkal chkal added the bug Something isn't working label Jan 19, 2019
@erdlet
Copy link
Member

erdlet commented Jul 26, 2019

Hey all,

I just debugged that issue and I'm not sure if this is really a bug or a lack of detail regarding custom exception mappers in the specification. As specified, we allow to provide custom exception mappers (covered by the TCK), but nowhere is written how the custom Response or an attached entity has to be processed (didn't find anything in the CSRF or ViewEngine section).

So at Krazo, we assume that everything in the Response#entity has to be a View (except it's a redirect-prefixed String) when we handle Responses in the ViewResponseFilter. If there is no matching ViewEngine, the request isn't further processed (as defined in the specification). This seems legit to me, as I normally want to show some UI to my user in any case when I implement a webapp with MVC (but maybe there is some use-case I'm not aware of, so please tell me if I missed something :) ).

To get both ways to work, I created a new Property for Krazo to enable either this strict (default) evaluation of the Response#entity or a less strict one which checks if we:

  • receive a view template (something with file suffix) or redirect -> process via Viewable
  • something else -> process as plain entity

PR for this will be created in a few minutes.

@chkal
Copy link
Contributor Author

chkal commented Jul 31, 2019

Hi,

thanks a lot for helping with this. I agree that the spec currently doesn't explicitly mention how entities produced by exception mappers are handled. At first, I was actually even surprised the Krazo processes these entities. In Krazo there is the ViewResponseFilter which is bound via the @Controller binding annotation and basically transforms the result of the controller method into a Viewable instance which is then processed by a MBW for rendering. The interesting thing here is that the CsrfValidationException is thrown even before the controller method is invoked. However, JAX-RS still considers the @Controller annotation of the matched controller method and therefore invokes ViewResponseFilter which eventually triggers the view rendering. AFAIK the JAX-RS spec doesn't explicitly mention this behavior and I would bet that JAX-RS implementations handle this case differently.

However, I also agree that in real world applications most users will most likely want that such exception mappers trigger the view processing pipeline of Krazo. Therefore, my initial example could be a weird corner case.

But I like the idea of making the Krazo's behavior more configurable.

@gtudan
Copy link
Contributor

gtudan commented Jul 31, 2019 via email

@chkal
Copy link
Contributor Author

chkal commented Aug 4, 2019

@gtudan Thanks for your comment. This CXF behavior sounds really weird. To be honest, I found a few cases where the CXF interpretation of the JAX-RS spec is a bit "special". Anyway, do we already have an issue for this CXF incompatibility? If not, let's create one.

@gtudan
Copy link
Contributor

gtudan commented Aug 5, 2019

We do. It's the long-standing issue over here #27

I started digging around in the code a bit, but kind-of forgot about it some time ago. I'll try to put together a minimal example and ask on the CXF mailing list about it.

@chkal
Copy link
Contributor Author

chkal commented Aug 6, 2019

@gtudan Ok, great! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants