Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion documentation/load-testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
29 changes: 1 addition & 28 deletions src/backend/core/middleware.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Custom middlewares of the impress core application."""
"""Custom middlewares of the impress core app."""

from secrets import compare_digest

Expand All @@ -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):
"""
Expand Down
114 changes: 0 additions & 114 deletions src/backend/core/tests/test_middleware.py

This file was deleted.

32 changes: 32 additions & 0 deletions src/backend/core/tests/test_session_invariants.py
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions src/backend/core/tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/backend/impress/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 10 additions & 1 deletion src/yhub-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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}`,
Expand Down Expand Up @@ -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
Expand Down
49 changes: 49 additions & 0 deletions src/yhub-server/__tests__/backend.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading
Loading