In mod_python, the session ID consists of 32 characters coming from the ranges
0-9 and a-f. At the moment the code will if it detects invalid characters in
the SID or it is the wrong size, raise a HTTP_INTERNAL_SERVER_ERROR exception.

        if self._sid:
            # Validate the sid *before* locking the session
            # _check_sid will raise an apache.SERVER_RETURN exception 
            if not _check_sid(self._sid):
                self._req.log_error("mod_python.Session warning: The session id 
contains invalid characters, valid characters are only 0-9 and a-f",
                raise apache.SERVER_RETURN, apache.HTTP_INTERNAL_SERVER_ERROR

This occurs regardless of whether the SID was supplied explicitly by user code,
or it was sent by the browser.

A problem with raising an Apache error like this is that if a invalid SID has 
to get into the browsers cache, a web application isn't able to automatically 
discard the existing value and replace it with a brand new SID. Instead, it is 
onto the user to go into the cookie cache of their browser and expunge the SID
which is causing the problem. Only after the user taking this action will they 
able to use your web application.

To me this seems to be the wrong way of going about it. It would make much more
sense to me if the web application, ie., mod_python session code, just treated 
it as
if the browser hadn't sent any session ID in the first place and create a new 
with the session ID in the browser being replaced.

That said, the other way the SID can be supplied is if the application code 
it explicitly to the session object when it is created. In that case any 
mistake is the
fault of the application code and I believe it should still raise an exception, 
it should be a general ValueError exception to make it easier for the 
to catch it, rather than a HTTP exception.

Thus propose the above code be replaced with:

        if self._sid:
            if not _check_sid(self._sid):
                if sid:
                    # Supplied explicitly by user of the class,
                    # raise an exception and make the user code
                    # deal with it.
                    raise ValueError("Invalid Session ID: sid=%s" % sid)
                    # Derived from the cookie sent by browser,
                    # wipe it out so it gets replaced with a
                    # correct value.
                    self._sid = None

Jim, you wrote the original code which raised the HTTP exception for all cases,
are you happy with this change? Anyone else see any issues with this?

I'll take the usual period of silence to mean I should just make the change and
you do still trust me to the do the correct thing. :-)


Reply via email to