diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a1e705..1f5ecc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,17 @@ Note that 0.3.3 was never released; 0.3.4 follows 0.3.2. ### Fixed +- Regenerating the session ID of the request's session (the documented defence + against session fixation at login) now sends a token for the new ID. + `FastAPICacheXSessionMiddleware` used to re-send the loaded token, which named + the record `regenerate_session_id()` had just deleted, so the user was logged + straight back out. Header clients got no token at all. The deprecated + `SessionMiddleware` could overwrite the new token with a sliding-renewed one + for the old ID. Both middlewares now notice the changed ID and send the new + token through the request's transport. `SessionManager.issue_token(session)` + is the one place tokens are signed. + ([#103](https://github.com/allen0099/FastAPI-CacheX/issues/103)) + - `ttl` means the same thing on every backend. Zero or negative TTLs now raise `ValueError` from `set`, `set_if_absent` and `increment` on all built-in backends, from the base-class fallbacks, and from `CacheManager` and diff --git a/docs/SESSION.md b/docs/SESSION.md index 81f2955..d65a2ca 100644 --- a/docs/SESSION.md +++ b/docs/SESSION.md @@ -598,14 +598,32 @@ match the bound one (or has no address) is treated as having no session. Prevents session fixation attacks: ```python -# After a successful login -session, _renewed_token = await session_manager.get_session(current_token) -session, new_token = await session_manager.regenerate_session_id(session) -# Hand new_token to the client; the old token no longer resolves to a session. +from fastapi_cachex.session.dependencies import SessionDep, SessionManagerDep + + +@app.post("/login") +async def login(request: Request, session: SessionDep, manager: SessionManagerDep): + ... # verify the credentials + await manager.regenerate_session_id(session) + request.session["user_id"] = "123" + return {"ok": True} ``` `regenerate_session_id()` deletes the backend record under the old ID and saves the session under -a new ID, keeping its data, user, `created_at` and expiry. +a new ID, keeping its data, user, `created_at` and expiry. When the session is the request's own +(from `SessionDep`, `get_session` and friends), either middleware sees the new ID and sends a token +for it through the transport the request used: `Set-Cookie` for a cookie, the response header for a +header token. After that the old token no longer resolves to a session. + +Outside a middleware, load the session with the same bindings the middleware would pass, and hand +the returned token to the client yourself: + +```python +session, _ = await manager.get_session( + current_token, ip_address=client_ip, user_agent=user_agent +) +session, new_token = await manager.regenerate_session_id(session) +``` ## SessionManager at a glance diff --git a/fastapi_cachex/session/manager.py b/fastapi_cachex/session/manager.py index d41cb90..63aebe8 100644 --- a/fastapi_cachex/session/manager.py +++ b/fastapi_cachex/session/manager.py @@ -153,8 +153,7 @@ async def _create_session( # Store in backend await self._save_session(session) - # Generate signed token (pass expires_at so JWT exp reflects sliding expiration) - token = self._create_token(session.session_id, expires_at=session.expires_at) + token = self.issue_token(session) logger.debug( "Session created; id=%s ttl=%s ip=%s ua=%s", session.session_id, @@ -163,7 +162,7 @@ async def _create_session( session.user_agent, ) - return session, self._serializer.to_string(token) + return session, token async def get_session( self, @@ -276,10 +275,7 @@ async def get_session( if time_remaining < threshold: session.renew(self.config.session_ttl) - token = self._create_token( - session.session_id, expires_at=session.expires_at - ) - renewed_token = self._serializer.to_string(token) + renewed_token = self.issue_token(session) logger.debug( "Session renewed (sliding expiration); id=%s ttl=%s", session.session_id, @@ -326,6 +322,11 @@ async def regenerate_session_id( ) -> tuple[Session, str]: """Regenerate session ID (after login for security). + ``session`` is changed in place. When it is the request's session under + either middleware, the middleware notices the new ID and sends the new + token through the request's transport, so a handler that only needs + the cookie or header updated can ignore the returned token. + Args: session: Session to regenerate @@ -342,13 +343,28 @@ async def regenerate_session_id( # Save with new ID await self._save_session(session) - # Create new token (pass expires_at so JWT exp reflects current expiry) - token = self._create_token(session.session_id, expires_at=session.expires_at) logger.debug( "Session ID regenerated; old_id=%s new_id=%s", old_id, session.session_id ) - return session, self._serializer.to_string(token) + return session, self.issue_token(session) + + def issue_token(self, session: Session) -> str: + """Sign a token string for ``session``'s current ID and expiry. + + This is the token ``create_session``, sliding renewal and + ``regenerate_session_id`` hand out; the middleware also uses it to + send a fresh token once a handler has regenerated the session ID. + + Args: + session: Session to issue a token for + + Returns: + Serialized session token + """ + # expires_at is passed so a JWT's exp matches the session's expiry. + token = self._create_token(session.session_id, expires_at=session.expires_at) + return self._serializer.to_string(token) async def delete_user_sessions(self, user_id: str) -> int: """Delete all sessions for a user. diff --git a/fastapi_cachex/session/middleware.py b/fastapi_cachex/session/middleware.py index bfe781c..9594b8b 100644 --- a/fastapi_cachex/session/middleware.py +++ b/fastapi_cachex/session/middleware.py @@ -228,11 +228,19 @@ async def dispatch( # Store session in request state setattr(request.state, "__fastapi_cachex_session", session) + loaded_session_id = session.session_id if session is not None else None + # Process request response: Response = await call_next(request) - # Propagate renewed token to client so its JWT exp stays in sync - if renewed_token is not None: + if session is not None and session.session_id != loaded_session_id: + # The handler regenerated the session ID; a renewed token would + # name the deleted record, so send a token for the new ID. + response.headers[self.config.header_name] = ( + self.session_manager.issue_token(session) + ) + elif renewed_token is not None: + # Propagate renewed token to client so its JWT exp stays in sync response.headers[self.config.header_name] = renewed_token return response @@ -321,6 +329,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: loaded_token: str | None = None renewed_token: str | None = None backend_session: Session | None = None + loaded_session_id: str | None = None # Resolve the incoming session token: prefer the header/bearer transport # (e.g. X-Session-Token, as used by SessionMiddleware) and fall back to @@ -340,6 +349,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: user_agent=user_agent, ) loaded_token = renewed_token or token_value + loaded_session_id = backend_session.session_id scope["session"] = StarletteSession(backend_session.data) initial_session_was_empty = not backend_session.data except SessionError: @@ -363,6 +373,10 @@ async def send_wrapper(message: Message) -> None: session: StarletteSession = scope["session"] headers = MutableHeaders(scope=message) + current_token, fresh_token = self._response_tokens( + backend_session, loaded_session_id, loaded_token, renewed_token + ) + if session.accessed: headers.add_vary_header("Cookie") @@ -371,8 +385,8 @@ async def send_wrapper(message: Message) -> None: session, connection, backend_session, - loaded_token, - renewed_token, + current_token, + fresh_token, ) # Header clients only need a genuinely new/renewed token (an # unchanged one is already held); cookie clients always get a @@ -394,15 +408,35 @@ async def send_wrapper(message: Message) -> None: # Cookie transport: expire the cookie. A header-based client # simply drops its now-dangling token (record is deleted). headers.append("Set-Cookie", self._build_clear_cookie_header()) - elif renewed_token is not None: - # Sliding expiration renewed the token even though the dict - # itself was untouched; propagate it via the same transport. - self._emit_token(headers, renewed_token, from_header=from_header) + elif fresh_token is not None: + # Sliding expiration renewed the token, or the ID was + # regenerated, even though the dict itself was untouched; + # propagate it via the same transport. + self._emit_token(headers, fresh_token, from_header=from_header) await send(message) await self.app(scope, receive, send_wrapper) + def _response_tokens( + self, + backend_session: "Session | None", + loaded_session_id: str | None, + loaded_token: str | None, + renewed_token: str | None, + ) -> tuple[str | None, str | None]: + """The ``(current, fresh)`` tokens to answer with. + + Normally the loaded token and the sliding-renewed one (if any). If the + handler regenerated the session ID (at login, say), the loaded token + names a deleted record, so both become a token for the new ID and the + client gets it on either transport. + """ + if backend_session is None or backend_session.session_id == loaded_session_id: + return loaded_token, renewed_token + token = self.session_manager.issue_token(backend_session) + return token, token + def _emit_token( self, headers: MutableHeaders, token: str, *, from_header: bool ) -> None: diff --git a/tests/session/test_starlette_middleware.py b/tests/session/test_starlette_middleware.py index 65ffcab..030150d 100644 --- a/tests/session/test_starlette_middleware.py +++ b/tests/session/test_starlette_middleware.py @@ -726,3 +726,113 @@ async def logout_route(request: Request) -> dict[str, bool]: assert "set-cookie" not in response.headers with pytest.raises(SessionNotFoundError): await manager.get_session(token) + + +def _regenerating_app( + manager: SessionManager, + config: SessionConfig, + *, + write_data: bool, + middleware: Any = FastAPICacheXSessionMiddleware, +) -> FastAPI: + """An app whose /login regenerates the request's session ID, as docs advise.""" + app = FastAPI() + app.add_middleware(middleware, session_manager=manager, config=config) + + @app.post("/login") + async def login(request: Request, session=Depends(get_session)): + await manager.regenerate_session_id(session) + if write_data: + request.session["logged_in"] = True + return {"ok": True} + + return app + + +async def _shorten_expiry(manager: SessionManager, token: str) -> None: + """Push the session under the sliding threshold so the load renews it.""" + session, _ = await manager.get_session(token) + session.expires_at = datetime.now(timezone.utc) + timedelta(seconds=100) + await manager._save_session(session) + + +@pytest.mark.parametrize("write_data", [True, False]) +@pytest.mark.parametrize("sliding", [True, False]) +@pytest.mark.asyncio +async def test_regenerated_session_id_is_sent_as_cookie( + manager: SessionManager, config: SessionConfig, write_data: bool, sliding: bool +) -> None: + """After regenerate_session_id() the cookie must carry a token for the new ID. + + The middleware used to re-send the loaded token, which named the deleted + record, so logging in with the documented fixation defence logged the user + straight back out. + """ + _session, old_token = await manager.create_session( + user=SessionUser(user_id="u"), a=1 + ) + if sliding: + await _shorten_expiry(manager, old_token) + client = TestClient(_regenerating_app(manager, config, write_data=write_data)) + client.cookies.set(config.cookie_name, old_token) + + response = client.post("/login") + + assert response.status_code == 200 + new_token = _extract_cookie_token( + response.headers["set-cookie"], config.cookie_name + ) + assert new_token != old_token + session, _ = await manager.get_session(new_token) + expected = {"a": 1, "logged_in": True} if write_data else {"a": 1} + assert session.data == expected + with pytest.raises(SessionNotFoundError): + await manager.get_session(old_token) + + +@pytest.mark.parametrize("write_data", [True, False]) +@pytest.mark.parametrize("sliding", [True, False]) +@pytest.mark.asyncio +async def test_regenerated_session_id_is_sent_in_the_header( + manager: SessionManager, config: SessionConfig, write_data: bool, sliding: bool +) -> None: + """A header client gets the new ID's token back in the response header.""" + _session, old_token = await manager.create_session(user=SessionUser(user_id="u")) + if sliding: + await _shorten_expiry(manager, old_token) + client = TestClient(_regenerating_app(manager, config, write_data=write_data)) + + response = client.post("/login", headers={config.header_name: old_token}) + + assert response.status_code == 200 + assert "set-cookie" not in response.headers + new_token = response.headers[config.header_name] + assert new_token != old_token + await manager.get_session(new_token) + with pytest.raises(SessionNotFoundError): + await manager.get_session(old_token) + + +@pytest.mark.filterwarnings("ignore::DeprecationWarning") +@pytest.mark.parametrize("sliding", [True, False]) +@pytest.mark.asyncio +async def test_deprecated_middleware_sends_regenerated_token( + manager: SessionManager, config: SessionConfig, sliding: bool +) -> None: + """SessionMiddleware must not overwrite the new ID's token with a renewed old one.""" + _session, old_token = await manager.create_session(user=SessionUser(user_id="u")) + if sliding: + await _shorten_expiry(manager, old_token) + client = TestClient( + _regenerating_app( + manager, config, write_data=False, middleware=SessionMiddleware + ) + ) + + response = client.post("/login", headers={config.header_name: old_token}) + + assert response.status_code == 200 + new_token = response.headers[config.header_name] + await manager.get_session(new_token) + with pytest.raises(SessionNotFoundError): + await manager.get_session(old_token)