8 Replies Latest reply on Feb 7, 2011 11:10 AM by larshuber

    Resteasy - destroy session after request - serious bug?

    larshuber

      Resteasy can be configured to destroy the websession right after the request (default behaviour). In few circumstances the session can't be destroyed anymore. Example is if using basic authentication you can access the previous authenticated session even if giving wrong credentials in request. This can end up in serious security issues.


      Current code - 2.2.1.CR3 - org.jboss.seam.resteasy.ResteasyResourceAdapter.java


      public void getResource(...) {
          ...
          dispatcher.invoke(in, theResponse);
      
          // Prevent anemic sessions clog up the server
          if (request.getSession().isNew()
              && application.isDestroySessionAfterRequest()
              && !Session.instance().isInvalid())
          {
              log.debug("Destroying HttpSession after REST request");
              Session.instance().invalidate();
          }
          
          ...
      }



      Take in account that destroying the session is not placed in finally block.


      Scenario:
      1. You miss giving the credentials on first service call. AuthenticationFilter throws an exception. From this point, the session.isNew() will always return false but the code in ResteasyResourceAdapter to destroy the session was never reached. Looking at the condition if destroying the session shows, the session will never be destroyed due to the NOT NEW session.


      2. Authentication succeeded or even no authentication required. The service throws an exception and the code to invalidate the session is skipped. From now on the old story: the session is not new anymore, never entering the responsible code block.


      Personal intepretation:
      If resteasy is configured to destroy the session, the session should be invalidated in any case, coming to following code and workaround:




      1. override the Adapter by your own implementation. Unfortunately you have to mention: @Install(precedence = Install.DEPLOYMENT) due the default implementation is already Install.APPLICATION and not Install.FRAMEWORK

      2. override getResource(...)

      3. put responsible code in try finally block as following




         public void getResource(final HttpServletRequest request, final HttpServletResponse response) throws ServletException, IOException {
      
            try {
               log.debug("processing REST request");
      
               // TODO: As far as I can tell from tracing RE code: All this thread-local stuff has no effect because
               // the "default" provider factory is always used. But we do it anyway, just to mimic the servlet handler
               // in RE...
      
               // Wrap in RESTEasy thread-local factory handling
               ThreadLocalResteasyProviderFactory.push(dispatcher.getProviderFactory());
      
               // Wrap in RESTEasy contexts (this also puts stuff in a thread-local)
               SeamResteasyProviderFactory.pushContext(HttpServletRequest.class, request);
               SeamResteasyProviderFactory.pushContext(HttpServletResponse.class, response);
               SeamResteasyProviderFactory.pushContext(SecurityContext.class, new ServletSecurityContext(request));
      
               // Wrap in Seam contexts
               new ContextualHttpServletRequest(request) {
                  @Override
                  public void process() throws ServletException, IOException {
                     try {
                        HttpHeaders headers = ServletUtil.extractHttpHeaders(request);
                        UriInfoImpl uriInfo = extractUriInfo(request, application.getResourcePathPrefix());
      
                        HttpResponse theResponse = new HttpServletResponseWrapper(response, dispatcher.getProviderFactory());
      
                        // TODO: This requires a SynchronousDispatcher
                        HttpRequest in = new HttpServletInputMessage(request, theResponse, headers, uriInfo, request.getMethod().toUpperCase(),
                              (SynchronousDispatcher) dispatcher);
      
                       dispatcher.invoke(in, theResponse);
                     } finally {
                        // Just take in account if configured to destroy on each request
                        if (application.isDestroySessionAfterRequest()) {
                           log.debug("Destroying HttpSession after REST request");
                           Session.instance().invalidate();
                        }
                     }
                  }
               }.run();
      
            } finally {
               // Clean up the thread-locals
               SeamResteasyProviderFactory.clearContextData();
               ThreadLocalResteasyProviderFactory.pop();
               log.debug("completed processing of REST request");
            }
         }
      



      Is it a bug or what is the reason for this kind of implementation.


      greets
      Lars

        • 1. Re: Resteasy - destroy session after request - serious bug?
          jharting

          Thanks for reporting. I'll look into this. https://issues.jboss.org/browse/JBSEAM-4770

          • 2. Re: Resteasy - destroy session after request - serious bug?
            skunk

            I think this might have broken my application!


            2.2.1 CR2 was fine. In 2.2.1 Final if you browse to a page which makes a request to resteasy after loading (in my case for a chart) the session is destroyed. If the user submits any forms on that page they will then get an exception because the view has expired. If they browse anywhere else they will find they have also been logged out.


            Adding:


            <resteasy:application destroy-session-after-request="false"/>
            



            isn't an option because then all rest requests will cause sessions to be created.


            Is there a problem with the code that detects whether there is an existing session?

            • 3. Re: Resteasy - destroy session after request - serious bug?
              jharting

              David,


              looks like I accidentally created a regressions when fixing the previous issue. I've opened JBSEAM-4775 to address the problem.

              • 4. Re: Resteasy - destroy session after request - serious bug?
                larshuber

                David, you are right. Using resteasy with authenticated user will not work. In the ResourceAdapter it is essential to test on session.isNew(). Inside the try finally block it will at least work for exceptions thrown by the resteasy service itself. But there is one problem left: session.isNew() will not work if resteasy service is authentication restricted and there are no or wrong credentials on first call. This means the ResteasyResourceAdapter will never be invoked to be able to destroy the session. So there must be another way to know if session must be destroyed directly on first failing call. This might be a flag in session scope who the session created, probabely if resteasy with destroy-session was the creator.
                There should be a third option for destroy-session-after-request : always, never, ifCreated .

                • 5. Re: Resteasy - destroy session after request - serious bug?
                  larshuber

                  David, are you sure the option


                  <resteasy:application destroy-session-after-request="false"/>
                  



                  does not work for you? Requests shouldn't always create a new session. Have you tried?


                  @Jozef: I'm sorry for not having provided a solution for all use cases.

                  • 6. Re: Resteasy - destroy session after request - serious bug?
                    skunk

                    Hi Lars,


                    I think I misread the documentation then. I have added the snippet above to my components.xml and the user is no longer logged out when they view pages which make rest requests.


                    It's obviously very important that not every request to my rest services results in a long lasting session (10 minutes in my app) being created. Is there an easy way to test whether this is happening?


                    Thanks,

                    David

                    • 7. Re: Resteasy - destroy session after request - serious bug?
                      skunk

                      I should also add that none of the rest services are authenticated in any way and the application as a whole has forms based login (not HTTP basic). Not sure if this is important to the code above.


                      David

                      • 8. Re: Resteasy - destroy session after request - serious bug?
                        larshuber

                        Hi David


                        You can use 2.2.1.FINAL . Just override the ResteasyResourceAdapter with old implementation.


                        @Name("org.jboss.seam.resteasy.resourceAdapter")
                        @BypassInterceptors
                        @Install(precedence = Install.DEPLOYMENT) // essential because seam's framework impl does not declare precedence=BUILT_IN
                        public class FixedResteasyResourceAdapter extends org.jboss.seam.resteasy.ResteasyResourceAdapter {
                        
                           @Logger
                           Log log;
                        
                           @Override
                           public void getResource(final HttpServletRequest request, final HttpServletResponse response)
                                 throws ServletException, IOException
                           {
                        
                              try
                              {
                                 log.debug("processing REST request");
                        
                                 // TODO: As far as I can tell from tracing RE code: All this thread-local stuff has no effect because
                                 // the "default" provider factory is always used. But we do it anyway, just to mimic the servlet handler
                                 // in RE...
                        
                                 // Wrap in RESTEasy thread-local factory handling
                                 ThreadLocalResteasyProviderFactory.push(dispatcher.getProviderFactory());
                        
                                 // Wrap in RESTEasy contexts (this also puts stuff in a thread-local)
                                 SeamResteasyProviderFactory.pushContext(HttpServletRequest.class, request);
                                 SeamResteasyProviderFactory.pushContext(HttpServletResponse.class, response);
                                 SeamResteasyProviderFactory.pushContext(SecurityContext.class, new ServletSecurityContext(request));
                        
                                 // Wrap in Seam contexts
                                 new ContextualHttpServletRequest(request)
                                 {
                                    @Override
                                    public void process() throws ServletException, IOException
                                    {
                        
                                       HttpHeaders headers = ServletUtil.extractHttpHeaders(request);
                                       UriInfoImpl uriInfo = extractUriInfo(request, application.getResourcePathPrefix());
                        
                                       HttpResponse theResponse = new HttpServletResponseWrapper(
                                             response,
                                             dispatcher.getProviderFactory()
                                       );
                        
                                       // TODO: This requires a SynchronousDispatcher
                                       HttpRequest in = new HttpServletInputMessage(
                                             request,
                                             theResponse,
                                             headers,
                                             uriInfo,
                                             request.getMethod().toUpperCase(),
                                             (SynchronousDispatcher) dispatcher
                                       );
                        
                                       dispatcher.invoke(in, theResponse);
                        
                                       // Prevent anemic sessions clog up the server
                                       if (request.getSession().isNew()
                                             && application.isDestroySessionAfterRequest()
                                             && !Session.instance().isInvalid())
                                       {
                                          log.debug("Destroying HttpSession after REST request");
                                          Session.instance().invalidate();
                                       }
                                    }
                                 }.run();
                        
                              }
                              finally
                              {
                                 // Clean up the thread-locals
                                 SeamResteasyProviderFactory.clearContextData();
                                 ThreadLocalResteasyProviderFactory.pop();
                                 log.debug("completed processing of REST request");
                              }
                           }
                        }