Skip to content

Fix BadSignature exception handling in SessionMiddleware - #1264

Merged
JayH5 merged 1 commit into
Kludex:masterfrom
hanneskuettner:fix/session-middleware-handle-invalid-cookie-format
Aug 14, 2021
Merged

JayH5 merged 1 commit into
Kludex:masterfrom
hanneskuettner:fix/session-middleware-handle-invalid-cookie-format

Conversation

@hanneskuettner

Copy link
Copy Markdown
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

@JayH5 JayH5 left a comment

Copy link
Copy Markdown
Contributor

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 Kludex:master Aug 14, 2021
@hanneskuettner
hanneskuettner deleted the fix/session-middleware-handle-invalid-cookie-format branch August 16, 2021 13:24
@gnat

gnat commented Sep 27, 2021

Copy link
Copy Markdown

Great patch, thanks. Eagerly awaiting the release.

@gnat

gnat commented Sep 27, 2021

Copy link
Copy Markdown

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.

3 participants