Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BadSignature exception handling in SessionMiddleware #1264

Conversation

hanneskuettner
Copy link
Contributor

The SessionMiddleware does not catch all possible exceptions of TimestampSigner.unsign, specifically there is an edge case where a BadSignature exception can be raised by the method if a totally bogus session cookie is provided that does not match the expected format.

This PR broadens the caught exceptions (since all the other exceptions inherit BadSignature) and fixes the issue.
The issue can be reproduced by running the new test without the fix applied.

@hanneskuettner hanneskuettner force-pushed the fix/session-middleware-handle-invalid-cookie-format branch from 024c811 to 2b64854 Compare August 12, 2021 11:33
Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, thanks 👍

@JayH5 JayH5 merged commit 7e675a0 into encode:master Aug 14, 2021
@hanneskuettner hanneskuettner deleted the fix/session-middleware-handle-invalid-cookie-format branch August 16, 2021 13:24
@gnat
Copy link

gnat commented Sep 27, 2021

Great patch, thanks. Eagerly awaiting the release.

@gnat
Copy link

gnat commented Sep 27, 2021

Just wanted to say I've tested this in my project and it works well. No more non-recoverable Internal Server Errors when passing in malformed session cookies- just an elegant blank session.

Glad to see people stress testing exploitation vectors on Starlette to make it more robust!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants