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

Rewrite tries to forward even if response has already been committed #151

Closed
chkal opened this Issue Nov 26, 2013 · 7 comments

Comments

Projects
None yet
2 participants
@chkal
Member

chkal commented Nov 26, 2013

http://ocpsoft.org/support/topic/rewrite-and-picketlink-illegalstateexception/

It looks like PicketLink redirects the request before the RewriteFilter receives it. If a Join matches the request path, Rewrite will try to perform the forward the committed response which fails with something like this:

java.lang.IllegalStateException: Cannot forward after response has been committed
    at org.apache.catalina.core.ApplicationDispatcher.doForward(ApplicationDispatcher.java:498) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.core.ApplicationDispatcher.forward(ApplicationDispatcher.java:488) [jbossweb-7.0.13.Final.jar:]
    at org.ocpsoft.rewrite.servlet.impl.HttpRewriteResultHandler.handleResult(HttpRewriteResultHandler.java:39) [rewrite-servlet.jar:2.0.9.Final-SNAPSHOT]
    at org.ocpsoft.rewrite.servlet.RewriteFilter.rewrite(RewriteFilter.java:263) [rewrite-servlet.jar:2.0.9.Final-SNAPSHOT]
    at org.ocpsoft.rewrite.servlet.RewriteFilter.doFilter(RewriteFilter.java:188) [rewrite-servlet.jar:2.0.9.Final-SNAPSHOT]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:280) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:248) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:275) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:161) [jbossweb-7.0.13.Final.jar:]
    at org.jboss.as.web.security.SecurityContextAssociationValve.invoke(SecurityContextAssociationValve.java:153) [jboss-as-web-7.1.1.Final.jar:7.1.1.Final]
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:155) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:102) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:109) [jbossweb-7.0.13.Final.jar:]
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:368) [jbossweb-7.0.13.Final.jar:]
    at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:877) [jbossweb-7.0.13.Final.jar:]
    at org.apache.coyote.http11.Http11Protocol$Http11ConnectionHandler.process(Http11Protocol.java:671) [jbossweb-7.0.13.Final.jar:]
    at org.apache.tomcat.util.net.JIoEndpoint$Worker.run(JIoEndpoint.java:930) [jbossweb-7.0.13.Final.jar:]
    at java.lang.Thread.run(Thread.java:724) [rt.jar:1.7.0_40]

chkal added a commit that referenced this issue Nov 27, 2013

@chkal

This comment has been minimized.

Show comment
Hide comment
@chkal

chkal Nov 27, 2013

Member

Added a test to reproduce this. I'm using a Rewrite rule instead of a filter to commit the response before the Join is processed, but the result seems to be the same.

Member

chkal commented Nov 27, 2013

Added a test to reproduce this. I'm using a Rewrite rule instead of a filter to commit the response before the Join is processed, but the result seems to be the same.

@chkal

This comment has been minimized.

Show comment
Hide comment
@chkal

chkal Nov 27, 2013

Member

Or we could add this to the FAQ as a workaround?

.addRule()
.when(Direction.isInbound())
.perform(new HttpOperation() {
   @Override
   public void performHttp(HttpServletRewrite event, EvaluationContext context)
   {
      if(event.getResponse().isCommitted()) {
         event.setFlow(ServletRewriteFlow.ABORT_REQUEST);
      }
   }
})
Member

chkal commented Nov 27, 2013

Or we could add this to the FAQ as a workaround?

.addRule()
.when(Direction.isInbound())
.perform(new HttpOperation() {
   @Override
   public void performHttp(HttpServletRewrite event, EvaluationContext context)
   {
      if(event.getResponse().isCommitted()) {
         event.setFlow(ServletRewriteFlow.ABORT_REQUEST);
      }
   }
})
@lincolnthree

This comment has been minimized.

Show comment
Hide comment
@lincolnthree

lincolnthree Dec 3, 2013

Member

I suppose this should be a warning situation. Join should probably give up more peacefully. I'll play with this, but it was my original intent that this force users to be more careful about what they do with their mappings. Though I suppose the whole idea is that we should do most things for people.

Member

lincolnthree commented Dec 3, 2013

I suppose this should be a warning situation. Join should probably give up more peacefully. I'll play with this, but it was my original intent that this force users to be more careful about what they do with their mappings. Though I suppose the whole idea is that we should do most things for people.

@lincolnthree

This comment has been minimized.

Show comment
Hide comment
@lincolnthree

lincolnthree Dec 3, 2013

Member

Okay, I've thought about this a little more, and I think it makes sense for Join and other pre-constructed rules that do forwards/etc to fail the condition gracefully. It makes sense for custom built rules to require customization to handle this imo, but for pre-constructed rules, I think we can do this for the user.

Member

lincolnthree commented Dec 3, 2013

Okay, I've thought about this a little more, and I think it makes sense for Join and other pre-constructed rules that do forwards/etc to fail the condition gracefully. It makes sense for custom built rules to require customization to handle this imo, but for pre-constructed rules, I think we can do this for the user.

@lincolnthree

This comment has been minimized.

Show comment
Hide comment
@lincolnthree

lincolnthree Dec 3, 2013

Member

Hmmm, however this introduces an interesting situation...

Let's say that the following occurs:

  1. PicketLink filter performs redirect.
  2. PicketLink filter calls filterChain.doFilter(...)
  3. RewriteFilter is called.
  4. Join does not evaluate.
  5. RewriteFIlter calls filterChain.doFilter(...)

In my opinion, this is a bug in PicketLink, because you should not call filterChain.doFilter(...) after issuing a redirect because the response will be committed. This does not mean that you cannot do further internal processing on the request if this is intentional, which in turn means that I'm not sure it would be correct to detect response.isCommitted() and abort in RewriteFilter, either. So either way we are going to end up with a potential IllegalStateException scenario.

If a user wishes to prevent this scenario, they could add a rule like you exampled above. I've added a new Response condition to make this simpler:

               .addRule()
               .when(Response.isCommitted().and(Direction.isInbound()))
               .perform(Lifecycle.abort())

I've also added this messaging to make the situation clear:

13:31:48,374 WARNING [org.ocpsoft.rewrite.servlet.RewriteFilter] (http-localhost/127.0.0.1:8080-1) Response is already committed and further write operations are not permitted. This may result in an IllegalStateException being triggered by the underlying application. To avoid this situation, consider adding a Rule `.when(Direction.isInbound().and(Response.isCommitted())).perform(Lifecycle.abort())`.
13:31:48,374 ERROR [org.apache.catalina.core.ContainerBase.[jboss.web].[default-host].[/rewrite-test].[default]] (http-localhost/127.0.0.1:8080-1) Servlet.service() for servlet default threw exception: java.lang.IllegalStateException
    at org.apache.catalina.connector.ResponseFacade.sendError(ResponseFacade.java:409) [jbossweb-7.0.16.Final.jar:]

If we really want to be proactive about this, we could add a new pre-constructed rule to make configuration even simpler, but I'm not sure it's necessary:

.addRule(Lifecycle.safe())

Member

lincolnthree commented Dec 3, 2013

Hmmm, however this introduces an interesting situation...

Let's say that the following occurs:

  1. PicketLink filter performs redirect.
  2. PicketLink filter calls filterChain.doFilter(...)
  3. RewriteFilter is called.
  4. Join does not evaluate.
  5. RewriteFIlter calls filterChain.doFilter(...)

In my opinion, this is a bug in PicketLink, because you should not call filterChain.doFilter(...) after issuing a redirect because the response will be committed. This does not mean that you cannot do further internal processing on the request if this is intentional, which in turn means that I'm not sure it would be correct to detect response.isCommitted() and abort in RewriteFilter, either. So either way we are going to end up with a potential IllegalStateException scenario.

If a user wishes to prevent this scenario, they could add a rule like you exampled above. I've added a new Response condition to make this simpler:

               .addRule()
               .when(Response.isCommitted().and(Direction.isInbound()))
               .perform(Lifecycle.abort())

I've also added this messaging to make the situation clear:

13:31:48,374 WARNING [org.ocpsoft.rewrite.servlet.RewriteFilter] (http-localhost/127.0.0.1:8080-1) Response is already committed and further write operations are not permitted. This may result in an IllegalStateException being triggered by the underlying application. To avoid this situation, consider adding a Rule `.when(Direction.isInbound().and(Response.isCommitted())).perform(Lifecycle.abort())`.
13:31:48,374 ERROR [org.apache.catalina.core.ContainerBase.[jboss.web].[default-host].[/rewrite-test].[default]] (http-localhost/127.0.0.1:8080-1) Servlet.service() for servlet default threw exception: java.lang.IllegalStateException
    at org.apache.catalina.connector.ResponseFacade.sendError(ResponseFacade.java:409) [jbossweb-7.0.16.Final.jar:]

If we really want to be proactive about this, we could add a new pre-constructed rule to make configuration even simpler, but I'm not sure it's necessary:

.addRule(Lifecycle.safe())

@chkal

This comment has been minimized.

Show comment
Hide comment
@chkal

chkal Dec 3, 2013

Member

Hmmmm. Lincoln, I think you are right. Actually this situation can only occur if PicketLink calls doFilter after sending the redirect, which is not correct and therefore a bug in PicketLink.

Member

chkal commented Dec 3, 2013

Hmmmm. Lincoln, I think you are right. Actually this situation can only occur if PicketLink calls doFilter after sending the redirect, which is not correct and therefore a bug in PicketLink.

@lincolnthree

This comment has been minimized.

Show comment
Hide comment
@lincolnthree

lincolnthree Dec 3, 2013

Member

Okay. I'm going to close this as "not a bug" - perhaps we should report this to the picketlink folks?

Member

lincolnthree commented Dec 3, 2013

Okay. I'm going to close this as "not a bug" - perhaps we should report this to the picketlink folks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment