From c9947fe0ac75b4ed2b89101cf992d739dfd90987 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:27:05 +0100 Subject: [PATCH 1/7] fix(users): CSRF-protect browser auth mutations; logout retires the browser session server-side Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 9 ++ docs/database/migrations.md | 2 +- docs/modules/users.md | 56 ++++++++- .../e805a1b2c3d4_users_browser_sessions.py | 34 +++++ host/tests/test_migration_graph.py | 4 +- modules/users/tests-js/users_Login.test.tsx | 4 + .../tests/test_session_version_hardening.py | 2 +- .../tests/test_users_browser_auth_boundary.py | 93 ++++++++++++++ .../test_users_browser_origin_validation.py | 50 ++++++++ .../test_users_browser_session_logout.py | 118 ++++++++++++++++++ .../test_users_browser_session_migration.py | 33 +++++ modules/users/users/auth_local/api.py | 9 +- .../users/users/auth_local/browser_guard.py | 41 ++++++ modules/users/users/auth_local/views.py | 18 ++- modules/users/users/browser_sessions.py | 36 ++++++ modules/users/users/manager.py | 6 +- modules/users/users/models/__init__.py | 2 + modules/users/users/models/revoked_session.py | 19 +++ modules/users/users/module.py | 16 ++- modules/users/users/provider.py | 43 ++++--- modules/users/users/session_version_cache.py | 19 +-- 21 files changed, 572 insertions(+), 42 deletions(-) create mode 100644 host/migrations/versions/e805a1b2c3d4_users_browser_sessions.py create mode 100644 modules/users/tests/test_users_browser_auth_boundary.py create mode 100644 modules/users/tests/test_users_browser_origin_validation.py create mode 100644 modules/users/tests/test_users_browser_session_logout.py create mode 100644 modules/users/tests/test_users_browser_session_migration.py create mode 100644 modules/users/users/auth_local/browser_guard.py create mode 100644 modules/users/users/browser_sessions.py create mode 100644 modules/users/users/models/revoked_session.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f6c16b03..b390cd783 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] +### Security +- **Behaviour change:** local browser auth mutations now validate their source + and session-bound CSRF token, extending the shared token support (#504). + Native API clients without browser headers retain their existing workflow. +- **Behaviour change:** local logout retires only the current browser session, + using the existing account-cache invalidation channel (#318). Migration + `e805a1b2c3d4` adds expiring session retirement records. Sessions issued before + the upgrade remain compatible until expiry or next sign-in. + ## [0.0.43] — 2026-11-04 ### Added diff --git a/docs/database/migrations.md b/docs/database/migrations.md index 8eb81d8e6..2ed1fe13a 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -71,7 +71,7 @@ The history is **not** a single linear chain. It has several roots and several h | Head | Owning module | Label Alembic reports | |---|---|---| -| `e731a1b2c3d4` | users (unique `lower(email)`, #731) | `api_tokens`, `background_tasks`, `file_storage`, `users` | +| `e805a1b2c3d4` | users (browser session retirement) | `api_tokens`, `background_tasks`, `file_storage`, `users` | | `a7c2e91d4b58` | audit_log | `audit_log` | | `f1a9c3d7b2e4` | tenants | `tenants` | | `d263ea727663` | framework (`sm_lease`) | `leases` | diff --git a/docs/modules/users.md b/docs/modules/users.md index 19f4304a8..ff92a0f74 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -33,7 +33,7 @@ fires `on_after_verify` followed by `on_after_update`, then signs the user in. | Method + path | Body | Notes | |---|---|---| | `POST /api/users/auth/login` | `OAuth2PasswordRequestForm` | sets `sm_auth` cookie + `session["user_id"]`; `400 LOGIN_BAD_CREDENTIALS`; `429` + `Retry-After` while locked — see [Login lockout](#login-lockout) | -| `POST /api/users/auth/logout` | — | clears the `sm_auth` cookie and the session | +| `POST /api/users/auth/logout` | — | retires this browser session, clears the `sm_auth` cookie and the session | | `POST /api/users/auth/register` | `UserCreate` | gated by `users.allow_signup`; rate-limited | | `POST /api/users/auth/forgot-password` | `PasswordReset` | rate-limited | | `POST /api/users/auth/reset-password` | `{token, password}` | | @@ -77,7 +77,7 @@ fires `on_after_verify` followed by `on_after_update`, then signs the user in. Public: - `GET /users/login` → `Users/Login` (shows dev-account buttons in dev) -- `POST /users/logout` → 303 redirect, clears cookie +- `POST /users/logout` → 303 redirect, retires this browser session and clears cookies - `GET /users/register` → `Users/Register` (404 if signup disabled) - `GET /users/forgot-password` → `Users/ForgotPassword` - `GET /users/reset-password` → `Users/ResetPassword` @@ -547,6 +547,58 @@ In the admin UI, the user list and edit page show an **"External · SSO"** badge Anything else without a session redirects to `/users/login` (or returns 401 JSON for `/api/*` / bearer requests). +## Browser auth requests and logout + +**Behaviour change:** anonymous auth pages issue a session-bound `csrfToken` prop. +Browser mutations under `/api/users/auth/` (password sign-in, demo sign-in, +registration, forgot/reset password, verification, invite acceptance, bearer token +operations and API logout) must echo it as `X-CSRF-Token`. The shared typed API +client already does this, including sign-in after a password reset. Read the prop +from `GET /users/login` through Inertia when building another browser client; +retain its session cookie. Signing in rotates the token: read the next page's +prop before another mutation. Missing or mismatched tokens return 403. + +Requests with `Origin`, `Referer` or `Sec-Fetch-Site` are treated as browser +requests. Foreign origins and `Sec-Fetch-Site: cross-site` are refused even with +a token; same-site sibling origins still need a token. Origin comparison uses +the request's effective scheme, host and port, so deployments behind a proxy +must configure the framework's trusted proxy handling correctly. The Inertia +`POST /users/logout` uses the origin check without requiring a token header, +so the existing menu submission continues to work. + +Native JSON/form API clients without browser headers continue to work without +first fetching an HTML page. If they explicitly send `X-CSRF-Token`, it is +validated against their session too. This compatibility exception is why the +browser guard combines source validation with the framework's `RequiresCsrf` +primitive rather than requiring a token unconditionally. Browser clients must +keep these auth pages private and uncached. + +The users OAuth, OIDC and Keycloak GET login starters only establish a login +transaction and redirect to the identity provider. Their callbacks validate the +session-bound state before signing in; OIDC and Keycloak additionally validate +nonce and use PKCE. These redirects continue to accept external navigation. + +**Behaviour change:** both logout routes retire this browser's session ID +server-side and revoke its `sm_auth` access-token row, then clear the cookies. +Other devices stay signed in. Password, invite, demo and users OAuth sign-ins +mint a fresh ID through the shared users sign-in hook. Migration +`e805a1b2c3d4` adds `users_revoked_session`, indexed by account and expiry; +logout lazily removes expired records. Records remain until the session's +original absolute deadline, including the remembered window. + +Retired IDs share the account's session-version cache entry and +`session_version_cache_ttl_seconds` (default 30 seconds). Logout publishes on the +existing `users.session_version` invalidation channel **after commit**. A Redis +invalidation transport propagates eviction to other workers; without one, or +if a message is lost, their TTL bounds the delay. Warm requests add no database +query. The existing availability policy still applies: a cache miss during a +database outage preserves an already-cached principal until a successful read. + +Sessions issued before this upgrade have no ID. They continue to work until +expiry or next sign-in; logout clears their browser cookies but cannot retire +another copy individually. Password changes and “Sign out everywhere” still +retire all sessions through the account version counter. + ## Roles cache `roles_cache.py` keeps an in-memory list of `RoleSummary(id, name)` on `app.state.users.roles_cache`. Refreshed at startup and on demand via `refresh_roles_cache(app)`. When the role set changes (the setup wizard's first administrator creates the `admin` role), `publish_roles_changed(app)` announces it on the `users.roles` invalidation channel: each subscribed worker empties its cache and `get_roles_cache` refills it on the next read (other workers hear it where the Redis invalidation transport is installed). Because delivery is not guaranteed, the cache also expires on its own after `TTL_SECONDS` (300 s, stamped as `roles_cache_loaded_at` on the users state) and is refetched on the next read. Used by the admin UI to render role pickers without hitting the DB on every request. diff --git a/host/migrations/versions/e805a1b2c3d4_users_browser_sessions.py b/host/migrations/versions/e805a1b2c3d4_users_browser_sessions.py new file mode 100644 index 000000000..fcde176e5 --- /dev/null +++ b/host/migrations/versions/e805a1b2c3d4_users_browser_sessions.py @@ -0,0 +1,34 @@ +"""Track retired local browser sessions. + +Revision ID: e805a1b2c3d4 +Revises: e731a1b2c3d4 +""" + +import sqlalchemy as sa +from alembic import op +from fastapi_users_db_sqlalchemy.generics import GUID +from simple_module_db.types import AwareDateTime + +revision = "e805a1b2c3d4" +down_revision = "e731a1b2c3d4" +branch_labels = None +depends_on = None + +_TABLE = "users_revoked_session" + + +def upgrade() -> None: + op.create_table( + _TABLE, + sa.Column("session_id", sa.String(64), primary_key=True, nullable=False), + sa.Column( + "user_id", GUID(), sa.ForeignKey("users_user.id", ondelete="CASCADE"), nullable=False + ), + sa.Column("expires_at", AwareDateTime(), nullable=False), + ) + op.create_index("ix_users_revoked_session_user_id", _TABLE, ["user_id"]) + op.create_index("ix_users_revoked_session_expires_at", _TABLE, ["expires_at"]) + + +def downgrade() -> None: + op.drop_table(_TABLE) diff --git a/host/tests/test_migration_graph.py b/host/tests/test_migration_graph.py index 6749026ac..0ea737f57 100644 --- a/host/tests/test_migration_graph.py +++ b/host/tests/test_migration_graph.py @@ -44,6 +44,7 @@ "a625f0b7c912": "users", # refresh-token families (#625) "a626b1c8d023": "users", # hashed bearer tokens (#626) "e731a1b2c3d4": "users", # unique lower(email) (#731) + "e805a1b2c3d4": "users", "51b2058d8afd": "api_tokens", "168a2882f443": "keycloak", "f6a3b9d2e8c4": "keycloak", @@ -69,7 +70,7 @@ # head revision -> module owning the tables it creates or alters. _HEADS = { "a7c2e91d4b58": "audit_log", - "e731a1b2c3d4": "users", # also carries ``api_tokens``, ``background_tasks``, ``file_storage`` + "e805a1b2c3d4": "users", # also carries ``api_tokens``, ``background_tasks``, ``file_storage`` "f1a9c3d7b2e4": "tenants", "f6a3b9d2e8c4": "keycloak", "c9d2e4f6a810": "oidc", @@ -117,6 +118,7 @@ "b31a70971001": "api_tokens", "f5b1c3e8a207": "api_tokens", "e731a1b2c3d4": "api_tokens", + "e805a1b2c3d4": "api_tokens", "a626b1c8d023": "api_tokens", # also inherits ``background_tasks`` "51b2058d8afd": "api_tokens", # ancestors of a labelled revision inherit its label too } diff --git a/modules/users/tests-js/users_Login.test.tsx b/modules/users/tests-js/users_Login.test.tsx index b7fbb71e7..eac367bbb 100644 --- a/modules/users/tests-js/users_Login.test.tsx +++ b/modules/users/tests-js/users_Login.test.tsx @@ -12,6 +12,7 @@ vi.mock('@inertiajs/react', () => ({ usePage: () => ({ props: { branding: { appName: 'Acme', logoUrl: null } } }), })); +import { setCsrfToken } from '@simple-module-py/ui/lib/api'; import Login from '../users/pages/Login'; import { json, sent, stubFetch } from './users_fixtures'; @@ -37,6 +38,7 @@ function submit() { } afterEach(() => { + setCsrfToken(undefined); vi.unstubAllGlobals(); vi.clearAllMocks(); }); @@ -59,6 +61,7 @@ describe('users Login', () => { }); it('posts the form-encoded credentials and follows the server redirect on 204', async () => { + setCsrfToken('anonymous-session-token'); const mock = stubFetch(() => new Response(null, { status: 204 })); render(); submit(); @@ -66,6 +69,7 @@ describe('users Login', () => { const call = await sent(mock); expect(call.path).toBe('/api/users/auth/login'); expect(call.method).toBe('POST'); + expect(mock.mock.calls[0][0].headers.get('X-CSRF-Token')).toBe('anonymous-session-token'); expect(call.contentType).toContain('application/x-www-form-urlencoded'); expect(call.credentials).toBe('same-origin'); const form = new URLSearchParams(call.text); diff --git a/modules/users/tests/test_session_version_hardening.py b/modules/users/tests/test_session_version_hardening.py index 6eb02090a..ab69cddae 100644 --- a/modules/users/tests/test_session_version_hardening.py +++ b/modules/users/tests/test_session_version_hardening.py @@ -238,5 +238,5 @@ def test_the_module_level_cache_is_the_one_the_helpers_use(): configure_session_version_cache(7) store_session_version("dana", 1) - assert cache_module._SESSION_VERSIONS.get("dana") == 1 + assert cache_module.peek_session_version("dana") == 1 assert cache_module._SESSION_VERSIONS.ttl == 7 diff --git a/modules/users/tests/test_users_browser_auth_boundary.py b/modules/users/tests/test_users_browser_auth_boundary.py new file mode 100644 index 000000000..30eeae06c --- /dev/null +++ b/modules/users/tests/test_users_browser_auth_boundary.py @@ -0,0 +1,93 @@ +"""Browser auth mutations use the anonymous session's token.""" + +import pytest + +LOGIN = "/api/users/auth/login" +CREDENTIALS = {"username": "admin@example.com", "password": "AdminPass1!"} + + +@pytest.mark.parametrize( + "path", + [ + LOGIN, + "/api/users/auth/register", + "/api/users/auth/forgot-password", + "/api/users/auth/reset-password", + "/api/users/auth/request-verify-token", + "/api/users/auth/verify", + "/api/users/auth/accept-invite", + "/api/users/auth/demo/user", + "/api/users/auth/logout", + ], +) +async def test_browser_mutations_require_session_token(anon_client, path): + for headers in [ + {"Origin": "https://other.example"}, + {"Origin": "null"}, + {"Referer": "https://other.example/form"}, + {"Sec-Fetch-Site": "cross-site"}, + {"Sec-Fetch-Site": "same-site"}, + {"Origin": "http://testserver"}, + ]: + response = await anon_client.post(path, headers=headers, data=CREDENTIALS) + assert response.status_code == 403, (headers, response.text) + assert "sm_auth=" not in response.headers.get("set-cookie", "") + + +async def test_anonymous_page_token_allows_login_and_rotates(anon_client): + props = ( + await anon_client.get( + "/users/login", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} + ) + ).json()["props"] + token = props["csrfToken"] + headers = {"Origin": "http://testserver", "X-CSRF-Token": token} + assert (await anon_client.post(LOGIN, data=CREDENTIALS, headers=headers)).status_code == 204 + assert (await anon_client.post(LOGIN, data=CREDENTIALS, headers=headers)).status_code == 403 + + +@pytest.mark.parametrize("header", ["Origin", "Referer"]) +async def test_foreign_source_refused_even_with_token(anon_client, header): + token = ( + await anon_client.get( + "/users/login", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} + ) + ).json()["props"]["csrfToken"] + response = await anon_client.post( + LOGIN, + data=CREDENTIALS, + headers={header: "https://other.example", "X-CSRF-Token": token}, + ) + assert response.status_code == 403 + + +async def test_non_browser_password_client_keeps_working(anon_client): + assert (await anon_client.post(LOGIN, data=CREDENTIALS)).status_code == 204 + + +async def test_view_logout_accepts_same_origin_inertia_post(anon_client): + assert (await anon_client.post(LOGIN, data=CREDENTIALS)).status_code == 204 + refused = await anon_client.post("/users/logout", headers={"Origin": "https://other.example"}) + assert refused.status_code == 403 + assert ( + await anon_client.post("/users/logout", headers={"Origin": "http://testserver"}) + ).status_code == 303 + + +@pytest.mark.parametrize( + "path", + [ + "/users/login", + "/users/register", + "/users/forgot-password", + "/users/reset-password", + "/users/verify", + "/users/invite/accept", + ], +) +async def test_anonymous_auth_pages_supply_token(anon_client_signup, path): + response = await anon_client_signup.get( + path, headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} + ) + assert response.status_code == 200 + assert response.json()["props"]["csrfToken"] diff --git a/modules/users/tests/test_users_browser_origin_validation.py b/modules/users/tests/test_users_browser_origin_validation.py new file mode 100644 index 000000000..2cda71c3d --- /dev/null +++ b/modules/users/tests/test_users_browser_origin_validation.py @@ -0,0 +1,50 @@ +"""Source normalization and token validation at the browser boundary.""" + +import pytest +from fastapi import HTTPException, Request +from simple_module_hosting.csrf import get_csrf_token +from users.auth_local.browser_guard import require_browser_csrf + + +@pytest.mark.parametrize( + ("headers", "allowed"), + [ + ({"origin": "http://testserver:80"}, True), + ({"referer": "http://testserver/login?next=%2F"}, True), + ({"origin": "https://testserver"}, False), + ({"origin": "http://testserver:8080"}, False), + ({"origin": "http://testserver:0"}, False), + ({"origin": "http://testserver:invalid"}, False), + ({"origin": "http://testserver.evil.example"}, False), + ({"origin": "http://person@testserver"}, False), + ({"origin": "http://[broken"}, False), + ({"sec-fetch-site": "cross-site"}, False), + ({"sec-fetch-site": "same-site"}, True), + ], +) +async def test_browser_source_with_valid_session_token(headers, allowed): + request = Request( + { + "type": "http", + "method": "POST", + "scheme": "http", + "path": "/api/users/auth/login", + "root_path": "", + "server": ("testserver", 80), + "session": {}, + "headers": [(b"host", b"testserver")], + } + ) + token = get_csrf_token(request) + request.scope["headers"].extend( + (name.encode(), value.encode()) + for name, value in {**headers, "x-csrf-token": token}.items() + ) + # Request.headers may already be cached by URL construction. + request = Request(request.scope) + if allowed: + await require_browser_csrf(request) + else: + with pytest.raises(HTTPException) as error: + await require_browser_csrf(request) + assert error.value.status_code == 403 diff --git a/modules/users/tests/test_users_browser_session_logout.py b/modules/users/tests/test_users_browser_session_logout.py new file mode 100644 index 000000000..7ca800532 --- /dev/null +++ b/modules/users/tests/test_users_browser_session_logout.py @@ -0,0 +1,118 @@ +"""Logout ends one browser session, including an earlier copy of its cookie.""" + +import httpx +import pytest +from users.session_version_cache import clear_session_version_cache + +CREDENTIALS = {"username": "admin@example.com", "password": "AdminPass1!"} + + +@pytest.mark.parametrize("path", ["/users/logout", "/api/users/auth/logout"]) +@pytest.mark.parametrize("remember", [False, True]) +async def test_logout_revokes_only_this_session(users_app, anon_client, path, remember): + transport = httpx.ASGITransport(app=users_app) + async with httpx.AsyncClient(transport=transport, base_url="http://testserver") as other: + for client in [anon_client, other]: + response = await client.post( + "/api/users/auth/login", data={**CREDENTIALS, "remember": str(remember).lower()} + ) + assert response.status_code == 204 + initial = anon_client.cookies.get("session") + # Cache both the principal in the signed cookie and the server's counter. + assert (await anon_client.get("/api/users/me")).status_code == 200 + copied = anon_client.cookies.get("session") + assert (await anon_client.post(path)).status_code in {204, 303} + async with httpx.AsyncClient( + transport=transport, base_url="http://testserver", cookies={"session": copied} + ) as replay: + assert (await replay.get("/api/users/me")).status_code == 401 + clear_session_version_cache() + replay.cookies.clear() + replay.cookies.set("session", copied) + assert (await replay.get("/api/users/me")).status_code == 401 + replay.cookies.clear() + replay.cookies.set("session", initial) + assert (await replay.get("/api/users/me")).status_code == 401 + assert (await other.get("/api/users/me")).status_code == 200 + + +async def test_legacy_cookie_without_id_remains_accepted(users_app, admin_client): + assert (await admin_client.get("/api/users/me")).status_code == 200 + # The fixture deliberately forges the pre-upgrade shape without an id. + copied = admin_client.cookies.get("session", domain="testserver.local") + await admin_client.post("/api/users/auth/logout") + admin_client.cookies.clear() + admin_client.cookies.set("session", copied) + assert (await admin_client.get("/api/users/me")).status_code == 200 + + +async def test_logout_cleans_expired_records_and_retains_deadline(users_app, anon_client): + from datetime import UTC, datetime, timedelta + + from _users_app_builders import user_row + from sqlalchemy import select + from users.models import RevokedSession + + user = await user_row(users_app, "admin@example.com") + async with users_app.state.sm.db.session_factory() as db: + db.add( + RevokedSession( + session_id="expired", + user_id=user.id, + expires_at=datetime.now(UTC) - timedelta(days=1), + ) + ) + await db.commit() + assert (await anon_client.post("/api/users/auth/login", data=CREDENTIALS)).status_code == 204 + before = datetime.now(UTC) + assert (await anon_client.post("/api/users/auth/logout")).status_code == 204 + async with users_app.state.sm.db.session_factory() as db: + rows = list((await db.execute(select(RevokedSession))).scalars()) + assert len(rows) == 1 + assert rows[0].session_id != "expired" + remaining = rows[0].expires_at - before + assert timedelta(days=13) < remaining <= timedelta(days=14) + + +async def test_remote_invalidation_drops_cached_retirements(): + import uuid + + from simple_module_core.invalidation import InvalidationBus + from users.session_revocation import CHANNEL, subscribe + from users.session_version_cache import read_session_version, store_session_version + + user_id = uuid.uuid4() + bus = InvalidationBus() + subscribe(bus) + store_session_version(user_id, 0, frozenset({"retired"})) + assert read_session_version(user_id, "retired") == (True, None) + assert read_session_version(user_id, "other-device") == (True, 0) + await bus.deliver( + f'{{"v": 1, "channel": "{CHANNEL}", "key": "{user_id}", "origin": "other-worker"}}' + ) + assert read_session_version(user_id, "retired") == (False, None) + + +async def test_logout_broadcasts_after_record_is_durable(users_app, anon_client): + import json + + from sqlalchemy import select + from users.models import RevokedSession + from users.session_revocation import CHANNEL + + messages = [] + + class Transport: + async def publish(self, message): + payload = json.loads(message) + if payload["channel"] == CHANNEL: + async with users_app.state.sm.db.session_factory() as db: + records = list((await db.execute(select(RevokedSession))).scalars()) + assert len(records) == 1 + assert str(records[0].user_id) == payload["key"] + messages.append(payload) + + users_app.state.sm.invalidation.set_transport(Transport()) + assert (await anon_client.post("/api/users/auth/login", data=CREDENTIALS)).status_code == 204 + assert (await anon_client.post("/api/users/auth/logout")).status_code == 204 + assert len(messages) == 1 diff --git a/modules/users/tests/test_users_browser_session_migration.py b/modules/users/tests/test_users_browser_session_migration.py new file mode 100644 index 000000000..d87aace46 --- /dev/null +++ b/modules/users/tests/test_users_browser_session_migration.py @@ -0,0 +1,33 @@ +"""The users revision supports a local upgrade/downgrade/upgrade.""" + +import importlib.util +from pathlib import Path + +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations + + +def test_browser_session_revision_roundtrip(monkeypatch): + path = ( + Path(__file__).resolve().parents[3] + / "host/migrations/versions/e805a1b2c3d4_users_browser_sessions.py" + ) + spec = importlib.util.spec_from_file_location("browser_session_revision", path) + revision = importlib.util.module_from_spec(spec) + spec.loader.exec_module(revision) + engine = sa.create_engine("sqlite://") + with engine.begin() as connection: + connection.execute(sa.text("CREATE TABLE users_user (id CHAR(36) PRIMARY KEY)")) + monkeypatch.setattr(revision, "op", Operations(MigrationContext.configure(connection))) + for _ in range(2): + revision.upgrade() + inspector = sa.inspect(connection) + assert "users_revoked_session" in inspector.get_table_names() + assert {index["name"] for index in inspector.get_indexes("users_revoked_session")} == { + "ix_users_revoked_session_user_id", + "ix_users_revoked_session_expires_at", + } + revision.downgrade() + assert "users_revoked_session" not in sa.inspect(connection).get_table_names() + engine.dispose() diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index f588673cb..4a5ff7c0a 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -23,6 +23,7 @@ from simple_module_hosting.token_guard import Authenticated from sqlalchemy.ext.asyncio import AsyncSession +from users.auth_local.browser_guard import require_browser_csrf from users.auth_local.lockout import ( admit, login_attempt, @@ -33,6 +34,7 @@ from users.auth_local.rate_limit import enforce_auth_throughput_limit from users.auth_local.self_account import router as self_account_router from users.backend import cookie_login_response +from users.browser_sessions import revoke_browser_session from users.constants import SESSION_USER_ID_KEY from users.contracts.schemas import ( AcceptInviteRequest, @@ -92,7 +94,7 @@ async def _remembered_login_response(request: Request, user, access_token_db) -> ) -@router.post("/auth/login", status_code=204) +@router.post("/auth/login", status_code=204, dependencies=[Depends(require_browser_csrf)]) async def login( request: Request, response: Response, @@ -166,9 +168,10 @@ async def login( # sits outside the lockout above (GH #530), and nothing called its logout. -@router.post("/auth/logout", status_code=204) +@router.post("/auth/logout", status_code=204, dependencies=[Depends(require_browser_csrf)]) async def api_logout(request: Request, db: AsyncSession = Depends(get_db)): """API logout — revokes the access token, clears its cookie and the session.""" + await revoke_browser_session(request, db) request.session.clear() cookie_name = request.app.state.users.settings.cookie_name await revoke_access_token(db, request.cookies.get(cookie_name)) @@ -183,7 +186,7 @@ async def api_logout(request: Request, db: AsyncSession = Depends(get_db)): @router.post( "/auth/accept-invite", status_code=204, - dependencies=[Depends(enforce_auth_throughput_limit)], + dependencies=[Depends(require_browser_csrf), Depends(enforce_auth_throughput_limit)], ) async def accept_invite( body: AcceptInviteRequest, diff --git a/modules/users/users/auth_local/browser_guard.py b/modules/users/users/auth_local/browser_guard.py new file mode 100644 index 000000000..4a2e449f8 --- /dev/null +++ b/modules/users/users/auth_local/browser_guard.py @@ -0,0 +1,41 @@ +"""Session-bound validation for browser auth mutations.""" + +from urllib.parse import urlsplit + +from fastapi import HTTPException, Request +from simple_module_hosting.csrf import CSRF_HEADER, RequiresCsrf + + +def _origin(value: str) -> tuple[str, str, int | None]: + parsed = urlsplit(value) + if parsed.scheme not in {"http", "https"} or not parsed.hostname or parsed.username: + raise ValueError("Invalid origin") + return ( + parsed.scheme, + parsed.hostname, + parsed.port if parsed.port is not None else (443 if parsed.scheme == "https" else 80), + ) + + +async def require_browser_origin(request: Request) -> None: + """Refuse browser mutations initiated by a foreign origin.""" + if request.method in {"GET", "HEAD", "OPTIONS"}: + return + source = request.headers.get("origin") or request.headers.get("referer") + site = request.headers.get("sec-fetch-site") + try: + foreign = source is not None and _origin(source) != _origin(str(request.base_url)) + except ValueError: + foreign = True + if foreign or site == "cross-site": + raise HTTPException(status_code=403, detail="Browser request source refused") + + +async def require_browser_csrf(request: Request) -> None: + """Native clients omit browser headers; browsers echo their page's token.""" + await require_browser_origin(request) + if ( + any(name in request.headers for name in ("origin", "referer", "sec-fetch-site")) + or CSRF_HEADER in request.headers + ): + await RequiresCsrf()(request) diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index 5112602db..f79f63984 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -7,13 +7,16 @@ from fastapi import APIRouter, Depends, HTTPException, Request from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from simple_module_db.deps import get_db +from simple_module_hosting.csrf import get_csrf_token from simple_module_hosting.inertia_deps import InertiaDep from simple_module_inertia import InertiaResponse from sqlalchemy.ext.asyncio import AsyncSession from starlette.responses import RedirectResponse +from users.auth_local.browser_guard import require_browser_origin from users.auth_local.token_preview import decode_verify_token, preview_reset from users.bootstrap import resolve_bootstrap_credentials +from users.browser_sessions import revoke_browser_session from users.contracts.schemas import UserRead from users.demo import resolve_demo_accounts from users.invites.preview import preview_invite @@ -74,6 +77,7 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: return await inertia.render( _PAGE_LOGIN, { + "csrfToken": get_csrf_token(request), "allow_signup": users_settings.allow_signup, # One entry per configured demo account, in button order. Carries # no passwords: each button posts to /api/users/auth/demo/{role} @@ -119,10 +123,11 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: ) -@router.post("/logout", response_model=None) +@router.post("/logout", response_model=None, dependencies=[Depends(require_browser_origin)]) async def logout(request: Request, db: AsyncSession = Depends(get_db)) -> RedirectResponse: """POST-only to resist cross-site `` logout attacks — the menu's logout link submits this as an Inertia form.""" + await revoke_browser_session(request, db) request.session.clear() cookie_name = request.app.state.users.settings.cookie_name await revoke_access_token(db, request.cookies.get(cookie_name)) @@ -141,7 +146,11 @@ async def register_page(request: Request, inertia: InertiaDep) -> InertiaRespons # Drives the "verify your email" bullet and the success screen, so the # page never promises an email that ``on_after_register`` won't send. return await inertia.render( - _PAGE_REGISTER, {"require_verification": users_settings.require_verification} + _PAGE_REGISTER, + { + "csrfToken": get_csrf_token(request), + "require_verification": users_settings.require_verification, + }, ) @@ -151,6 +160,7 @@ async def forgot_password_page(request: Request, inertia: InertiaDep) -> Inertia return await inertia.render( _PAGE_FORGOT_PASSWORD, { + "csrfToken": get_csrf_token(request), "reset_link_lifetime_minutes": ( users_settings.reset_password_token_lifetime_seconds // _SECONDS_PER_MINUTE ), @@ -177,6 +187,7 @@ async def reset_password_page( return await inertia.render( _PAGE_RESET_PASSWORD, { + "csrfToken": get_csrf_token(request), "token": token, # The address the reset is for — "Save and sign in" needs it to # complete the sign-in without asking for it again. @@ -202,6 +213,7 @@ async def verify_page(request: Request, inertia: InertiaDep, token: str = "") -> return await inertia.render( _PAGE_VERIFY_EMAIL, { + "csrfToken": get_csrf_token(request), "token": token, "email": claims.get("email") if claims else None, "verification_lifetime_days": ( @@ -213,6 +225,7 @@ async def verify_page(request: Request, inertia: InertiaDep, token: str = "") -> @router.get("/invite/accept", response_model=None) async def accept_invite_page( + request: Request, inertia: InertiaDep, user_manager: UserManager = Depends(get_user_manager), token: str = "", @@ -222,6 +235,7 @@ async def accept_invite_page( return await inertia.render( _PAGE_ACCEPT_INVITE, { + "csrfToken": get_csrf_token(request), "token": token, "invite": invite, }, diff --git a/modules/users/users/browser_sessions.py b/modules/users/users/browser_sessions.py new file mode 100644 index 000000000..61118a296 --- /dev/null +++ b/modules/users/users/browser_sessions.py @@ -0,0 +1,36 @@ +"""Retire the current browser session and refresh the account's cached state.""" + +import uuid +from datetime import UTC, datetime + +from auth.constants import SESSION_ID_KEY +from fastapi import Request +from simple_module_db.callbacks import register_on_commit +from simple_module_hosting.session import SESSION_EXPIRES_AT_KEY +from sqlalchemy import delete +from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import col + +from users.constants import SESSION_USER_ID_KEY +from users.models import RevokedSession +from users.session_revocation import publish_revocation + + +async def revoke_browser_session(request: Request, db: AsyncSession) -> None: + session = request.session + identity = session.get(SESSION_ID_KEY) + user_id = session.get(SESSION_USER_ID_KEY) + if not identity or not user_id: + return + now = datetime.now(UTC) + await db.execute(delete(RevokedSession).where(col(RevokedSession.expires_at) <= now)) + # Merge makes repeated logout requests for the same cookie idempotent. + await db.merge( + RevokedSession( + session_id=identity, + user_id=uuid.UUID(user_id), + expires_at=datetime.fromtimestamp(session[SESSION_EXPIRES_AT_KEY], UTC), + ) + ) + await db.flush() + register_on_commit(db, lambda: publish_revocation(request.app, uuid.UUID(user_id))) diff --git a/modules/users/users/manager.py b/modules/users/users/manager.py index 0891b3a2d..3d72c15c1 100644 --- a/modules/users/users/manager.py +++ b/modules/users/users/manager.py @@ -4,10 +4,12 @@ import asyncio import logging +import secrets import uuid from datetime import UTC, datetime from typing import TYPE_CHECKING +from auth.constants import SESSION_ID_KEY from fastapi import Depends, Request from fastapi_users import BaseUserManager, UUIDIDMixin, exceptions from fastapi_users.jwt import generate_jwt @@ -206,10 +208,10 @@ async def on_after_login( for key in (*LOGIN_SESSION_PRESERVED_KEYS, *sign_in_preserved_keys(request.app)) if key in request.session } - # Signed-cookie sessions have no server-side id: replace their state - # on sign-in, including CSRF, cached principal and remember/demo flags. + # Replace principal-bound state on sign-in, including CSRF and cached flags. request.session.clear() request.session.update(preserved) + request.session[SESSION_ID_KEY] = secrets.token_urlsafe(24) request.session[SESSION_USER_ID_KEY] = str(user.id) # Stamp the session with the revocation counter it was minted # under. ``UsersAuthProvider`` refuses any session whose stamp has diff --git a/modules/users/users/models/__init__.py b/modules/users/users/models/__init__.py index d92f31f93..8672c5cf0 100644 --- a/modules/users/users/models/__init__.py +++ b/modules/users/users/models/__init__.py @@ -11,6 +11,7 @@ from users.models.access_token import UserAccessToken from users.models.oauth_account import OAuthAccount from users.models.refresh_token import RefreshToken +from users.models.revoked_session import RevokedSession from users.models.role import Role from users.models.user import User from users.models.user_role import UserRole @@ -19,6 +20,7 @@ "Base", "OAuthAccount", "RefreshToken", + "RevokedSession", "Role", "SQLAlchemyAccessTokenDatabase", "SQLAlchemyUserDatabase", diff --git a/modules/users/users/models/revoked_session.py b/modules/users/users/models/revoked_session.py new file mode 100644 index 000000000..f875547a1 --- /dev/null +++ b/modules/users/users/models/revoked_session.py @@ -0,0 +1,19 @@ +"""Per-browser session retirement records.""" + +import uuid +from datetime import datetime + +from fastapi_users_db_sqlalchemy.generics import GUID +from sqlmodel import Field + +from users.models._base import Base + + +class RevokedSession(Base, table=True): # ty: ignore[unsupported-base] + __tablename__ = "users_revoked_session" + + session_id: str = Field(primary_key=True, max_length=64) + user_id: uuid.UUID = Field( + sa_type=GUID, foreign_key="users_user.id", ondelete="CASCADE", index=True + ) + expires_at: datetime = Field(index=True) diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 236210b22..0b9e9bfc8 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -216,6 +216,7 @@ def locale_dirs(self) -> dict[str, Path]: def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from users.admin.api import admin_router from users.auth_local import api as auth_local_api + from users.auth_local.browser_guard import require_browser_csrf from users.auth_local.token_api import router as token_router from users.auth_local.views import router as auth_views from users.contracts.schemas import UserCreate, UserRead @@ -224,8 +225,8 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None from users.oauth.api import register_oauth_routes api_router.include_router(auth_local_api.router) - api_router.include_router(demo_router) - api_router.include_router(token_router) + api_router.include_router(demo_router, dependencies=[Depends(require_browser_csrf)]) + api_router.include_router(token_router, dependencies=[Depends(require_browser_csrf)]) api_router.include_router(admin_router) # Throughput-wrap the stock fastapi-users routers; ``require_signup_enabled`` # gates /register at request time so ``allow_signup`` is hot-reloadable. @@ -233,19 +234,26 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None fastapi_users.get_reset_password_router(), prefix="/auth", tags=["users-auth"], - dependencies=[Depends(auth_local_api.enforce_auth_throughput_limit)], + dependencies=[ + Depends(require_browser_csrf), + Depends(auth_local_api.enforce_auth_throughput_limit), + ], ) api_router.include_router( fastapi_users.get_verify_router(UserRead), # ty: ignore[invalid-argument-type] prefix="/auth", tags=["users-auth"], - dependencies=[Depends(auth_local_api.enforce_auth_throughput_limit)], + dependencies=[ + Depends(require_browser_csrf), + Depends(auth_local_api.enforce_auth_throughput_limit), + ], ) api_router.include_router( fastapi_users.get_register_router(UserRead, UserCreate), # ty: ignore[invalid-argument-type] prefix="/auth", tags=["users-auth"], dependencies=[ + Depends(require_browser_csrf), Depends(auth_local_api.require_signup_enabled), Depends(auth_local_api.enforce_auth_throughput_limit), ], diff --git a/modules/users/users/provider.py b/modules/users/users/provider.py index 85fec660e..bb6e50bf3 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -12,6 +12,7 @@ from collections.abc import Mapping from typing import Any +from auth.constants import SESSION_ID_KEY from auth.contracts.schemas import UserContext from auth.contracts.tokens import is_api_token, parse_bearer from simple_module_hosting.session import ( @@ -59,6 +60,7 @@ def _stamped_version(session) -> int: def _forget(session) -> None: """Drop everything this session claimed about who is signed in.""" + session.pop(SESSION_ID_KEY, None) session.pop(SESSION_USER_ID_KEY, None) session.pop(_SESSION_USER_CTX_KEY, None) session.pop(SESSION_VERSION_KEY, None) @@ -129,21 +131,16 @@ async def resolve_user(self, request: Request) -> UserContext | None: _forget(session) return None - cached = UserContext.from_session_dict(session.get(_SESSION_USER_CTX_KEY)) - if cached is not None and cached.id == user_id_str: - # The cached context lives in the client's own signed cookie, so - # the server cannot reach in and delete it. One indexed - # primary-key read is what makes "Sign out everywhere" actually - # sign out a browser this process has never seen — the alternative - # is a button that only logs out the person pressing it. - if await self._version_still_current(request.scope, user_uuid, session): - # Cookies minted before #381 still cache the retired user-row - # tenant; the active tenant now comes from memberships alone. - cached.tenant_id = None - return cached + if not await self._version_still_current(request.scope, user_uuid, session): _forget(session) return None + cached = UserContext.from_session_dict(session.get(_SESSION_USER_CTX_KEY)) + if cached is not None and cached.id == user_id_str: + # The active tenant comes from memberships, never the cached row. + cached.tenant_id = None + return cached + user_ctx = await self._load_user(request.scope, user_uuid, session) if user_ctx is None: _forget(session) @@ -194,19 +191,25 @@ async def _version_still_current(self, scope, user_id: uuid_mod.UUID, session) - knowing about before shortening the TTL to 0, which removes the shield entirely. """ - hit, cached = read_session_version(user_id) + hit, cached = read_session_version(user_id, session.get(SESSION_ID_KEY)) if hit: return cached is not None and int(cached) == _stamped_version(session) try: from sqlalchemy import select - from users.models import User + from users.models import RevokedSession, User session_factory = scope["app"].state.sm.db.session_factory async with session_factory() as db_session: - stored = ( - await db_session.execute(select(User.session_version).where(User.id == user_id)) - ).scalar_one_or_none() + rows = ( + await db_session.execute( + select(User.session_version, RevokedSession.session_id) + .outerjoin(RevokedSession, col(RevokedSession.user_id) == User.id) + .where(User.id == user_id) + ) + ).all() + stored = rows[0][0] if rows else None + revoked = frozenset(row[1] for row in rows if row[1] is not None) except Exception: # A database that cannot be reached is not evidence of a # revocation. Refusing here would log every signed-in user out of @@ -214,10 +217,12 @@ async def _version_still_current(self, scope, user_id: uuid_mod.UUID, session) - # request should retry rather than inherit the outage. logger.exception("Session version check failed for %s; keeping the session", user_id) return True - store_session_version(user_id, stored) + store_session_version(user_id, stored, revoked) if stored is None: return False - return int(stored) == _stamped_version(session) + return ( + int(stored) == _stamped_version(session) and session.get(SESSION_ID_KEY) not in revoked + ) def get_login_url(self, request: Request | None, next_url: str | None = None) -> str: return "/users/login" diff --git a/modules/users/users/session_version_cache.py b/modules/users/users/session_version_cache.py index 29708023d..63572dc51 100644 --- a/modules/users/users/session_version_cache.py +++ b/modules/users/users/session_version_cache.py @@ -66,7 +66,7 @@ _CACHE_MAXSIZE = 10_000 _SESSION_VERSIONS: TTLCache = TTLCache(maxsize=_CACHE_MAXSIZE, ttl=SESSION_VERSION_TTL_SECONDS) -"""``user_id -> session_version`` (or ``None`` for "no such row"). +"""``user_id -> (session_version, retired session ids)``. Bounded and per-process. An LRU eviction is not a correctness problem: a miss just costs the read the cache was avoiding. @@ -93,7 +93,8 @@ def peek_session_version(user_id): caller that needs to tell "absent" from "cached as missing" apart wants :func:`read_session_version`, which reports both in one read. """ - return _SESSION_VERSIONS.get(user_id) + entry = _SESSION_VERSIONS.get(user_id) + return entry[0] if entry is not None else None def clear_session_version_cache() -> None: @@ -101,9 +102,11 @@ def clear_session_version_cache() -> None: _SESSION_VERSIONS.clear() -def read_session_version(user_id) -> tuple[bool, int | None]: +def read_session_version(user_id, session_id: str | None = None) -> tuple[bool, int | None]: """``(hit, stored)`` — ``hit`` is False when nothing is cached. + A retired session answers ``(True, None)`` like an absent account. + Two return values rather than a sentinel in the *signature* because ``None`` is a legitimate cached answer ("no such row"), and collapsing it with "not cached" would turn a deleted account into a DB read on every request. @@ -116,12 +119,14 @@ def read_session_version(user_id) -> tuple[bool, int | None]: cached = _SESSION_VERSIONS.get(user_id, _MISS) if cached is _MISS: return False, None - return True, cached + return True, None if session_id in cached[1] else cached[0] -def store_session_version(user_id, stored: int | None) -> None: - """Record what the DB answered for this account.""" - _SESSION_VERSIONS[user_id] = None if stored is None else int(stored) +def store_session_version( + user_id, stored: int | None, revoked: frozenset[str] = frozenset() +) -> None: + """Record the account counter and retired IDs together with one TTL.""" + _SESSION_VERSIONS[user_id] = (None if stored is None else int(stored), revoked) def session_version_ttl() -> float: From 8555b18e683c302acc8171deb0c2d62451830dff Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:33:16 +0100 Subject: [PATCH 2/7] refactor(optimize): share logout teardown and auth guards; tidy session cache and tests Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/users/tests-js/users_Login.test.tsx | 2 +- modules/users/tests-js/users_fixtures.ts | 1 + .../tests/test_users_browser_auth_boundary.py | 22 ++++----- .../test_users_browser_session_logout.py | 47 +++++++++---------- modules/users/users/auth_local/api.py | 8 +--- .../users/users/auth_local/browser_guard.py | 7 ++- modules/users/users/auth_local/views.py | 8 +--- modules/users/users/browser_sessions.py | 10 ++++ modules/users/users/module.py | 16 +++---- modules/users/users/provider.py | 11 ++--- modules/users/users/session_version_cache.py | 5 +- 11 files changed, 63 insertions(+), 74 deletions(-) diff --git a/modules/users/tests-js/users_Login.test.tsx b/modules/users/tests-js/users_Login.test.tsx index eac367bbb..f356c0bdf 100644 --- a/modules/users/tests-js/users_Login.test.tsx +++ b/modules/users/tests-js/users_Login.test.tsx @@ -69,7 +69,7 @@ describe('users Login', () => { const call = await sent(mock); expect(call.path).toBe('/api/users/auth/login'); expect(call.method).toBe('POST'); - expect(mock.mock.calls[0][0].headers.get('X-CSRF-Token')).toBe('anonymous-session-token'); + expect(call.csrf).toBe('anonymous-session-token'); expect(call.contentType).toContain('application/x-www-form-urlencoded'); expect(call.credentials).toBe('same-origin'); const form = new URLSearchParams(call.text); diff --git a/modules/users/tests-js/users_fixtures.ts b/modules/users/tests-js/users_fixtures.ts index 1bda48081..e371ed2d4 100644 --- a/modules/users/tests-js/users_fixtures.ts +++ b/modules/users/tests-js/users_fixtures.ts @@ -18,6 +18,7 @@ export async function sent(mock: ReturnType, index = 0) { method: request.method, contentType: request.headers.get('Content-Type'), credentials: request.credentials, + csrf: request.headers.get('X-CSRF-Token'), text: await request.clone().text(), }; } diff --git a/modules/users/tests/test_users_browser_auth_boundary.py b/modules/users/tests/test_users_browser_auth_boundary.py index 30eeae06c..e7f03571b 100644 --- a/modules/users/tests/test_users_browser_auth_boundary.py +++ b/modules/users/tests/test_users_browser_auth_boundary.py @@ -4,6 +4,11 @@ LOGIN = "/api/users/auth/login" CREDENTIALS = {"username": "admin@example.com", "password": "AdminPass1!"} +_INERTIA = {"X-Inertia": "true", "X-Inertia-Version": "1.0"} + + +async def _login_token(client) -> str: + return (await client.get("/users/login", headers=_INERTIA)).json()["props"]["csrfToken"] @pytest.mark.parametrize( @@ -35,12 +40,7 @@ async def test_browser_mutations_require_session_token(anon_client, path): async def test_anonymous_page_token_allows_login_and_rotates(anon_client): - props = ( - await anon_client.get( - "/users/login", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} - ) - ).json()["props"] - token = props["csrfToken"] + token = await _login_token(anon_client) headers = {"Origin": "http://testserver", "X-CSRF-Token": token} assert (await anon_client.post(LOGIN, data=CREDENTIALS, headers=headers)).status_code == 204 assert (await anon_client.post(LOGIN, data=CREDENTIALS, headers=headers)).status_code == 403 @@ -48,11 +48,7 @@ async def test_anonymous_page_token_allows_login_and_rotates(anon_client): @pytest.mark.parametrize("header", ["Origin", "Referer"]) async def test_foreign_source_refused_even_with_token(anon_client, header): - token = ( - await anon_client.get( - "/users/login", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} - ) - ).json()["props"]["csrfToken"] + token = await _login_token(anon_client) response = await anon_client.post( LOGIN, data=CREDENTIALS, @@ -86,8 +82,6 @@ async def test_view_logout_accepts_same_origin_inertia_post(anon_client): ], ) async def test_anonymous_auth_pages_supply_token(anon_client_signup, path): - response = await anon_client_signup.get( - path, headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} - ) + response = await anon_client_signup.get(path, headers=_INERTIA) assert response.status_code == 200 assert response.json()["props"]["csrfToken"] diff --git a/modules/users/tests/test_users_browser_session_logout.py b/modules/users/tests/test_users_browser_session_logout.py index 7ca800532..aa0d086b0 100644 --- a/modules/users/tests/test_users_browser_session_logout.py +++ b/modules/users/tests/test_users_browser_session_logout.py @@ -1,8 +1,21 @@ """Logout ends one browser session, including an earlier copy of its cookie.""" +import json +import uuid +from datetime import UTC, datetime, timedelta + import httpx import pytest -from users.session_version_cache import clear_session_version_cache +from _users_app_builders import user_row +from simple_module_core.invalidation import InvalidationBus +from sqlalchemy import select +from users.models import RevokedSession +from users.session_revocation import CHANNEL, subscribe +from users.session_version_cache import ( + clear_session_version_cache, + read_session_version, + store_session_version, +) CREDENTIALS = {"username": "admin@example.com", "password": "AdminPass1!"} @@ -26,13 +39,13 @@ async def test_logout_revokes_only_this_session(users_app, anon_client, path, re transport=transport, base_url="http://testserver", cookies={"session": copied} ) as replay: assert (await replay.get("/api/users/me")).status_code == 401 - clear_session_version_cache() - replay.cookies.clear() - replay.cookies.set("session", copied) - assert (await replay.get("/api/users/me")).status_code == 401 - replay.cookies.clear() - replay.cookies.set("session", initial) - assert (await replay.get("/api/users/me")).status_code == 401 + # The same copy again with a cold counter cache, then the first cookie. + for cookie, cold in [(copied, True), (initial, False)]: + if cold: + clear_session_version_cache() + replay.cookies.clear() + replay.cookies.set("session", cookie) + assert (await replay.get("/api/users/me")).status_code == 401 assert (await other.get("/api/users/me")).status_code == 200 @@ -47,12 +60,6 @@ async def test_legacy_cookie_without_id_remains_accepted(users_app, admin_client async def test_logout_cleans_expired_records_and_retains_deadline(users_app, anon_client): - from datetime import UTC, datetime, timedelta - - from _users_app_builders import user_row - from sqlalchemy import select - from users.models import RevokedSession - user = await user_row(users_app, "admin@example.com") async with users_app.state.sm.db.session_factory() as db: db.add( @@ -75,12 +82,6 @@ async def test_logout_cleans_expired_records_and_retains_deadline(users_app, ano async def test_remote_invalidation_drops_cached_retirements(): - import uuid - - from simple_module_core.invalidation import InvalidationBus - from users.session_revocation import CHANNEL, subscribe - from users.session_version_cache import read_session_version, store_session_version - user_id = uuid.uuid4() bus = InvalidationBus() subscribe(bus) @@ -94,12 +95,6 @@ async def test_remote_invalidation_drops_cached_retirements(): async def test_logout_broadcasts_after_record_is_durable(users_app, anon_client): - import json - - from sqlalchemy import select - from users.models import RevokedSession - from users.session_revocation import CHANNEL - messages = [] class Transport: diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index 4a5ff7c0a..f3ac7576e 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -34,7 +34,7 @@ from users.auth_local.rate_limit import enforce_auth_throughput_limit from users.auth_local.self_account import router as self_account_router from users.backend import cookie_login_response -from users.browser_sessions import revoke_browser_session +from users.browser_sessions import end_browser_session from users.constants import SESSION_USER_ID_KEY from users.contracts.schemas import ( AcceptInviteRequest, @@ -51,7 +51,6 @@ ) from users.invites.accept import complete_invite from users.manager import UserManager -from users.token_storage import revoke_access_token logger = logging.getLogger(__name__) router = APIRouter() @@ -171,10 +170,7 @@ async def login( @router.post("/auth/logout", status_code=204, dependencies=[Depends(require_browser_csrf)]) async def api_logout(request: Request, db: AsyncSession = Depends(get_db)): """API logout — revokes the access token, clears its cookie and the session.""" - await revoke_browser_session(request, db) - request.session.clear() - cookie_name = request.app.state.users.settings.cookie_name - await revoke_access_token(db, request.cookies.get(cookie_name)) + cookie_name = await end_browser_session(request, db) response = Response(status_code=204) response.delete_cookie(cookie_name, path="/") return response diff --git a/modules/users/users/auth_local/browser_guard.py b/modules/users/users/auth_local/browser_guard.py index 4a2e449f8..9e8416af5 100644 --- a/modules/users/users/auth_local/browser_guard.py +++ b/modules/users/users/auth_local/browser_guard.py @@ -6,7 +6,7 @@ from simple_module_hosting.csrf import CSRF_HEADER, RequiresCsrf -def _origin(value: str) -> tuple[str, str, int | None]: +def _origin(value: str) -> tuple[str, str, int]: parsed = urlsplit(value) if parsed.scheme not in {"http", "https"} or not parsed.hostname or parsed.username: raise ValueError("Invalid origin") @@ -34,8 +34,7 @@ async def require_browser_origin(request: Request) -> None: async def require_browser_csrf(request: Request) -> None: """Native clients omit browser headers; browsers echo their page's token.""" await require_browser_origin(request) - if ( - any(name in request.headers for name in ("origin", "referer", "sec-fetch-site")) - or CSRF_HEADER in request.headers + if any( + name in request.headers for name in ("origin", "referer", "sec-fetch-site", CSRF_HEADER) ): await RequiresCsrf()(request) diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index f79f63984..e5ee39acd 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -16,7 +16,7 @@ from users.auth_local.browser_guard import require_browser_origin from users.auth_local.token_preview import decode_verify_token, preview_reset from users.bootstrap import resolve_bootstrap_credentials -from users.browser_sessions import revoke_browser_session +from users.browser_sessions import end_browser_session from users.contracts.schemas import UserRead from users.demo import resolve_demo_accounts from users.invites.preview import preview_invite @@ -24,7 +24,6 @@ from users.manager import UserManager, get_user_manager from users.models import User from users.oauth.errors import OAUTH_ERROR_CODES -from users.token_storage import revoke_access_token router = APIRouter() @@ -127,10 +126,7 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: async def logout(request: Request, db: AsyncSession = Depends(get_db)) -> RedirectResponse: """POST-only to resist cross-site `` logout attacks — the menu's logout link submits this as an Inertia form.""" - await revoke_browser_session(request, db) - request.session.clear() - cookie_name = request.app.state.users.settings.cookie_name - await revoke_access_token(db, request.cookies.get(cookie_name)) + cookie_name = await end_browser_session(request, db) # 303 forces the follow-up to GET — Inertia treats the redirect as a full # navigation rather than replaying the POST. response = RedirectResponse("/", status_code=303) diff --git a/modules/users/users/browser_sessions.py b/modules/users/users/browser_sessions.py index 61118a296..9b1292f03 100644 --- a/modules/users/users/browser_sessions.py +++ b/modules/users/users/browser_sessions.py @@ -14,6 +14,7 @@ from users.constants import SESSION_USER_ID_KEY from users.models import RevokedSession from users.session_revocation import publish_revocation +from users.token_storage import revoke_access_token async def revoke_browser_session(request: Request, db: AsyncSession) -> None: @@ -34,3 +35,12 @@ async def revoke_browser_session(request: Request, db: AsyncSession) -> None: ) await db.flush() register_on_commit(db, lambda: publish_revocation(request.app, uuid.UUID(user_id))) + + +async def end_browser_session(request: Request, db: AsyncSession) -> str: + """Revoke the browser session and its access token; return the cookie name.""" + await revoke_browser_session(request, db) + request.session.clear() + cookie_name = request.app.state.users.settings.cookie_name + await revoke_access_token(db, request.cookies.get(cookie_name)) + return cookie_name diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 0b9e9bfc8..a03d58b74 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -228,34 +228,30 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None api_router.include_router(demo_router, dependencies=[Depends(require_browser_csrf)]) api_router.include_router(token_router, dependencies=[Depends(require_browser_csrf)]) api_router.include_router(admin_router) + csrf = Depends(require_browser_csrf) + throughput = Depends(auth_local_api.enforce_auth_throughput_limit) # Throughput-wrap the stock fastapi-users routers; ``require_signup_enabled`` # gates /register at request time so ``allow_signup`` is hot-reloadable. api_router.include_router( fastapi_users.get_reset_password_router(), prefix="/auth", tags=["users-auth"], - dependencies=[ - Depends(require_browser_csrf), - Depends(auth_local_api.enforce_auth_throughput_limit), - ], + dependencies=[csrf, throughput], ) api_router.include_router( fastapi_users.get_verify_router(UserRead), # ty: ignore[invalid-argument-type] prefix="/auth", tags=["users-auth"], - dependencies=[ - Depends(require_browser_csrf), - Depends(auth_local_api.enforce_auth_throughput_limit), - ], + dependencies=[csrf, throughput], ) api_router.include_router( fastapi_users.get_register_router(UserRead, UserCreate), # ty: ignore[invalid-argument-type] prefix="/auth", tags=["users-auth"], dependencies=[ - Depends(require_browser_csrf), + csrf, Depends(auth_local_api.require_signup_enabled), - Depends(auth_local_api.enforce_auth_throughput_limit), + throughput, ], ) register_oauth_routes(api_router) diff --git a/modules/users/users/provider.py b/modules/users/users/provider.py index bb6e50bf3..6992b9799 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -191,7 +191,8 @@ async def _version_still_current(self, scope, user_id: uuid_mod.UUID, session) - knowing about before shortening the TTL to 0, which removes the shield entirely. """ - hit, cached = read_session_version(user_id, session.get(SESSION_ID_KEY)) + session_id = session.get(SESSION_ID_KEY) + hit, cached = read_session_version(user_id, session_id) if hit: return cached is not None and int(cached) == _stamped_version(session) try: @@ -220,9 +221,7 @@ async def _version_still_current(self, scope, user_id: uuid_mod.UUID, session) - store_session_version(user_id, stored, revoked) if stored is None: return False - return ( - int(stored) == _stamped_version(session) and session.get(SESSION_ID_KEY) not in revoked - ) + return int(stored) == _stamped_version(session) and session_id not in revoked def get_login_url(self, request: Request | None, next_url: str | None = None) -> str: return "/users/login" @@ -266,8 +265,8 @@ async def _load_user(self, scope, user_id: uuid_mod.UUID, session=None) -> UserC user = (await db_session.execute(stmt)).scalar_one_or_none() if user is None or not user.is_active or user.disabled_at is not None: return None - # Same revocation check as the cached path, free here because - # the row is already loaded. + # Backs up the earlier version check, which fails open on a + # DB error; free here because the row is already loaded. if session is not None and int(user.session_version or 0) != _stamped_version( session ): diff --git a/modules/users/users/session_version_cache.py b/modules/users/users/session_version_cache.py index 63572dc51..b63819b03 100644 --- a/modules/users/users/session_version_cache.py +++ b/modules/users/users/session_version_cache.py @@ -119,7 +119,10 @@ def read_session_version(user_id, session_id: str | None = None) -> tuple[bool, cached = _SESSION_VERSIONS.get(user_id, _MISS) if cached is _MISS: return False, None - return True, None if session_id in cached[1] else cached[0] + version, retired = cached + if session_id in retired: + return True, None + return True, version def store_session_version( From 665ab2479719ba30a015250fe8eb5f517cb5145f Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:39:19 +0100 Subject: [PATCH 3/7] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 4 +-- docs/modules/users.md | 15 +++++---- .../tests/test_users_browser_auth_boundary.py | 17 +++++++--- .../test_users_browser_origin_validation.py | 10 ++---- .../users/users/auth_local/browser_guard.py | 31 ++++++------------- modules/users/users/browser_sessions.py | 8 +++-- modules/users/users/provider.py | 11 +++++-- 7 files changed, 51 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b390cd783..bb4eec5a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,8 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Security -- **Behaviour change:** local browser auth mutations now validate their source - and session-bound CSRF token, extending the shared token support (#504). +- **Behaviour change:** local browser auth mutations now refuse `Sec-Fetch-Site: cross-site` + and require the session-bound CSRF token (no Origin/Referer equality check, so TLS proxies work), extending the shared token support (#504). Native API clients without browser headers retain their existing workflow. - **Behaviour change:** local logout retires only the current browser session, using the existing account-cache invalidation channel (#318). Migration diff --git a/docs/modules/users.md b/docs/modules/users.md index ff92a0f74..fd47ae097 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -559,12 +559,15 @@ retain its session cookie. Signing in rotates the token: read the next page's prop before another mutation. Missing or mismatched tokens return 403. Requests with `Origin`, `Referer` or `Sec-Fetch-Site` are treated as browser -requests. Foreign origins and `Sec-Fetch-Site: cross-site` are refused even with -a token; same-site sibling origins still need a token. Origin comparison uses -the request's effective scheme, host and port, so deployments behind a proxy -must configure the framework's trusted proxy handling correctly. The Inertia -`POST /users/logout` uses the origin check without requiring a token header, -so the existing menu submission continues to work. +requests and must carry the session token. `Sec-Fetch-Site: cross-site` is refused +even with a token. `Origin`/`Referer` are not compared with the request's own +origin, so deployments behind a TLS-terminating proxy work without extra +configuration; the token is what defeats login CSRF. A cross-site form always +carries `Origin` in current browsers, so it cannot skip the token; a very old +browser sending none of the three headers would be treated as a native client, +which is why the session cookie stays `SameSite=Lax`. The Inertia +`POST /users/logout` applies only the cross-site refusal and needs no token +header, so the existing menu submission continues to work. Native JSON/form API clients without browser headers continue to work without first fetching an HTML page. If they explicitly send `X-CSRF-Token`, it is diff --git a/modules/users/tests/test_users_browser_auth_boundary.py b/modules/users/tests/test_users_browser_auth_boundary.py index e7f03571b..7dcc345de 100644 --- a/modules/users/tests/test_users_browser_auth_boundary.py +++ b/modules/users/tests/test_users_browser_auth_boundary.py @@ -46,24 +46,33 @@ async def test_anonymous_page_token_allows_login_and_rotates(anon_client): assert (await anon_client.post(LOGIN, data=CREDENTIALS, headers=headers)).status_code == 403 -@pytest.mark.parametrize("header", ["Origin", "Referer"]) -async def test_foreign_source_refused_even_with_token(anon_client, header): +async def test_cross_site_fetch_metadata_refused_even_with_token(anon_client): token = await _login_token(anon_client) response = await anon_client.post( LOGIN, data=CREDENTIALS, - headers={header: "https://other.example", "X-CSRF-Token": token}, + headers={"Sec-Fetch-Site": "cross-site", "X-CSRF-Token": token}, ) assert response.status_code == 403 +async def test_proxied_origin_with_token_is_accepted(anon_client): + token = await _login_token(anon_client) + response = await anon_client.post( + LOGIN, + data=CREDENTIALS, + headers={"Origin": "https://app.example", "X-CSRF-Token": token}, + ) + assert response.status_code == 204 + + async def test_non_browser_password_client_keeps_working(anon_client): assert (await anon_client.post(LOGIN, data=CREDENTIALS)).status_code == 204 async def test_view_logout_accepts_same_origin_inertia_post(anon_client): assert (await anon_client.post(LOGIN, data=CREDENTIALS)).status_code == 204 - refused = await anon_client.post("/users/logout", headers={"Origin": "https://other.example"}) + refused = await anon_client.post("/users/logout", headers={"Sec-Fetch-Site": "cross-site"}) assert refused.status_code == 403 assert ( await anon_client.post("/users/logout", headers={"Origin": "http://testserver"}) diff --git a/modules/users/tests/test_users_browser_origin_validation.py b/modules/users/tests/test_users_browser_origin_validation.py index 2cda71c3d..d2e628c87 100644 --- a/modules/users/tests/test_users_browser_origin_validation.py +++ b/modules/users/tests/test_users_browser_origin_validation.py @@ -1,4 +1,4 @@ -"""Source normalization and token validation at the browser boundary.""" +"""Fetch-metadata refusal and token validation at the browser boundary.""" import pytest from fastapi import HTTPException, Request @@ -10,14 +10,8 @@ ("headers", "allowed"), [ ({"origin": "http://testserver:80"}, True), + ({"origin": "https://app.example"}, True), # proxy: scheme/host differ from base_url ({"referer": "http://testserver/login?next=%2F"}, True), - ({"origin": "https://testserver"}, False), - ({"origin": "http://testserver:8080"}, False), - ({"origin": "http://testserver:0"}, False), - ({"origin": "http://testserver:invalid"}, False), - ({"origin": "http://testserver.evil.example"}, False), - ({"origin": "http://person@testserver"}, False), - ({"origin": "http://[broken"}, False), ({"sec-fetch-site": "cross-site"}, False), ({"sec-fetch-site": "same-site"}, True), ], diff --git a/modules/users/users/auth_local/browser_guard.py b/modules/users/users/auth_local/browser_guard.py index 9e8416af5..83ce51ab3 100644 --- a/modules/users/users/auth_local/browser_guard.py +++ b/modules/users/users/auth_local/browser_guard.py @@ -1,33 +1,22 @@ """Session-bound validation for browser auth mutations.""" -from urllib.parse import urlsplit - from fastapi import HTTPException, Request from simple_module_hosting.csrf import CSRF_HEADER, RequiresCsrf -def _origin(value: str) -> tuple[str, str, int]: - parsed = urlsplit(value) - if parsed.scheme not in {"http", "https"} or not parsed.hostname or parsed.username: - raise ValueError("Invalid origin") - return ( - parsed.scheme, - parsed.hostname, - parsed.port if parsed.port is not None else (443 if parsed.scheme == "https" else 80), - ) - - async def require_browser_origin(request: Request) -> None: - """Refuse browser mutations initiated by a foreign origin.""" + """Refuse browser mutations the browser itself labels ``cross-site``. + + Origin/Referer are deliberately not compared with ``request.base_url``: behind + a TLS-terminating proxy without trusted-proxy handling the app sees + ``http://internal`` while the browser sends ``https://app.example``, which + would refuse every sign-in and sign-out. Login CSRF is stopped by the session + CSRF token (``require_browser_csrf``); this check adds the fetch-metadata + refusal for browsers that send it. + """ if request.method in {"GET", "HEAD", "OPTIONS"}: return - source = request.headers.get("origin") or request.headers.get("referer") - site = request.headers.get("sec-fetch-site") - try: - foreign = source is not None and _origin(source) != _origin(str(request.base_url)) - except ValueError: - foreign = True - if foreign or site == "cross-site": + if request.headers.get("sec-fetch-site") == "cross-site": raise HTTPException(status_code=403, detail="Browser request source refused") diff --git a/modules/users/users/browser_sessions.py b/modules/users/users/browser_sessions.py index 9b1292f03..ad31e9a4e 100644 --- a/modules/users/users/browser_sessions.py +++ b/modules/users/users/browser_sessions.py @@ -6,7 +6,7 @@ from auth.constants import SESSION_ID_KEY from fastapi import Request from simple_module_db.callbacks import register_on_commit -from simple_module_hosting.session import SESSION_EXPIRES_AT_KEY +from simple_module_hosting.session import SESSION_EXPIRES_AT_KEY, SESSION_SIGNATURE_MAX_AGE from sqlalchemy import delete from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import col @@ -24,13 +24,17 @@ async def revoke_browser_session(request: Request, db: AsyncSession) -> None: if not identity or not user_id: return now = datetime.now(UTC) + deadline = session.get(SESSION_EXPIRES_AT_KEY) + if isinstance(deadline, bool) or not isinstance(deadline, (int, float)): + # No usable deadline: hold the record for the longest the signer accepts. + deadline = now.timestamp() + SESSION_SIGNATURE_MAX_AGE await db.execute(delete(RevokedSession).where(col(RevokedSession.expires_at) <= now)) # Merge makes repeated logout requests for the same cookie idempotent. await db.merge( RevokedSession( session_id=identity, user_id=uuid.UUID(user_id), - expires_at=datetime.fromtimestamp(session[SESSION_EXPIRES_AT_KEY], UTC), + expires_at=datetime.fromtimestamp(deadline, UTC), ) ) await db.flush() diff --git a/modules/users/users/provider.py b/modules/users/users/provider.py index 6992b9799..d1cffec9c 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -10,6 +10,7 @@ import logging import uuid as uuid_mod from collections.abc import Mapping +from datetime import UTC, datetime from typing import Any from auth.constants import SESSION_ID_KEY @@ -196,7 +197,7 @@ async def _version_still_current(self, scope, user_id: uuid_mod.UUID, session) - if hit: return cached is not None and int(cached) == _stamped_version(session) try: - from sqlalchemy import select + from sqlalchemy import and_, select from users.models import RevokedSession, User @@ -205,7 +206,13 @@ async def _version_still_current(self, scope, user_id: uuid_mod.UUID, session) - rows = ( await db_session.execute( select(User.session_version, RevokedSession.session_id) - .outerjoin(RevokedSession, col(RevokedSession.user_id) == User.id) + .outerjoin( + RevokedSession, + and_( + col(RevokedSession.user_id) == User.id, + col(RevokedSession.expires_at) > datetime.now(UTC), + ), + ) .where(User.id == user_id) ) ).all() From 9caa85bda8ae36d4a3696b3c0c929805da13a99f Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:42:55 +0100 Subject: [PATCH 4/7] fix: address code review findings (round 1, pass 1b) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- host/tests/test_migration_graph.py | 2 +- ...y => test_users_browser_fetch_metadata.py} | 0 .../test_users_browser_session_logout.py | 36 +++++++++++++++++++ modules/users/users/browser_sessions.py | 28 ++++++++++----- 4 files changed, 57 insertions(+), 9 deletions(-) rename modules/users/tests/{test_users_browser_origin_validation.py => test_users_browser_fetch_metadata.py} (100%) diff --git a/host/tests/test_migration_graph.py b/host/tests/test_migration_graph.py index 0ea737f57..76c6b1915 100644 --- a/host/tests/test_migration_graph.py +++ b/host/tests/test_migration_graph.py @@ -44,7 +44,7 @@ "a625f0b7c912": "users", # refresh-token families (#625) "a626b1c8d023": "users", # hashed bearer tokens (#626) "e731a1b2c3d4": "users", # unique lower(email) (#731) - "e805a1b2c3d4": "users", + "e805a1b2c3d4": "users", # browser session retirement "51b2058d8afd": "api_tokens", "168a2882f443": "keycloak", "f6a3b9d2e8c4": "keycloak", diff --git a/modules/users/tests/test_users_browser_origin_validation.py b/modules/users/tests/test_users_browser_fetch_metadata.py similarity index 100% rename from modules/users/tests/test_users_browser_origin_validation.py rename to modules/users/tests/test_users_browser_fetch_metadata.py diff --git a/modules/users/tests/test_users_browser_session_logout.py b/modules/users/tests/test_users_browser_session_logout.py index aa0d086b0..adf505996 100644 --- a/modules/users/tests/test_users_browser_session_logout.py +++ b/modules/users/tests/test_users_browser_session_logout.py @@ -3,12 +3,16 @@ import json import uuid from datetime import UTC, datetime, timedelta +from types import SimpleNamespace import httpx import pytest from _users_app_builders import user_row +from auth.constants import SESSION_ID_KEY from simple_module_core.invalidation import InvalidationBus from sqlalchemy import select +from users.browser_sessions import revoke_browser_session +from users.constants import SESSION_USER_ID_KEY from users.models import RevokedSession from users.session_revocation import CHANNEL, subscribe from users.session_version_cache import ( @@ -81,6 +85,38 @@ async def test_logout_cleans_expired_records_and_retains_deadline(users_app, ano assert timedelta(days=13) < remaining <= timedelta(days=14) +async def test_second_logout_of_same_session_is_a_noop(users_app, anon_client): + assert (await anon_client.post("/api/users/auth/login", data=CREDENTIALS)).status_code == 204 + copied = anon_client.cookies.get("session") + assert (await anon_client.post("/api/users/auth/logout")).status_code == 204 + async with users_app.state.sm.db.session_factory() as db: + first = list((await db.execute(select(RevokedSession))).scalars()) + assert len(first) == 1 + transport = httpx.ASGITransport(app=users_app) + async with httpx.AsyncClient( + transport=transport, base_url="http://testserver", cookies={"session": copied} + ) as again: + assert (await again.post("/api/users/auth/logout")).status_code in {204, 401, 403} + async with users_app.state.sm.db.session_factory() as db: + rows = list((await db.execute(select(RevokedSession))).scalars()) + assert [(r.session_id, r.expires_at) for r in rows] == [ + (r.session_id, r.expires_at) for r in first + ] + + +async def test_revoke_browser_session_twice_in_one_transaction(users_app): + user = await user_row(users_app, "admin@example.com") + session = {SESSION_ID_KEY: "dup", SESSION_USER_ID_KEY: str(user.id)} + request = SimpleNamespace(session=session, app=users_app) + async with users_app.state.sm.db.session_factory() as db: + await revoke_browser_session(request, db) + await revoke_browser_session(request, db) + await db.commit() + async with users_app.state.sm.db.session_factory() as db: + rows = list((await db.execute(select(RevokedSession))).scalars()) + assert [r.session_id for r in rows] == ["dup"] + + async def test_remote_invalidation_drops_cached_retirements(): user_id = uuid.uuid4() bus = InvalidationBus() diff --git a/modules/users/users/browser_sessions.py b/modules/users/users/browser_sessions.py index ad31e9a4e..5c2180466 100644 --- a/modules/users/users/browser_sessions.py +++ b/modules/users/users/browser_sessions.py @@ -2,12 +2,14 @@ import uuid from datetime import UTC, datetime +from typing import Any from auth.constants import SESSION_ID_KEY from fastapi import Request from simple_module_db.callbacks import register_on_commit from simple_module_hosting.session import SESSION_EXPIRES_AT_KEY, SESSION_SIGNATURE_MAX_AGE from sqlalchemy import delete +from sqlalchemy.dialects import postgresql, sqlite from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import col @@ -17,6 +19,16 @@ from users.token_storage import revoke_access_token +def _insert(dialect: str) -> Any: + if dialect == "postgresql": + return postgresql.insert(RevokedSession) + if dialect == "sqlite": + return sqlite.insert(RevokedSession) + raise NotImplementedError( + f"users_revoked_session supports postgresql and sqlite, not {dialect}" + ) + + async def revoke_browser_session(request: Request, db: AsyncSession) -> None: session = request.session identity = session.get(SESSION_ID_KEY) @@ -29,15 +41,15 @@ async def revoke_browser_session(request: Request, db: AsyncSession) -> None: # No usable deadline: hold the record for the longest the signer accepts. deadline = now.timestamp() + SESSION_SIGNATURE_MAX_AGE await db.execute(delete(RevokedSession).where(col(RevokedSession.expires_at) <= now)) - # Merge makes repeated logout requests for the same cookie idempotent. - await db.merge( - RevokedSession( - session_id=identity, - user_id=uuid.UUID(user_id), - expires_at=datetime.fromtimestamp(deadline, UTC), - ) + # ON CONFLICT DO NOTHING keeps concurrent logouts of one cookie idempotent + # (a SELECT-then-INSERT merge races on the primary key). DML run through the + # session marks it written, so the request session commits. + stmt = _insert(db.get_bind().dialect.name).values( + session_id=identity, + user_id=uuid.UUID(user_id), + expires_at=datetime.fromtimestamp(deadline, UTC), ) - await db.flush() + await db.execute(stmt.on_conflict_do_nothing(index_elements=["session_id"])) register_on_commit(db, lambda: publish_revocation(request.app, uuid.UUID(user_id))) From 9ee2dbbe83ee6cfaefea1de9d5095ae83d6973ce Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:49:42 +0100 Subject: [PATCH 5/7] =?UTF-8?q?fix:=20address=20code=20review=20findings?= =?UTF-8?q?=20(round=201,=20pass=202)=20=E2=80=94=20bearer-token=20routes?= =?UTF-8?q?=20need=20no=20session=20CSRF=20token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/modules/users.md | 6 ++++-- .../users/tests/test_users_browser_auth_boundary.py | 12 ++++++++++++ modules/users/users/module.py | 4 +++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/docs/modules/users.md b/docs/modules/users.md index fd47ae097..01f53566b 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -551,8 +551,10 @@ Anything else without a session redirects to `/users/login` (or returns 401 JSON **Behaviour change:** anonymous auth pages issue a session-bound `csrfToken` prop. Browser mutations under `/api/users/auth/` (password sign-in, demo sign-in, -registration, forgot/reset password, verification, invite acceptance, bearer token -operations and API logout) must echo it as `X-CSRF-Token`. The shared typed API +registration, forgot/reset password, verification, invite acceptance and API +logout) must echo it as `X-CSRF-Token`. The bearer-token routes +(`/api/users/auth/token*`) set no cookie and need no token, so cross-origin +API clients keep working. The shared typed API client already does this, including sign-in after a password reset. Read the prop from `GET /users/login` through Inertia when building another browser client; retain its session cookie. Signing in rotates the token: read the next page's diff --git a/modules/users/tests/test_users_browser_auth_boundary.py b/modules/users/tests/test_users_browser_auth_boundary.py index 7dcc345de..1d38dd095 100644 --- a/modules/users/tests/test_users_browser_auth_boundary.py +++ b/modules/users/tests/test_users_browser_auth_boundary.py @@ -94,3 +94,15 @@ async def test_anonymous_auth_pages_supply_token(anon_client_signup, path): response = await anon_client_signup.get(path, headers=_INERTIA) assert response.status_code == 200 assert response.json()["props"]["csrfToken"] + + +async def test_bearer_token_login_needs_no_session_token(anon_client): + """A cross-origin API client gets tokens in the body; no cookie, no CSRF.""" + response = await anon_client.post( + "/api/users/auth/token", + headers={"Origin": "https://spa.example", "Sec-Fetch-Site": "cross-site"}, + json={"email": CREDENTIALS["username"], "password": CREDENTIALS["password"]}, + ) + assert response.status_code == 200, response.text + assert response.json()["access_token"] + assert "set-cookie" not in response.headers diff --git a/modules/users/users/module.py b/modules/users/users/module.py index a03d58b74..c19e23de3 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -226,7 +226,9 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None api_router.include_router(auth_local_api.router) api_router.include_router(demo_router, dependencies=[Depends(require_browser_csrf)]) - api_router.include_router(token_router, dependencies=[Depends(require_browser_csrf)]) + # Bearer-token routes set no cookie and touch no session, so a + # cross-origin client (CORS SPA, mobile) needs no session CSRF token. + api_router.include_router(token_router) api_router.include_router(admin_router) csrf = Depends(require_browser_csrf) throughput = Depends(auth_local_api.enforce_auth_throughput_limit) From efa59a953139b1a96a5f8f366fdfab7bdbd95777 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:49:53 +0100 Subject: [PATCH 6/7] docs(changelog): spacing after merge Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f2b7165b..e48b5d092 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ All notable changes to this project are documented in this file. The format is b using the existing account-cache invalidation channel (#318). Migration `e805a1b2c3d4` adds expiring session retirement records. Sessions issued before the upgrade remain compatible until expiry or next sign-in. + ### Changed - Scaffold new modules with `core/` and `web/` areas and mirrored nested Python tests; existing modules migrate individually. See `docs/module-layout.md`. From 34f5ca9c9903abe4cdc0da0c5b84e2d3da1e22f8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 16:59:31 +0100 Subject: [PATCH 7/7] fix(users): move browser session teardown under auth_local; framework CSRF test probes a shared-prop page Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/modules/users.md | 4 ++-- .../hosting/tests/test_hosting_csrf_shared_prop.py | 11 +++++------ .../users/tests/test_users_browser_session_logout.py | 2 +- modules/users/users/auth_local/api.py | 2 +- .../users/users/{ => auth_local}/browser_sessions.py | 0 modules/users/users/auth_local/views.py | 2 +- 6 files changed, 10 insertions(+), 11 deletions(-) rename modules/users/users/{ => auth_local}/browser_sessions.py (100%) diff --git a/docs/modules/users.md b/docs/modules/users.md index 01f53566b..360f187d1 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -553,8 +553,8 @@ Anything else without a session redirects to `/users/login` (or returns 401 JSON Browser mutations under `/api/users/auth/` (password sign-in, demo sign-in, registration, forgot/reset password, verification, invite acceptance and API logout) must echo it as `X-CSRF-Token`. The bearer-token routes -(`/api/users/auth/token*`) set no cookie and need no token, so cross-origin -API clients keep working. The shared typed API +(`/api/users/auth/token*`) set no cookie and need no token, so native clients +and cross-origin clients allowed by `cors_allow_origins` can use them. The shared typed API client already does this, including sign-in after a password reset. Read the prop from `GET /users/login` through Inertia when building another browser client; retain its session cookie. Signing in rotates the token: read the next page's diff --git a/framework/hosting/tests/test_hosting_csrf_shared_prop.py b/framework/hosting/tests/test_hosting_csrf_shared_prop.py index 430b4cdde..c9eca4a66 100644 --- a/framework/hosting/tests/test_hosting_csrf_shared_prop.py +++ b/framework/hosting/tests/test_hosting_csrf_shared_prop.py @@ -47,13 +47,12 @@ async def _probe() -> dict[str, bool]: async def test_anonymous_page_has_no_token(client) -> None: - resp = await client.get("/users/login", headers=_INERTIA) - assert resp.status_code == 200, resp.text + # The shared prop never mints a token for anonymous pages; the error page + # renders through the same shared-prop pipeline. (Auth pages such as + # /users/login pass their own token deliberately, for the sign-in post.) + resp = await client.get("/no-such-page", headers=_INERTIA) + assert resp.json()["component"] == "Error", resp.text assert "csrfToken" not in resp.json()["props"] - # The full document of a page a module may mark cacheable stays token-free too. - doc = await client.get("/users/login", headers={"Accept": "text/html"}) - assert doc.status_code == 200 - assert "csrfToken" not in doc.text def test_helper_needs_a_signed_in_session() -> None: diff --git a/modules/users/tests/test_users_browser_session_logout.py b/modules/users/tests/test_users_browser_session_logout.py index adf505996..1a6394ede 100644 --- a/modules/users/tests/test_users_browser_session_logout.py +++ b/modules/users/tests/test_users_browser_session_logout.py @@ -11,7 +11,7 @@ from auth.constants import SESSION_ID_KEY from simple_module_core.invalidation import InvalidationBus from sqlalchemy import select -from users.browser_sessions import revoke_browser_session +from users.auth_local.browser_sessions import revoke_browser_session from users.constants import SESSION_USER_ID_KEY from users.models import RevokedSession from users.session_revocation import CHANNEL, subscribe diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index f3ac7576e..f1aeac67d 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -24,6 +24,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from users.auth_local.browser_guard import require_browser_csrf +from users.auth_local.browser_sessions import end_browser_session from users.auth_local.lockout import ( admit, login_attempt, @@ -34,7 +35,6 @@ from users.auth_local.rate_limit import enforce_auth_throughput_limit from users.auth_local.self_account import router as self_account_router from users.backend import cookie_login_response -from users.browser_sessions import end_browser_session from users.constants import SESSION_USER_ID_KEY from users.contracts.schemas import ( AcceptInviteRequest, diff --git a/modules/users/users/browser_sessions.py b/modules/users/users/auth_local/browser_sessions.py similarity index 100% rename from modules/users/users/browser_sessions.py rename to modules/users/users/auth_local/browser_sessions.py diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index e5ee39acd..53de991dc 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -14,9 +14,9 @@ from starlette.responses import RedirectResponse from users.auth_local.browser_guard import require_browser_origin +from users.auth_local.browser_sessions import end_browser_session from users.auth_local.token_preview import decode_verify_token, preview_reset from users.bootstrap import resolve_bootstrap_credentials -from users.browser_sessions import end_browser_session from users.contracts.schemas import UserRead from users.demo import resolve_demo_accounts from users.invites.preview import preview_invite