diff --git a/CHANGELOG.md b/CHANGELOG.md index bebdbd9f8b..fc73d7c179 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to ### Changed +- ⚡️(yhub) skip the `users/me` call for callers without a session cookie - 🔧(helm) allow to set the period and failure threshold of probes ### Fixed @@ -25,6 +26,12 @@ and this project adheres to - ♿️(frontend) fix the accessibility of the document tree #2769 +### Removed + +- 🔥(backend) remove `ForceSessionMiddleware`, an orphan of the removed + non-websocket save safeguard + + ## [v6.0.0] - 2026-10-05 ### Added diff --git a/documentation/load-testing.md b/documentation/load-testing.md index 3043d8868b..1b006e9076 100644 --- a/documentation/load-testing.md +++ b/documentation/load-testing.md @@ -118,7 +118,13 @@ open one socket with a cookie of the manifest and read its logs for the store makes the clients anonymous, which looks like success on public documents. (In this repository's compose stack, the `Development` configuration keeps its sessions in another Redis database than `LoadTest`: -mint with `REDIS_URL=redis://redis:6379/2` there.) +mint with `REDIS_URL=redis://redis:6379/2` there.) One thing changed with +the session-cookie fast path: a client holding no session cookie at all is +resolved to anonymous by the collaboration server without any call. A +manifest's clients always send the session cookie, so a bad manifest still +costs one `users_me` call per client and still shows up as `users_me:401` +in `yhub_backend_request_duration_seconds` — that signal stands; the +`userid` in the collaboration server's logs tells the rest. ## Where the load comes from diff --git a/src/backend/core/middleware.py b/src/backend/core/middleware.py index c4720426e4..e9cb5be5d8 100644 --- a/src/backend/core/middleware.py +++ b/src/backend/core/middleware.py @@ -1,4 +1,4 @@ -"""Custom middlewares of the impress core application.""" +"""Custom middlewares of the impress core app.""" from secrets import compare_digest @@ -11,33 +11,6 @@ # loaded, does not import prometheus_client when the metrics are disabled. METRICS_PATH = "/metrics" -# Paths that must never touch the session store. Liveness and readiness -# probes should never create a new session in redis. Also, the liveness probe -# should never reach redis before returning its answer. A scraper carries no -# cookie either: each of its calls would leave one more session behind. -SESSION_EXEMPT_PATHS = ( - "/__lbheartbeat__", - "/__heartbeat__", - METRICS_PATH, -) - - -class ForceSessionMiddleware(MiddlewareMixin): - """ - Force session creation for unauthenticated users. - Must be used after Authentication middleware. - """ - - def process_request(self, request): - """Force session creation for unauthenticated users.""" - # Check the path before touching `request.user`: evaluating it loads - # the session from the cache backend. - if request.path.rstrip("/") in SESSION_EXEMPT_PATHS: - return - - if not request.user.is_authenticated and request.session.session_key is None: - request.session.create() - class SaveRawBodyMiddleware(MiddlewareMixin): """ diff --git a/src/backend/core/tests/test_middleware.py b/src/backend/core/tests/test_middleware.py deleted file mode 100644 index 6ee3e7c9d9..0000000000 --- a/src/backend/core/tests/test_middleware.py +++ /dev/null @@ -1,114 +0,0 @@ -""" -Test the ForceSessionMiddleware of the Impress core app. -""" - -from unittest.mock import patch - -from django.contrib.auth.models import AnonymousUser -from django.contrib.sessions.backends.cache import SessionStore -from django.test import RequestFactory - -import pytest -from rest_framework.test import APIClient - -from core import factories -from core.middleware import ForceSessionMiddleware - -pytestmark = pytest.mark.django_db - - -def _process(path, user): - """Run the middleware on a request for `path` and return the request.""" - request = RequestFactory().get(path) - request.user = user - request.session = SessionStore() - ForceSessionMiddleware(lambda req: None).process_request(request) - return request - - -def test_middleware_force_session_anonymous_creates_session(): - """An anonymous request on a regular path gets a session created.""" - request = _process("/api/v1.0/config/", AnonymousUser()) - assert request.session.session_key is not None - - -def test_middleware_force_session_authenticated_does_not_create_session(): - """An authenticated request does not get a new session forced on it.""" - request = _process("/api/v1.0/config/", factories.UserFactory()) - assert request.session.session_key is None - - -@pytest.mark.parametrize("path", ["/__lbheartbeat__", "/__lbheartbeat__/"]) -def test_middleware_force_session_liveness_probe_skips_session(path): - """ - The liveness probe must never create a session nor load one: it has to keep - answering when the session backend is unavailable. - """ - with ( - patch.object(SessionStore, "create") as mock_create, - patch.object(SessionStore, "load") as mock_load, - ): - request = _process(path, AnonymousUser()) - - mock_create.assert_not_called() - mock_load.assert_not_called() - assert request.session.session_key is None - - -def test_middleware_force_session_readiness_probe_end_to_end(): - """A readiness probe request answers 200 without touching the session store.""" - client = APIClient() - - with ( - patch.object(SessionStore, "create") as mock_create, - patch.object(SessionStore, "load") as mock_load, - ): - response = client.get("/__heartbeat__") - - assert response.status_code == 200 - mock_create.assert_not_called() - mock_load.assert_not_called() - assert "docs_sessionid" not in response.cookies - - -@pytest.mark.parametrize("path", ["/__heartbeat__", "/__heartbeat__/"]) -def test_middleware_force_session_readiness_probe_skips_session(path): - """ - The readiness probe must never create a session nor load one: it has to keep - answering when the session backend is unavailable. - """ - with ( - patch.object(SessionStore, "create") as mock_create, - patch.object(SessionStore, "load") as mock_load, - ): - request = _process(path, AnonymousUser()) - - mock_create.assert_not_called() - mock_load.assert_not_called() - assert request.session.session_key is None - - -def test_middleware_force_session_liveness_probe_end_to_end(): - """A liveness probe request answers 200 without touching the session store.""" - client = APIClient() - - with ( - patch.object(SessionStore, "create") as mock_create, - patch.object(SessionStore, "load") as mock_load, - ): - response = client.get("/__lbheartbeat__") - - assert response.status_code == 200 - mock_create.assert_not_called() - mock_load.assert_not_called() - assert "docs_sessionid" not in response.cookies - - -def test_middleware_force_session_anonymous_end_to_end_still_gets_cookie(): - """A regular anonymous request still receives a session cookie.""" - client = APIClient() - - response = client.get("/api/v1.0/config/") - - assert response.status_code == 200 - assert "docs_sessionid" in response.cookies diff --git a/src/backend/core/tests/test_session_invariants.py b/src/backend/core/tests/test_session_invariants.py new file mode 100644 index 0000000000..7ab67bd8e8 --- /dev/null +++ b/src/backend/core/tests/test_session_invariants.py @@ -0,0 +1,32 @@ +""" +Test the session-related invariants of the Impress core app. + +`ForceSessionMiddleware` used to create a session for every anonymous request, +which forced the probes to be exempted (commits 1e61b4a78 and 0b8808f9a). The +middleware is gone: pin the invariant that probe requests create no session and +receive no session cookie. +""" + +from unittest.mock import patch + +from django.contrib.sessions.backends.cache import SessionStore + +import pytest +from rest_framework.test import APIClient + +pytestmark = pytest.mark.django_db + + +@pytest.mark.parametrize("path", ["/__heartbeat__", "/__lbheartbeat__"]) +def test_probes_get_no_session_nor_cookie(path): + """Probe requests answer 200 without touching the session store.""" + with ( + patch.object(SessionStore, "create") as mock_create, + patch.object(SessionStore, "load") as mock_load, + ): + response = APIClient().get(path) + + assert response.status_code == 200 + mock_create.assert_not_called() + mock_load.assert_not_called() + assert "docs_sessionid" not in response.cookies diff --git a/src/backend/core/tests/test_settings.py b/src/backend/core/tests/test_settings.py index 19be8a6e9e..a5f29637da 100644 --- a/src/backend/core/tests/test_settings.py +++ b/src/backend/core/tests/test_settings.py @@ -5,11 +5,26 @@ import os import tempfile +from django.conf import settings + import pytest from impress.settings import Base, Production +def test_session_cookie_name_matches_yhub_contract(): + """ + The yhub collaboration server (`src/yhub-server/src/config.ts`) hardcodes + the same session cookie name to decide whether a caller may hold a session + at all: a caller whose cookies carry no `docs_sessionid` is resolved to + anonymous there without calling `users/me`. Renaming the cookie on the + backend side alone would silently degrade every signed-in user to + anonymous, so the contract is pinned on both ends. + """ + + assert settings.SESSION_COOKIE_NAME == "docs_sessionid" + + def test_invalid_settings_oidc_email_configuration(): """ The OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION and OIDC_ALLOW_DUPLICATE_EMAILS settings diff --git a/src/backend/impress/settings.py b/src/backend/impress/settings.py index f799b42c85..3d52c3519b 100755 --- a/src/backend/impress/settings.py +++ b/src/backend/impress/settings.py @@ -374,7 +374,6 @@ class Base(Configuration): "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", - "core.middleware.ForceSessionMiddleware", "core.middleware.SaveRawBodyMiddleware", "django.contrib.messages.middleware.MessageMiddleware", "csp.middleware.CSPMiddleware", diff --git a/src/yhub-server/README.md b/src/yhub-server/README.md index a746c19b79..a4c8c9bf39 100644 --- a/src/yhub-server/README.md +++ b/src/yhub-server/README.md @@ -30,7 +30,9 @@ It is not a fork of yhub — it is a thin wrapper, written in TypeScript under below), - plugs in an auth plugin that resolves users and per-document access rights by calling the Docs Django backend (`/api/v1.0/users/me/` and - `/api/v1.0/documents/{id}/`), + `/api/v1.0/documents/{id}/`; the user call is skipped for any caller whose + `Cookie` header does not carry the backend's `docs_sessionid` cookie — + they can only be anonymous), - serves every route under the `/collaboration/` prefix (`server.apiPrefix`), including the websocket sync route `/collaboration/ws/v1/{org}/{docid}`, @@ -201,6 +203,13 @@ yhub refuses the upgrade of a caller that holds the write but has no identity, because attributions carry the userid. Every anonymous edit is therefore attributed to that one shared author. +A caller whose `Cookie` header does not carry the backend's session cookie +(`docs_sessionid`) is anonymous without a roundtrip: the backend only ever +speaks sessions through that one cookie, so the `users/me` call could only +answer 401 for them, and the identity is resolved locally instead. Browsers +still send their other cookies (csrf, consent, …) on that header, which is why +the check is on the session cookie rather than on the header being present. + ### Origins and cors `COLLABORATION_SERVER_ORIGIN` is the list of origins a browser may reach this diff --git a/src/yhub-server/__tests__/backend.spec.ts b/src/yhub-server/__tests__/backend.spec.ts index 838f1826e3..c88f638064 100644 --- a/src/yhub-server/__tests__/backend.spec.ts +++ b/src/yhub-server/__tests__/backend.spec.ts @@ -172,6 +172,55 @@ describe('backendFetch', () => { }); }); +describe('carriesSessionCookie', () => { + // imported lazily per test through `load()` like the other describes, so it + // runs after the file-level beforeEach has stubbed the env: a beforeAll on + // the describe would import config.ts before the first beforeEach runs, and + // only pass while every config read keeps a default + + it('accepts a header carrying only the session cookie', async () => { + const { carriesSessionCookie } = await load(); + expect(carriesSessionCookie('docs_sessionid=abc')).toBe(true); + }); + + it('accepts the session cookie among others', async () => { + const { carriesSessionCookie } = await load(); + expect( + carriesSessionCookie('csrftoken=x; docs_sessionid=abc; consent=y'), + ).toBe(true); + }); + + it('accepts it despite surrounding whitespace around the pair', async () => { + // browsers separate cookies with "; " and may pad: the name is trimmed, + // though it stays case-sensitive — RFC 6265 cookie names are + const { carriesSessionCookie } = await load(); + expect(carriesSessionCookie('csrftoken=x; docs_sessionid = abc')).toBe( + true, + ); + }); + + it('is case-sensitive on the cookie name, as RFC 6265 makes them', async () => { + const { carriesSessionCookie } = await load(); + expect(carriesSessionCookie('DOCS_SESSIONID=abc')).toBe(false); + }); + + it('refuses a header with other cookies but no session one', async () => { + const { carriesSessionCookie } = await load(); + expect(carriesSessionCookie('csrftoken=x; consent=y')).toBe(false); + }); + + it('refuses lookalike names that are not the session cookie', async () => { + const { carriesSessionCookie } = await load(); + expect(carriesSessionCookie('xdocs_sessionid=abc')).toBe(false); + expect(carriesSessionCookie('docs_sessionid_extra=abc')).toBe(false); + }); + + it('refuses an empty header', async () => { + const { carriesSessionCookie } = await load(); + expect(carriesSessionCookie('')).toBe(false); + }); +}); + describe('the backend signing key', () => { it('publishes no JWK when YHUB_JWT_PRIVATE_KEY is unset', async () => { const { backendPublicJwk } = await load(); diff --git a/src/yhub-server/__tests__/server.spec.ts b/src/yhub-server/__tests__/server.spec.ts index 20c0d21661..d5e8a9d8ff 100644 --- a/src/yhub-server/__tests__/server.spec.ts +++ b/src/yhub-server/__tests__/server.spec.ts @@ -204,3 +204,63 @@ describe('the S3 persistence plugin', () => { await expect(boot()).rejects.toThrow(/must be http:\/\/ or https:\/\//); }); }); + +describe('the authenticate cookie gate', () => { + // backend.ts is NOT mocked here: the spy on `fetch` observes the real + // `backendFetch`, so these pin what the auth plugin wires together — who + // is resolved locally, and who costs a backend call. Each test re-boots + // through boot(), after the file-level beforeEach has stubbed BASE_ENV. + const request = ( + headers: Record, + url = '/collaboration/ydoc/v1/docs/00000000-0000-4000-8000-000000000000/', + ) => ({ + getHeader: (name: string) => headers[name] ?? '', + getUrl: () => url, + }); + + const bootGate = async (): Promise<{ + authenticate: (req: ReturnType) => Promise; + fetchMock: ReturnType; + }> => { + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + await boot(); + expect(createYHub.lastConfig.server).not.toBeNull(); + return { + authenticate: createYHub.lastConfig.server.auth.authenticate, + fetchMock, + }; + }; + + it('resolves a caller holding other cookies but no session one to anonymous, without calling the backend', async () => { + const { authenticate, fetchMock } = await bootGate(); + + const user = await authenticate( + request({ cookie: 'csrftoken=x; consent=y' }), + ); + + expect(user).toMatchObject({ userid: 'anonymous' }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('still asks the backend who a caller carrying the session cookie is', async () => { + const { authenticate, fetchMock } = await bootGate(); + fetchMock.mockResolvedValue({ + ok: true, + status: 200, + json: async () => ({ id: 'user-1' }), + }); + + const user = await authenticate( + request({ cookie: 'csrftoken=x; docs_sessionid=abc' }), + ); + + expect(user).toMatchObject({ userid: 'user-1' }); + expect(fetchMock.mock.calls[0][0]).toBe( + 'http://app-dev:8000/api/v1.0/users/me/', + ); + expect(fetchMock.mock.calls[0][1].headers).toMatchObject({ + cookie: 'csrftoken=x; docs_sessionid=abc', + }); + }); +}); diff --git a/src/yhub-server/src/backend.ts b/src/yhub-server/src/backend.ts index 2179f0a315..9a0639aac6 100644 --- a/src/yhub-server/src/backend.ts +++ b/src/yhub-server/src/backend.ts @@ -33,6 +33,7 @@ import { BACKEND_TOKEN_LIFETIME_S, BACKEND_TOKEN_MARGIN_MS, COLLABORATION_BACKEND_BASE_URL, + SESSION_COOKIE_NAME, Y_PROVIDER_API_KEY, YHUB_JWT_PRIVATE_KEY, } from './config.js'; @@ -47,6 +48,16 @@ export interface BackendDocument { abilities?: DocumentAbilities; } +// Whether a caller's `Cookie` header carries the backend's session cookie. +// The backend only ever speaks sessions through that one cookie, so a header +// carrying others (csrf, consent, …) but not this one can only resolve to the +// anonymous identity — the `users/me` call would answer 401 — and +// `authenticate` answers it as such without the roundtrip. +export const carriesSessionCookie = (cookieHeader: string): boolean => + cookieHeader + .split(';') + .some((pair) => pair.split('=')[0].trim() === SESSION_COOKIE_NAME); + const touchLog = logger.child({ module: 'updated-at-notifier' }); const backendSigningKey = YHUB_JWT_PRIVATE_KEY diff --git a/src/yhub-server/src/config.ts b/src/yhub-server/src/config.ts index eb706c70d1..6867bf6299 100644 --- a/src/yhub-server/src/config.ts +++ b/src/yhub-server/src/config.ts @@ -58,6 +58,15 @@ export const MIN_MESSAGE_LIFETIME_MS = intEnv( ); export const COLLABORATION_BACKEND_BASE_URL = process.env.COLLABORATION_BACKEND_BASE_URL || 'http://app-dev:8000'; +// The name of the backend's session cookie (`SESSION_COOKIE_NAME` on the +// Django side, `docs_sessionid` there too). It decides whether +// `authenticate` has to ask the backend who a caller is at all: the backend +// only ever speaks sessions through that one cookie, so a `Cookie` header +// carrying others (csrf, consent, …) but not this one can only resolve to the +// anonymous identity — the `users/me` call would answer 401 — and is answered +// anonymously without the roundtrip. Not read from the environment: a +// two-party contract, hardcoded like the audiences. +export const SESSION_COOKIE_NAME = 'docs_sessionid'; export const allowedOrigins = ( process.env.COLLABORATION_SERVER_ORIGIN || 'http://localhost:3000' ).split(','); diff --git a/src/yhub-server/src/server.ts b/src/yhub-server/src/server.ts index 587bc6bda8..90adf7d748 100644 --- a/src/yhub-server/src/server.ts +++ b/src/yhub-server/src/server.ts @@ -12,7 +12,12 @@ import { jwtVerify } from 'jose'; import { api } from './api.js'; // outbound calls to the Docs Django backend, and the keys both ends verify with import type { BackendDocument, BackendUser } from './backend.js'; -import { JWKS, backendFetch, touchDocument } from './backend.js'; +import { + JWKS, + backendFetch, + carriesSessionCookie, + touchDocument, +} from './backend.js'; import { ANONYMOUS_USERID, API_PREFIX, @@ -192,9 +197,16 @@ const auth = createAuthPlugin({ // `Origin` header at all, so on a deployment where the page and /collaboration/ share a // host every round would 401 while the PATCH beside it succeeded. // - // A caller with no cookie at all is anonymous too: the probes arrive that way, and so does - // a public document opened in a fresh browser. What they may do is `authorize`'s answer. - if (!cookie) { + // The backend only ever speaks sessions through its one session cookie + // (`docs_sessionid`): a caller whose `Cookie` header does not carry it can + // only resolve to the anonymous identity, and is answered as such without + // the `users/me` roundtrip. This is not just the cookie-less case — the + // probes and a public document in a fresh browser — but every signed-out + // tab: since the backend stopped forcing a session onto anonymous users, + // they hold no `docs_sessionid` at all, and browsers still send their other + // cookies (csrf, consent, …) on this header. What an anonymous caller may + // do is `authorize`'s answer, as before. + if (!carriesSessionCookie(cookie)) { return { userid: ANONYMOUS_USERID, origin, endpoint }; } try {