From 477f88807548c9492adc3faad8d6b3e064b9cac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Contreras=20Guill=C3=A9n?= Date: Fri, 5 Jun 2026 18:40:48 +0200 Subject: [PATCH] fix(session): session-fixation rotation, Secure-cookie auto-detect, Redis allowlist + tests + bump v26.06.13 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the session-hardening follow-up deferred from v26.06.12 (surfaced by the implement-security adversarial audit). The session subsystem was previously untested. - Session fixation: HttpSession.rotate_id() (preserves data, records previous_id); SessionFilter migrates the store entry + re-issues the cookie under the new id; OAuth2 login rotates on successful authentication. - Cookie Secure auto-set when the request is HTTPS (honors X-Forwarded-Proto), even if not explicitly configured — hardens prod without breaking HTTP dev. - RedisSessionStore: rehydration restricted to an allowlist (SecurityContext pre-registered; allow_session_type() to opt in custom types) — closes the arbitrary-object instantiation gadget. - New tests/session suite (16): HttpSession+rotation, InMemory store+TTL, filter (new/existing/invalidate/rotation/secure auto-detect), Redis round-trip + gadget guard. Gates: mypy --strict (607), ruff + format, full suite 3661 passed. --- CHANGELOG.md | 32 +++ README.md | 2 +- pyproject.toml | 2 +- src/pyfly/__init__.py | 2 +- src/pyfly/security/oauth2/login.py | 3 + src/pyfly/session/adapters/redis.py | 28 ++- src/pyfly/session/filter.py | 23 +- src/pyfly/session/session.py | 21 ++ tests/session/__init__.py | 7 + tests/session/test_session_subsystem.py | 285 ++++++++++++++++++++++++ uv.lock | 2 +- 11 files changed, 400 insertions(+), 7 deletions(-) create mode 100644 tests/session/__init__.py create mode 100644 tests/session/test_session_subsystem.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c9ef2ea..028e53ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,38 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). --- +## v26.06.13 (2026-06-05) + +### Security + +- **Session fixation fixed.** Authentication now rotates the session id, so an + attacker who fixed a victim's pre-auth `PYFLY_SESSION` id cannot ride the + authenticated session. New `HttpSession.rotate_id()` (preserves data, records + `previous_id`); `SessionFilter` migrates the store entry and re-issues the + cookie under the new id; the OAuth2 login flow calls it on successful login. +- **Session cookie `Secure` auto-set over HTTPS.** `SessionFilter` now marks the + cookie `Secure` when the request arrives over HTTPS (honoring + `X-Forwarded-Proto`) even if not explicitly configured — hardening production + without breaking plain-HTTP local development. +- **Redis session deserialization hardened.** `RedisSessionStore` rehydrated + *any* tagged type via `importlib` + `obj(**payload)` — an arbitrary-object + instantiation gadget if the store were ever attacker-writable. Rehydration is + now restricted to an allowlist (`SecurityContext` pre-registered); other tagged + values return a plain dict. Apps opt custom dataclasses in via + `allow_session_type()`. + +### Added + +- **`tests/session/` suite (16 tests)** — the session subsystem was previously + untested. Covers `HttpSession` (incl. rotation), `InMemorySessionStore` + (incl. TTL expiry), `SessionFilter` (new/existing/invalidate/rotation/secure + auto-detect), and `RedisSessionStore` (SecurityContext round-trip + the + allowlist gadget guard). + +Completes the session-hardening follow-up deferred from v26.06.12. + +--- + ## v26.06.12 (2026-06-05) ### Security diff --git a/README.md b/README.md index 4d0c33c6..0ea5c4a1 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ Firefly Framework Python 3.12+ License: Apache 2.0 - Version: 26.06.12 + Version: 26.06.13 Type Checked: mypy strict Code Style: Ruff Async First diff --git a/pyproject.toml b/pyproject.toml index e2d5928c..03298cac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ name = "pyfly" # CalVer YY.MM.PATCH — package metadata uses PEP 440 normalized form (26.5.4); # git tag, GitHub release and human-readable display use leading-zero form # (v26.05.04) to match the Java/.NET/Go siblings. -version = "26.6.12" +version = "26.6.13" description = "The official Python implementation of the Firefly Framework — DI, CQRS, EDA, hexagonal architecture, and more." readme = "README.md" license = "Apache-2.0" diff --git a/src/pyfly/__init__.py b/src/pyfly/__init__.py index f334f015..977633e0 100644 --- a/src/pyfly/__init__.py +++ b/src/pyfly/__init__.py @@ -13,4 +13,4 @@ # limitations under the License. """PyFly — Enterprise Python Framework.""" -__version__ = "26.06.12" +__version__ = "26.06.13" diff --git a/src/pyfly/security/oauth2/login.py b/src/pyfly/security/oauth2/login.py index ae7a67b6..c6c67a03 100644 --- a/src/pyfly/security/oauth2/login.py +++ b/src/pyfly/security/oauth2/login.py @@ -185,6 +185,9 @@ async def _handle_callback(self, request: Request) -> Response: status_code=401, ) + # Rotate the session id on successful authentication to prevent session + # fixation — the pre-auth id (which an attacker may have fixed) is dropped. + session.rotate_id() session.set_attribute(_SECURITY_CONTEXT_KEY, security_context) logger.info("OAuth2 login successful for user: %s (via %s)", security_context.user_id, registration_id) diff --git a/src/pyfly/session/adapters/redis.py b/src/pyfly/session/adapters/redis.py index 57982f3b..5877c9c7 100644 --- a/src/pyfly/session/adapters/redis.py +++ b/src/pyfly/session/adapters/redis.py @@ -26,6 +26,22 @@ _KEY_PREFIX = "pyfly:session:" _TYPE_KEY = "__pyfly_type__" +# Tagged dataclass types allowed to be reconstructed from session JSON on read. +# Restricting this prevents an arbitrary-object instantiation gadget if the +# session store is ever attacker-writable. Framework types are pre-registered; +# applications opt their own session-stored dataclasses in via allow_session_type. +_ALLOWED_TYPE_TAGS: set[str] = {"pyfly.security.context:SecurityContext"} + + +def allow_session_type(cls: type) -> None: + """Allow *cls* (a dataclass) to be rehydrated from the Redis session store. + + Only allowlisted tagged types are reconstructed on read; any other tagged + value is returned as a plain dict. Call this once at startup for a custom + dataclass an application stores in the session. + """ + _ALLOWED_TYPE_TAGS.add(f"{cls.__module__}:{cls.__qualname__}") + def _json_default(obj: Any) -> Any: """Encode dataclass session attributes (e.g. SecurityContext) with a type tag. @@ -41,12 +57,20 @@ def _json_default(obj: Any) -> Any: def _json_object_hook(d: dict[str, Any]) -> Any: - """Rehydrate a tagged dataclass dict back into its original type on read.""" + """Rehydrate an allowlisted tagged dataclass dict into its original type. + + A tag that is not on the allowlist is NOT imported or instantiated — the + plain dict is returned instead — to avoid an arbitrary-object instantiation + gadget from session data. + """ tag = d.get(_TYPE_KEY) if not tag: return d - module_name, _, qualname = tag.partition(":") payload = {k: v for k, v in d.items() if k != _TYPE_KEY} + if tag not in _ALLOWED_TYPE_TAGS: + _logger.warning("Refusing to rehydrate non-allowlisted session type %r", tag) + return payload + module_name, _, qualname = tag.partition(":") try: obj: Any = importlib.import_module(module_name) for part in qualname.split("."): diff --git a/src/pyfly/session/filter.py b/src/pyfly/session/filter.py index 2767aa17..65161821 100644 --- a/src/pyfly/session/filter.py +++ b/src/pyfly/session/filter.py @@ -68,7 +68,7 @@ async def do_filter(self, request: Any, call_next: CallNext) -> Any: key=self._cookie_name, value=session.id, httponly=True, - secure=self._secure, + secure=self._secure or self._is_secure_request(request), samesite="lax", max_age=self._ttl, ) @@ -93,7 +93,28 @@ async def _load_or_create_session(self, request: Any) -> HttpSession: async def _persist_session(self, session: HttpSession) -> None: """Save or delete the session in the store based on its state.""" + # If the id was rotated (e.g. on login), drop the pre-rotation entry so a + # fixed/stale id can no longer resolve to this session (anti-fixation). + if session.previous_id is not None and session.previous_id != session.id: + await self._store.delete(session.previous_id) + if session.invalidated: await self._store.delete(session.id) elif session.modified: await self._store.save(session.id, session.get_data(), self._ttl) + + @staticmethod + def _is_secure_request(request: Any) -> bool: + """Whether the request arrived over HTTPS (honoring ``X-Forwarded-Proto``). + + Sets the cookie ``Secure`` attribute automatically in production (HTTPS) + without breaking plain-HTTP local development. + """ + headers = getattr(request, "headers", None) + forwarded = "" + if headers is not None and hasattr(headers, "get"): + forwarded = headers.get("x-forwarded-proto", "") + if forwarded: + return str(forwarded).split(",")[0].strip().lower() == "https" + url = getattr(request, "url", None) + return url is not None and getattr(url, "scheme", "") == "https" diff --git a/src/pyfly/session/session.py b/src/pyfly/session/session.py index 49f7c0f4..94eb5b6d 100644 --- a/src/pyfly/session/session.py +++ b/src/pyfly/session/session.py @@ -16,6 +16,7 @@ from __future__ import annotations import time +import uuid from typing import Any @@ -39,6 +40,7 @@ def __init__( self._is_new = is_new self._invalidated = False self._modified = is_new + self._previous_id: str | None = None now = time.time() if "_created_at" not in self._data: @@ -49,6 +51,11 @@ def __init__( def id(self) -> str: return self._id + @property + def previous_id(self) -> str | None: + """The id this session was rotated away from (set by :meth:`rotate_id`).""" + return self._previous_id + @property def is_new(self) -> bool: return self._is_new @@ -88,6 +95,20 @@ def get_attribute_names(self) -> list[str]: """Return all attribute names, excluding internal metadata keys.""" return [k for k in self._data if not k.startswith("_")] + def rotate_id(self) -> None: + """Assign a fresh session id, preserving all data. + + Call on authentication / privilege elevation to prevent session-fixation + attacks: an attacker who fixed the victim's pre-auth session id cannot + ride the authenticated session. The store entry and cookie are migrated + to the new id when the session is persisted by the ``SessionFilter``. + """ + if self._invalidated: + return + self._previous_id = self._id + self._id = uuid.uuid4().hex + self._modified = True + def invalidate(self) -> None: """Mark the session for deletion.""" self._invalidated = True diff --git a/tests/session/__init__.py b/tests/session/__init__.py new file mode 100644 index 00000000..b6e82019 --- /dev/null +++ b/tests/session/__init__.py @@ -0,0 +1,7 @@ +# Copyright 2026 Firefly Software Foundation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 diff --git a/tests/session/test_session_subsystem.py b/tests/session/test_session_subsystem.py new file mode 100644 index 00000000..c886e727 --- /dev/null +++ b/tests/session/test_session_subsystem.py @@ -0,0 +1,285 @@ +# Copyright 2026 Firefly Software Foundation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Tests for the session subsystem, including the v26.06.13 hardening: + +- session-fixation: ``HttpSession.rotate_id()`` + ``SessionFilter`` store/cookie migration. +- cookie ``Secure`` auto-set over HTTPS (and via ``X-Forwarded-Proto``). +- Redis store rehydration is restricted to an allowlist (no arbitrary-object gadget). +""" + +from __future__ import annotations + +import json +from dataclasses import dataclass +from types import SimpleNamespace +from typing import Any + +import pytest + +from pyfly.security.context import SecurityContext +from pyfly.session.adapters.memory import InMemorySessionStore +from pyfly.session.adapters.redis import RedisSessionStore, allow_session_type +from pyfly.session.filter import SessionFilter +from pyfly.session.session import HttpSession + + +# --------------------------------------------------------------------------- +# HttpSession +# --------------------------------------------------------------------------- +class TestHttpSession: + def test_attribute_roundtrip(self) -> None: + s = HttpSession("sid", is_new=True) + s.set_attribute("user", "ada") + assert s.get_attribute("user") == "ada" + assert s.modified is True + s.remove_attribute("user") + assert s.get_attribute("user") is None + + def test_invalidate(self) -> None: + s = HttpSession("sid", {"k": "v"}) + s.invalidate() + assert s.invalidated is True + + def test_rotate_id_assigns_new_id_and_preserves_data(self) -> None: + s = HttpSession("old-id", {"k": "v"}) + s.rotate_id() + assert s.id != "old-id" + assert s.previous_id == "old-id" + assert s.get_attribute("k") == "v" + assert s.modified is True + + def test_rotate_id_is_noop_when_invalidated(self) -> None: + s = HttpSession("old-id") + s.invalidate() + s.rotate_id() + assert s.id == "old-id" + assert s.previous_id is None + + +# --------------------------------------------------------------------------- +# InMemorySessionStore +# --------------------------------------------------------------------------- +class TestInMemorySessionStore: + @pytest.mark.asyncio + async def test_save_get_delete_exists(self) -> None: + store = InMemorySessionStore() + await store.save("sid", {"a": 1}, ttl=60) + assert await store.get("sid") == {"a": 1} + assert await store.exists("sid") is True + await store.delete("sid") + assert await store.get("sid") is None + assert await store.exists("sid") is False + + @pytest.mark.asyncio + async def test_get_missing_returns_none(self) -> None: + assert await InMemorySessionStore().get("nope") is None + + @pytest.mark.asyncio + async def test_expired_entry_is_evicted(self, monkeypatch: pytest.MonkeyPatch) -> None: + store = InMemorySessionStore() + await store.save("sid", {"a": 1}, ttl=10) + # Jump past expiry deterministically. + monkeypatch.setattr("pyfly.session.adapters.memory.time.monotonic", lambda: 10_000_000.0) + assert await store.get("sid") is None + assert await store.exists("sid") is False + + +# --------------------------------------------------------------------------- +# SessionFilter +# --------------------------------------------------------------------------- +def _request(*, cookies: dict[str, str] | None = None, scheme: str = "http", headers: dict[str, str] | None = None): + return SimpleNamespace( + cookies=cookies or {}, + url=SimpleNamespace(scheme=scheme), + headers=headers or {}, + state=SimpleNamespace(), + ) + + +class _Response: + def __init__(self) -> None: + self.set_cookie_calls: list[dict[str, Any]] = [] + self.deleted: list[str] = [] + + def set_cookie(self, **kwargs: Any) -> None: + self.set_cookie_calls.append(kwargs) + + def delete_cookie(self, *, key: str) -> None: + self.deleted.append(key) + + +class TestSessionFilter: + @pytest.mark.asyncio + async def test_new_session_issues_cookie_insecure_over_http(self) -> None: + store = InMemorySessionStore() + f = SessionFilter(store=store) + request = _request() + response = _Response() + + async def call_next(req: Any) -> _Response: + req.state.session.set_attribute("hello", "world") + return response + + await f.do_filter(request, call_next) + assert len(response.set_cookie_calls) == 1 + cookie = response.set_cookie_calls[0] + assert cookie["httponly"] is True + assert cookie["samesite"] == "lax" + assert cookie["secure"] is False # plain HTTP dev + assert await store.get(cookie["value"]) == request.state.session.get_data() + + @pytest.mark.asyncio + async def test_cookie_secure_over_https(self) -> None: + f = SessionFilter(store=InMemorySessionStore()) + request = _request(scheme="https") + response = _Response() + + async def call_next(req: Any) -> _Response: + req.state.session.set_attribute("x", "1") + return response + + await f.do_filter(request, call_next) + assert response.set_cookie_calls[0]["secure"] is True + + @pytest.mark.asyncio + async def test_cookie_secure_via_forwarded_proto(self) -> None: + f = SessionFilter(store=InMemorySessionStore()) + request = _request(headers={"x-forwarded-proto": "https"}) + response = _Response() + + async def call_next(req: Any) -> _Response: + req.state.session.set_attribute("x", "1") + return response + + await f.do_filter(request, call_next) + assert response.set_cookie_calls[0]["secure"] is True + + @pytest.mark.asyncio + async def test_existing_session_is_loaded(self) -> None: + store = InMemorySessionStore() + await store.save("existing", {"user": "ada"}, ttl=60) + f = SessionFilter(store=store) + request = _request(cookies={"PYFLY_SESSION": "existing"}) + + async def call_next(req: Any) -> _Response: + assert req.state.session.id == "existing" + assert req.state.session.get_attribute("user") == "ada" + return _Response() + + await f.do_filter(request, call_next) + + @pytest.mark.asyncio + async def test_invalidate_deletes_cookie_and_store_entry(self) -> None: + store = InMemorySessionStore() + await store.save("existing", {"user": "ada"}, ttl=60) + f = SessionFilter(store=store) + request = _request(cookies={"PYFLY_SESSION": "existing"}) + response = _Response() + + async def call_next(req: Any) -> _Response: + req.state.session.invalidate() + return response + + await f.do_filter(request, call_next) + assert "PYFLY_SESSION" in response.deleted + assert response.set_cookie_calls == [] + assert await store.get("existing") is None + + @pytest.mark.asyncio + async def test_rotation_migrates_store_and_cookie(self) -> None: + store = InMemorySessionStore() + await store.save("fixed-id", {"user": "ada"}, ttl=60) + f = SessionFilter(store=store) + request = _request(cookies={"PYFLY_SESSION": "fixed-id"}) + response = _Response() + + async def call_next(req: Any) -> _Response: + req.state.session.rotate_id() # e.g. on login + return response + + await f.do_filter(request, call_next) + new_id = response.set_cookie_calls[0]["value"] + assert new_id != "fixed-id" + assert await store.get("fixed-id") is None # old (fixed) id no longer resolves + assert (await store.get(new_id))["user"] == "ada" # data carried to the new id + + +# --------------------------------------------------------------------------- +# RedisSessionStore (fake async client — no redis dependency needed) +# --------------------------------------------------------------------------- +class _FakeRedis: + def __init__(self) -> None: + self.kv: dict[str, bytes] = {} + + async def get(self, key: str) -> bytes | None: + return self.kv.get(key) + + async def set(self, key: str, value: bytes, ex: int | None = None) -> None: + self.kv[key] = value + + async def delete(self, key: str) -> None: + self.kv.pop(key, None) + + async def exists(self, key: str) -> int: + return 1 if key in self.kv else 0 + + +class _Tripwire: + """If the deserialization gadget were active, instantiating this would raise.""" + + def __init__(self, **_kwargs: Any) -> None: + raise AssertionError("non-allowlisted session type was instantiated!") + + +@dataclass +class _Prefs: + """Module-level so its tag (module:_Prefs) resolves via importlib on read.""" + + theme: str = "dark" + + +class TestRedisSessionStore: + @pytest.mark.asyncio + async def test_security_context_roundtrip(self) -> None: + client = _FakeRedis() + store = RedisSessionStore(client=client) + ctx = SecurityContext(user_id="u-1", roles=["ADMIN"], permissions=["order:read"]) + await store.save("sid", {"_sc": ctx}, ttl=60) + + loaded = await store.get("sid") + assert isinstance(loaded["_sc"], SecurityContext) + assert loaded["_sc"].user_id == "u-1" + assert loaded["_sc"].has_role("ADMIN") + + @pytest.mark.asyncio + async def test_non_allowlisted_tag_is_not_instantiated(self, caplog: pytest.LogCaptureFixture) -> None: + client = _FakeRedis() + store = RedisSessionStore(client=client) + tag = f"{_Tripwire.__module__}:{_Tripwire.__qualname__}" + client.kv["pyfly:session:evil"] = json.dumps({"__pyfly_type__": tag, "a": 1}).encode() + + result = await store.get("evil") + # Returned as a plain dict — _Tripwire was NOT imported or instantiated. + assert result == {"a": 1} + + @pytest.mark.asyncio + async def test_allow_session_type_opts_in_a_custom_type(self) -> None: + allow_session_type(_Prefs) + client = _FakeRedis() + store = RedisSessionStore(client=client) + await store.save("sid", {"prefs": _Prefs(theme="light")}, ttl=60) + + loaded = await store.get("sid") + assert isinstance(loaded["prefs"], _Prefs) + assert loaded["prefs"].theme == "light" diff --git a/uv.lock b/uv.lock index 4b4ec0e5..0b88ecd9 100644 --- a/uv.lock +++ b/uv.lock @@ -1967,7 +1967,7 @@ wheels = [ [[package]] name = "pyfly" -version = "26.6.12" +version = "26.6.13" source = { editable = "." } dependencies = [ { name = "pydantic" },