fix(session): add rotate_session_id() against session fixation at login - #255
Merged
Merged
Conversation
A Starlette-style login under FastAPICacheXSessionMiddleware writes into the session the request arrived with and sends the same token back, so whoever planted that cookie is logged in too. rotate_session_id(request) regenerates the loaded session's ID (a no-op when none was loaded) and the middleware sends the new token through the request's transport. The SESSION.md example used SessionDep and answered 401 to new visitors; it now uses the helper, and the migration section explains the difference from Starlette's cookie sessions. Closes #225
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #225.
Problem
Under
FastAPICacheXSessionMiddlewarethe cookie only names a server-side record. A Starlette-style login that writesrequest.session["user_id"] = ...keeps the session ID the request arrived with. As a result, a planted cookie is logged in along with the victim. The documented defence (SESSION.md §5) tookSessionDep, which answers401to a new visitor, so it could not be used as written.Change
rotate_session_id(request) -> boolinfastapi_cachex.session.dependencies, also exported fromfastapi_cachex.session.SessionManager.regenerate_session_id()on it. The middlewares already notice the changed ID and send the new token through the request's transport:Set-Cookie, or the response header.Falseand does nothing. The first write then starts a session under a fresh ID.get_optional_sessionalternative for handlers that callregenerate_session_id()directly.request.sessionmigration section warns about the difference from Starlette.### Securityentry under Unreleased.I left out the automatic "rotate when a user is first attached" config switch the issue mentions. The middleware cannot tell a login from any other write to
request.session, so an explicit call is the reliable option. The breaking version of that idea (an explicitlogin()/logout()that always rotates, a__Host-cookie by default) is tracked for 0.4.0 in #256;rotate_session_id()stays useful there for privilege changes.Tests
tests/session/test_starlette_middleware.py:test_rotate_session_id_defeats_a_planted_cookiereplays the issue's repro:cartplususer_id) is kept;SessionNotFoundError.test_rotate_session_id_without_a_session: a new visitor gets200,rotated: False, and a session cookie.test_rotate_session_id_over_the_header: the header transport returns the new token in the header, with noSet-Cookie.Mutation check: with the
regenerate_session_id()call removed from the helper, exactly the two rotation tests fail. The no-session test guards the401regression, not the rotation.Local results:
ruff checkandruff format --checkpass.mypy --strictpasses.--strict --clean.