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",
apache.APLOG_WARNING)
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
managed
to get into the browsers cache, a web application isn't able to automatically
just
discard the existing value and replace it with a brand new SID. Instead, it is
pushed
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
be
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
session
with the session ID in the browser being replaced.
That said, the other way the SID can be supplied is if the application code
supplied
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,
however
it should be a general ValueError exception to make it easier for the
application
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)
else:
# 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. :-)
Graham
