diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ae96e8e7..52998b4aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,13 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Security +- **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 + `e805a1b2c3d4` adds expiring session retirement records. Sessions issued before + the upgrade remain compatible until expiry or next sign-in. - **Audit hardening:** audit entries no longer record credential values. Redacted fields retain change indicators using `[redacted]`; credential primary-key identities are masked too. Migration `b8d4f2a6c901` redacts diff --git a/docs/database/migrations.md b/docs/database/migrations.md index 8eb81d8e6..1cd95f353 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -71,8 +71,8 @@ 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` | -| `a7c2e91d4b58` | audit_log | `audit_log` | +| `e805a1b2c3d4` | users (browser session retirement) | `api_tokens`, `background_tasks`, `file_storage`, `users` | +| `b8d4f2a6c901` | audit_log | `audit_log` | | `f1a9c3d7b2e4` | tenants | `tenants` | | `d263ea727663` | framework (`sm_lease`) | `leases` | | `f6a3b9d2e8c4` | keycloak | `keycloak` | diff --git a/docs/modules/users.md b/docs/modules/users.md index 19f4304a8..360f187d1 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,63 @@ 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 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 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 +prop before another mutation. Missing or mismatched tokens return 403. + +Requests with `Origin`, `Referer` or `Sec-Fetch-Site` are treated as browser +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 +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/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/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 fc8e9e4fc..454001765 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", # browser session retirement "51b2058d8afd": "api_tokens", "168a2882f443": "keycloak", "f6a3b9d2e8c4": "keycloak", @@ -70,7 +71,7 @@ # head revision -> module owning the tables it creates or alters. _HEADS = { "b8d4f2a6c901": "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", @@ -118,6 +119,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..f356c0bdf 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(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_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..1d38dd095 --- /dev/null +++ b/modules/users/tests/test_users_browser_auth_boundary.py @@ -0,0 +1,108 @@ +"""Browser auth mutations use the anonymous session's token.""" + +import pytest + +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( + "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): + 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 + + +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={"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={"Sec-Fetch-Site": "cross-site"}) + 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=_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/tests/test_users_browser_fetch_metadata.py b/modules/users/tests/test_users_browser_fetch_metadata.py new file mode 100644 index 000000000..d2e628c87 --- /dev/null +++ b/modules/users/tests/test_users_browser_fetch_metadata.py @@ -0,0 +1,44 @@ +"""Fetch-metadata refusal 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), + ({"origin": "https://app.example"}, True), # proxy: scheme/host differ from base_url + ({"referer": "http://testserver/login?next=%2F"}, True), + ({"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..1a6394ede --- /dev/null +++ b/modules/users/tests/test_users_browser_session_logout.py @@ -0,0 +1,149 @@ +"""Logout ends one browser session, including an earlier copy of its cookie.""" + +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.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 +from users.session_version_cache import ( + clear_session_version_cache, + read_session_version, + store_session_version, +) + +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 + # 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 + + +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): + 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_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() + 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): + 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..f1aeac67d 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -23,6 +23,8 @@ 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.browser_sessions import end_browser_session from users.auth_local.lockout import ( admit, login_attempt, @@ -49,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() @@ -92,7 +93,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,12 +167,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.""" - 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 @@ -183,7 +182,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..83ce51ab3 --- /dev/null +++ b/modules/users/users/auth_local/browser_guard.py @@ -0,0 +1,29 @@ +"""Session-bound validation for browser auth mutations.""" + +from fastapi import HTTPException, Request +from simple_module_hosting.csrf import CSRF_HEADER, RequiresCsrf + + +async def require_browser_origin(request: Request) -> None: + """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 + if request.headers.get("sec-fetch-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", CSRF_HEADER) + ): + await RequiresCsrf()(request) diff --git a/modules/users/users/auth_local/browser_sessions.py b/modules/users/users/auth_local/browser_sessions.py new file mode 100644 index 000000000..5c2180466 --- /dev/null +++ b/modules/users/users/auth_local/browser_sessions.py @@ -0,0 +1,62 @@ +"""Retire the current browser session and refresh the account's cached state.""" + +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 + +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 + + +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) + user_id = session.get(SESSION_USER_ID_KEY) + 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)) + # 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.execute(stmt.on_conflict_do_nothing(index_elements=["session_id"])) + 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/auth_local/views.py b/modules/users/users/auth_local/views.py index 5112602db..53de991dc 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -7,11 +7,14 @@ 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.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.contracts.schemas import UserRead @@ -21,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() @@ -74,6 +76,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,13 +122,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.""" - 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) @@ -141,7 +142,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 +156,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 +183,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 +209,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 +221,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 +231,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/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..c19e23de3 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,30 +225,35 @@ 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(demo_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) # 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(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(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=[ + 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 85fec660e..d1cffec9c 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -10,8 +10,10 @@ 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 from auth.contracts.schemas import UserContext from auth.contracts.tokens import is_api_token, parse_bearer from simple_module_hosting.session import ( @@ -59,6 +61,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 +132,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 +192,32 @@ 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_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: - from sqlalchemy import select + from sqlalchemy import and_, 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, + and_( + col(RevokedSession.user_id) == User.id, + col(RevokedSession.expires_at) > datetime.now(UTC), + ), + ) + .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 +225,10 @@ 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_id not in revoked def get_login_url(self, request: Request | None, next_url: str | None = None) -> str: return "/users/login" @@ -261,8 +272,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 29708023d..b63819b03 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,17 @@ 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 + version, retired = cached + if session_id in retired: + return True, None + return True, version -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: