catch other pickle errors when loading content #3325
Conversation
| return pickle.loads(bstruct) | ||
| # at least ValueError, AttributeError, ImportError but more to be safe | ||
| except Exception: | ||
| raise ValueError |
There was a problem hiding this comment.
It seems odd that we have no direct unit tests for PickleSerializer.
There was a problem hiding this comment.
Well, you can rest easy because we have some for the SimpleSerializer that does nothing. :-)
There was a problem hiding this comment.
Anyway the short answer is that the PickleSerializer was pulled out of existing session code and so I just assumed pyramid was exercising those paths well enough. Turns out it was missing a few things. Certainly it'd be easier to write tests for such things directly to the PickleSerializer if it got pulled out but I'm also not upset with more integration tests for sessions either.
There was a problem hiding this comment.
I was going to let this slide but we are looking at changing the default serializer so I pushed some specific PickleSerializer tests so they don't get lost in translation later.
mmerickel
force-pushed
the
fix-unpickle-crash
branch
from
eadf7a2 to
9d68512
Compare
Hypothetical: you have more pyramid projects than dummy secrets and the browser starts sending back signed-pickled session cookies from project A to the server for project B. In this case it passes all signature checks but fails to unpickle with a non-
ValueErrorexception.