Bug 28709 - javax.servlet.http.HttpServletRequest.isRequestedSessionIdValid() returns true for an invalidated session!
javax.servlet.http.HttpServletRequest.isRequestedSessionIdValid() returns tru...
Status: CLOSED FIXED
Product: Tomcat 5
Classification: Unclassified
Component: Catalina
5.0.23
Other Linux
: P3 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2004-04-30 10:49 UTC by blumm
Modified: 2006-06-11 18:03 UTC (History)
0 users



Attachments
Test webapplications for the reproduction of the problem described in the bug report. (15.48 KB, application/octet-stream)
2004-05-17 11:19 UTC, blumm
Details

Note You need to log in before you can comment on or make changes to this bug.
Description blumm 2004-04-30 10:49:59 UTC
This is the of the code sequence executed:

  javax.servlet.http.HttpServletRequestWrapper.getSession(true);
  // here, the debugger shows that the session is valid
  javax.servlet.http.HttpSession.invalidate();
  // here the debugger shows that the session has been invalidated
  javax.servlet.http.HttpServletRequestWrapper.getRequest();
  javax.servlet.http.HttpServletRequest.isRequestedSessionIdValid();
  // this call returns true, which is not expected!
Comment 1 Remy Maucherat 2004-04-30 13:38:43 UTC
Cool, let's start.
I disagree with that, please try the latest release.
Comment 2 blumm 2004-05-17 11:19:37 UTC
Created attachment 11576 [details]
Test webapplications for the reproduction of the problem described in the bug report.
Comment 3 blumm 2004-05-17 11:40:22 UTC
I still think there is a bug in Tomcat 5.x. BTW I wrote two test
web-applications, which will reproduce the problem for you. I have attached code   
for you. Please take a look at the readme for some instructions about
installation and execution.

Here some comments about the generell idea of the test:

Servlet 1 includes Servlet2 in a cross context environment. It gets a dispatcher
by "coServletContext.getRequestDispatcher(coServletPath)".
Servlet2 gets a new session on the first request and invalidates this session by
"httpSession.invalidate()" and tests the invalidation by
"httpServletRequestWrapper.isRequestedSessionIdValid()" on the second request.
"isRequestedSessionIdValid()" returns "true" instead of "false", which is a bug
 from my point of view.

What do you think? Thank you in advance for your help.
Comment 4 Yoav Shapira 2004-05-26 17:10:55 UTC
The Servlet Specification states (SRV.7.3) that the session is withint the 
scope of the servlet context and cannot be shared.  Accordingly I don't think 
the semantics of invalidation and the isRequestedSessionIdValid call are clear 
when doing cross-context includes.

Looking at the code, it should be fine.  From CoyoteRequest:
/**
     * Return <code>true</code> if the session identifier included in this
     * request identifies a valid session.
     */
    public boolean isRequestedSessionIdValid() {

        if (requestedSessionId == null)
            return (false);
        if (context == null)
            return (false);
        Manager manager = context.getManager();
        if (manager == null)
            return (false);
        Session session = null;
        try {
            session = manager.findSession(requestedSessionId);
        } catch (IOException e) {
            session = null;
        }
        if ((session != null) && session.isValid())
            return (true);
        else
            return (false);
    }

StandardSession#invalidate calls StandardSession#expire:
public void expire(boolean notify) {

        // Mark this session as "being expired" if needed
        if (expiring)
            return;

        synchronized (this) {

            if (manager == null)
                return;

            expiring = true;
        
            // Notify interested application event listeners
            // FIXME - Assumes we call listeners in reverse order
            Context context = (Context) manager.getContainer();
            Object listeners[] = context.getApplicationLifecycleListeners();
            if (notify && (listeners != null)) {
                HttpSessionEvent event =
                    new HttpSessionEvent(getSession());
                for (int i = 0; i < listeners.length; i++) {
                    int j = (listeners.length - 1) - i;
                    if (!(listeners[j] instanceof HttpSessionListener))
                        continue;
                    HttpSessionListener listener =
                        (HttpSessionListener) listeners[j];
                    try {
                        fireContainerEvent(context,
                                           "beforeSessionDestroyed",
                                           listener);
                        listener.sessionDestroyed(event);
                        fireContainerEvent(context,
                                           "afterSessionDestroyed",
                                           listener);
                    } catch (Throwable t) {
                        try {
                            fireContainerEvent(context,
                                               "afterSessionDestroyed",
                                               listener);
                        } catch (Exception e) {
                            ;
                        }
                        log(sm.getString("standardSession.sessionEvent"), t);
                    }
                }
            }
            accessCount = 0;
            setValid(false);

            // Remove this session from our manager's active sessions
            if (manager != null)
                manager.remove(this);

            // Notify interested session event listeners
            if (notify) {
                fireSessionEvent(Session.SESSION_DESTROYED_EVENT, null);
            }

            // We have completed expire of this session
            expiring = false;

            // Unbind any objects associated with this session
            String keys[] = keys();
            for (int i = 0; i < keys.length; i++)
                removeAttributeInternal(keys[i], notify);

        }

    }

Because of the Manager#remove call, the findSession call in CoyoteRequest will 
return null, and the isRequestedSessionIdValid method will return false.
Comment 5 blumm 2004-06-07 15:41:18 UTC
The excerpts from the code base look fine for me too. What makes me wonder is
that if the test-web-application uses
"coServletContext.getNamedDispatcher(coServlet)"
instead of
"coServletContext.getRequestDispatcher(coServletPath)"
then everything works as expected. I think there must be a consistent behavior.
Did you run the test code that I have provided with the attachments? Is the bug
in test code? 




 
Comment 6 Remy Maucherat 2004-06-07 17:02:39 UTC
The request wrapper used by the regular RD doesn't override this method (while
the named RD won't wrap).

No matter how I look at this, the answer to this issue remains the same: sorry,
but we don't care about it.
Comment 7 Remy Maucherat 2004-06-07 17:31:49 UTC
Ok, actually, my first sentence is a problem: not wrapping for named forwards
would mean trouble with cross context and session handling. That's a separate
issue, but will explain the different session behavior in that case (you're
going to interact with the session from the first context, which is a problem).
This is the only case, because named includes will wrap.
Comment 8 blumm 2004-06-08 16:19:53 UTC
SRV.7.3(from Version 2.4) says:
...
To illustrate this requirement with an example: if a servlet uses the
RequestDispatcher to call a servlet in another Web application, any sessions
created for and visible to the servlet being called must be different from those
visible to the calling servlet.

By the way the spec. does not distinguish at this point between a named request
dispatcher and a regular one.

What do you exactly mean by saying "you're going to interact with the session
from the first context, which is a problem" ? Does not the Spec. cover the
request wrapper in an consistent manner with SRV.7.3 ?
Comment 9 Remy Maucherat 2004-07-19 12:32:29 UTC
The issue affecting named dispatcher has been fixed (no wrapping was done
previously, so cross context sessions couldn't be handled properly). For the
rest, we won't fix it for now.
Comment 10 Jan Luehe 2004-12-08 20:02:16 UTC
Fixed by implementing
org.apache.catalina.core.ApplicationHttpRequest.isRequestedSessionIdValid() to
check the request's session id (if any) against the session manager of the
context associated with the ApplicationHttpRequest (wrapper), which may be
different from the context of the nested request (it will be different if the
request was dispatched into a foreign context).
Comment 11 Jan Luehe 2004-12-08 20:02:39 UTC
Fixed by implementing
org.apache.catalina.core.ApplicationHttpRequest.isRequestedSessionIdValid() to
check the request's session id (if any) against the session manager of the
context associated with the ApplicationHttpRequest (wrapper), which may be
different from the context of the nested request (it will be different if the
request was dispatched into a foreign context).
Comment 12 Peter Lynch 2005-01-26 20:17:40 UTC
So this is stated as fixed...

What file revision and tomcat release is the fix in?
Comment 13 Jan Luehe 2005-01-28 20:47:09 UTC
Fix went into revision 1.22 of
jakarta-tomcat-catalina/catalina/src/share/org/apache/catalina/core/ApplicationHttpRequest.java,
which was tagged with TOMCAT_5_5_6.