[mod_python] Can i have both Marshal and Signed CookieswithPublisher?

Clodoaldo Pinto Neto wrote ..
> 2006/10/29, Graham Dumpleton <grahamd at dscpl.com.au>:
> > Clodoaldo Pinto Neto wrote ..
> > > 2006/10/29, Graham Dumpleton <grahamd at dscpl.com.au>:
> > > IMHO, the least surprise behavior is if Cookie.get_cookies() returned
> > > all cookies of the given class regardless of how many different cookie
> > > classes there are in the header.
> >
> > There is nothing in a cookie string though to be able to identify it
> as being
> > of a particular type. The only way you can know is to try and decode
> it and
> > if it works then it probably is.
> 
> It is exactly what MarshalCookie.parse() does:

But it is still an assumption because they aren't tagged as being of a
particular type. :-)
 
>     def parse(Class, s, secret):
> 
>         dict = _parse_cookie(s, Class)
> 
>         for k in dict:
>             c = dict[k]
>             try:
>                 c.unmarshal(secret)
>             except (CookieError, ValueError):
>                 # downgrade to Cookie
>                 dict[k] = Cookie.parse(Cookie.__str__(c))[k]
> 
>         return dict
> 
> The problem that is happening is that one exception was not expected
> by the original code piece author. The exception is binascii.Error. It
> will happen whenever MarshalCookie.parse() tries to parse a Signed
> cookie. My proposed fix is just to import binascii and include the
> exception:
> 
>             try:
>                 c.unmarshal(secret)
>             except (CookieError, ValueError, binascii.Error):
>                 # downgrade to Cookie
>                 dict[k] = Cookie.parse(Cookie.__str__(c))[k]
> 
> This way MarshalCookie.get_cookies() will behave as per the documentation.

Yes and no. Yes one could do it, but after looking at it and thinking about
it overnight I would probably do it in a different place. Instead, one would
be better of first changing unmarshal() to:

    def unmarshal(self, secret):

        self.unsign(secret)

        try:
            data = base64.decodestring(self.value)
        except binascii.Error:
            raise CookieError, "Incorrectly Encoded Cookie: %s=%s" % (self.name, self.value)

        self.value = marshal.loads(data)

and leave parse() as is.

The difference here is that you have split out the base64 decoding of
the value string from the actual unmarshalling of the data.

Next one has to contemplate whether one should for the marshal.loads()
call separately catch EOFError, ValueError or TypeError exceptions as
being indicative of badly formatted marshal data.

The reason for doing this would be that I could use a SignedCookie at
the same time where I decide to base64 encode the value myself. If I do
this and the SignedCookie uses the same secret, the data would get
past the signed check and the base64 decoding implemented by the
MarshalCookie but then fail on unmarshalling if the data wasn't actually
marshal data but some other format.

So, to be even more precise:

    def unmarshal(self, secret):

        self.unsign(secret)

        try:
            data = base64.decodestring(self.value)
        except:
            raise CookieError, "Value Not BASE64 Encoded: %s=%s" % (self.name, self.value)

        try:
            self.value = marshal.loads(data)
        except (EOFError, ValueError, TypeError):
            raise CookieError, "Cannot Unmarshal Cookie: %s=%s" % (self.name, self.value)

> > This is probably not a good way of doing
> > things. First off the application should only be decoding its own cookies
> > and not others which may have been sent to the site in general. Thus,
> allowing
> > one to say which cookies to decode is probably a better step.
> 
> I'm not opposing the new names argument. I just don't know enough
> about mod_python to opine on it, although the names argument alone
> will not fix the issue: an *expected* error that should be handled by
> mod_python. Or better, the names argument will only fix the issue if
> it is passed *and* if none of the passed cookie names is a Signed
> cookie. A nice addition perhaps, but not a fix.

The other thing I believe also should be added is a new keyword
parameter called 'strict'. This would default to 0 to preserve existing
behaviour. So I could say:

  cookies = Cookie.get_cookies(req, Cookie.MarshalCookie, names=['xxx'], strict=1)

If 'strict' is set to a truth value, then if a cookie fails to be decoded as
the intended type, it would not fallback to returning Cookie, but raise
again the original exception that cause the cookie not to be decoded.

The Session code would for example always use 'strict' to ensure that
SignedCookie doesn't fallback to 'Cookie', thus ensuring the extra
security of the 64 character vs 32 character string is used.

Graham