From 163adbe40e41d21faa20bd84a98968ba97a2547d Mon Sep 17 00:00:00 2001 From: Mike Mulligan Date: Thu, 8 Oct 2026 16:47:14 -0400 Subject: [PATCH 01/14] chore(security): migrate JWT handling from unmaintained python-jose to PyJWT python-jose has had no release since 3.5.0 (May 2025) and its maintainer has not responded to the open advisory GHSA-3qf3-8w2g-rqmx (CVE-2026-85394: HS256 algorithm confusion when a DER-encoded public key is used as the HMAC secret). Nojoin is not exposed to it: tokens are HS256 only, every keyring entry is a random symmetric secret (or SECRET_KEY), decode pins algorithms=["HS256"] and an unknown kid is refused. The motivation is maintenance and dependency hygiene: a security-critical library that no longer ships fixes, and an unfixable CRITICAL that dependency scans keep reporting. Decision record (token handling is a trust boundary): - Context: the JWT surface is small (encode with a kid header, read the unverified header for the keyring lookup, decode with a pinned algorithm), and PyJWT is already installed in the api image as a dependency of mcp. - Alternatives: keep python-jose (no upstream fixes, permanent scan noise); joserfc/Authlib (a new dependency for the same three calls). - Trade-offs: PyJWT 2.10+ validates iat (a future iat is refused, no leeway: the api process both issues and verifies every token), requires sub and jti to be strings (both always are), parses the header strictly (a non-string kid used to raise TypeError in the keyring lookup and surface as a 500; it is now a 401), and warns, without refusing, on HS256 keys shorter than 32 bytes. Generated keyring keys are 32 random bytes; a short SECRET_KEY keeps working so no deployment is locked out, and docs/SECURITY.md now says how long it should be. Tokens issued by python-jose verify unchanged, so the upgrade forces no sign-out: the new tests carry session, API and MCP tokens minted by python-jose 3.5.0 through the replaced code. deps.py catches jwt.PyJWTError, PyJWT's base class, which like python-jose's JWTError covers every decode failure, so every rejection stays a 401. requirements/base.txt drops python-jose[cryptography] (and with it ecdsa and rsa) for PyJWT==2.15.1, and pins cryptography==50.0.2: encryption.py imports it directly, but it was only ever installed transitively, declared by the python-jose extra. --- backend/api/deps.py | 4 +- backend/api/services/oauth_service.py | 4 +- backend/core/security.py | 18 +- backend/tests/test_jwt_keyring.py | 5 +- backend/tests/test_jwt_verification.py | 352 +++++++++++++++++++++++++ docs/SECURITY.md | 3 +- requirements/base.txt | 6 +- 7 files changed, 372 insertions(+), 20 deletions(-) create mode 100644 backend/tests/test_jwt_verification.py diff --git a/backend/api/deps.py b/backend/api/deps.py index c20621d4f..4b60f2417 100644 --- a/backend/api/deps.py +++ b/backend/api/deps.py @@ -1,9 +1,9 @@ from typing import Any, AsyncGenerator, Optional from urllib.parse import urlparse +import jwt from fastapi import Depends, HTTPException, Request, WebSocket, status from fastapi.security import OAuth2PasswordBearer -from jose import JWTError from pydantic import ValidationError from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import select @@ -178,7 +178,7 @@ async def get_authenticated_token_details( status_code=status.HTTP_403_FORBIDDEN, detail="Token does not have the required scope", ) - except (JWTError, ValidationError): + except (jwt.PyJWTError, ValidationError): raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", diff --git a/backend/api/services/oauth_service.py b/backend/api/services/oauth_service.py index d321f08cf..27e7b2c51 100644 --- a/backend/api/services/oauth_service.py +++ b/backend/api/services/oauth_service.py @@ -304,8 +304,8 @@ def _build_access_token( token_version=user.token_version, extra_claims={ "client_id": client_id, - # Custom claim (not "aud": python-jose rejects tokens carrying - # "aud" unless every decode call passes an audience option). + # Custom claim (not "aud": PyJWT rejects tokens carrying "aud" + # unless every decode call passes an audience option). "res": mcp_resource_url(), # Lets the MCP endpoint attribute a request to its consent grant # so Connected Apps can show when the grant was actually used. diff --git a/backend/core/security.py b/backend/core/security.py index 011a831e2..41ec638e1 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -8,9 +8,9 @@ from threading import Lock from typing import Any, Optional, Union +import jwt from argon2 import PasswordHasher from argon2.exceptions import InvalidHashError, VerificationError, VerifyMismatchError -from jose import jwt from backend.utils.path_manager import path_manager from backend.utils.time import utc_now @@ -312,19 +312,15 @@ def create_access_token( def decode_access_token(token: str) -> dict[str, Any]: """Decode and verify a JWT using the keyring entry indicated by its ``kid``. - Raises :class:`jose.JWTError` if the token cannot be verified with any - known key. + Raises :class:`jwt.InvalidTokenError` if the token is malformed or cannot + be verified with any known key. """ - try: - unverified_header = jwt.get_unverified_header(token) - except Exception: # pragma: no cover - defensive: jose raises subclass of Exception # noqa: BLE001 - unverified_header = {} - kid = unverified_header.get("kid") if isinstance(unverified_header, dict) else None + # PyJWT parses the header strictly: a malformed token or a non-string + # ``kid`` raises InvalidTokenError here, before any key lookup. + kid = jwt.get_unverified_header(token).get("kid") signing_key = get_signing_key_for_kid(kid) if signing_key is None: - from jose import JWTError - - raise JWTError("Unknown signing key id") + raise jwt.InvalidTokenError("Unknown signing key id") return jwt.decode(token, signing_key, algorithms=[ALGORITHM]) diff --git a/backend/tests/test_jwt_keyring.py b/backend/tests/test_jwt_keyring.py index 450a1b139..9c3b511c1 100644 --- a/backend/tests/test_jwt_keyring.py +++ b/backend/tests/test_jwt_keyring.py @@ -1,5 +1,6 @@ from datetime import timedelta +import jwt import pytest from backend.core import security @@ -88,9 +89,7 @@ def test_pruning_retired_keys_invalidates_tokens_signed_by_them(isolated_keyring assert original_kid in removed - from jose import JWTError - - with pytest.raises(JWTError): + with pytest.raises(jwt.InvalidTokenError): security.decode_access_token(token_before) diff --git a/backend/tests/test_jwt_verification.py b/backend/tests/test_jwt_verification.py new file mode 100644 index 000000000..f0f5618ef --- /dev/null +++ b/backend/tests/test_jwt_verification.py @@ -0,0 +1,352 @@ +"""JWT verification contract: tokens issued before the PyJWT migration keep +working, every token type round-trips, and forged tokens are refused.""" + +from __future__ import annotations + +import base64 +import hashlib +import hmac +import json +import time +import warnings +from datetime import timedelta +from typing import Any + +import jwt +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa +from fastapi import HTTPException +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine +from sqlalchemy.orm import sessionmaker +from sqlalchemy.pool import StaticPool + +from backend.api import deps +from backend.core import security +from backend.utils.time import utc_now + +# Minted once by python-jose 3.5.0 through upstream's create_access_token (the +# implementation this migration replaced), for user "alice" with +# token_version 3 and a 100-year expiry, signed by the keyring entry below. +# python-jose is no longer installed, so the tokens are kept as static strings: +# they prove that sessions, API tokens and MCP tokens issued before an upgrade +# still verify after it. +JOSE_KID = "k_f1x7ure" +JOSE_KEY = "6a4f0c2e9b8d71a3c5e0f4b2d6a8c1e3f5b7d9a0c2e4f6a8b0d2c4e6f8a0b2c4" +JOSE_TOKENS = { + security.SESSION_TOKEN_TYPE: ( + "eyJhbGciOiJIUzI1NiIsImtpZCI6ImtfZjF4N3VyZSIsInR5cCI6IkpXVCJ9." + "eyJleHAiOjQ5NDUwOTIxMzcsImlhdCI6MTc5MTQ5MjEzNywic3ViIjoiYWxpY2UiLCJ0b2tl" + "bl90eXBlIjoic2Vzc2lvbiIsInNjb3BlcyI6WyJzZXNzaW9uOndlYiJdLCJqdGkiOiJhZGEy" + "ZTVjODdmZDg0OTM1ODU5ZTljOGZlN2IwMzRjNyIsInR2IjozfQ." + "fCA26sLNcXh8yo_HuKZSdp2I-4F_vEb58QD5BNfu7R4" + ), + security.API_TOKEN_TYPE: ( + "eyJhbGciOiJIUzI1NiIsImtpZCI6ImtfZjF4N3VyZSIsInR5cCI6IkpXVCJ9." + "eyJleHAiOjQ5NDUwOTIxMzcsImlhdCI6MTc5MTQ5MjEzNywic3ViIjoiYWxpY2UiLCJ0b2tl" + "bl90eXBlIjoiYXBpIiwic2NvcGVzIjpbImFwaTpmdWxsIl0sImp0aSI6Ijk5ZGZiMzU1YjY1" + "MzQ3Nzk4MTE0YzE1OTFiNmQzOWQxIiwidHYiOjN9." + "mZo7zyy0yeinsLASqjxlvVA6fULkdg3QXhm9y1fiSQM" + ), + security.MCP_TOKEN_TYPE: ( + "eyJhbGciOiJIUzI1NiIsImtpZCI6ImtfZjF4N3VyZSIsInR5cCI6IkpXVCJ9." + "eyJleHAiOjQ5NDUwOTIxMzcsImlhdCI6MTc5MTQ5MjEzNywic3ViIjoiYWxpY2UiLCJ0b2tl" + "bl90eXBlIjoibWNwIiwic2NvcGVzIjpbIm1jcDpyZWFkIiwibWNwOndyaXRlIl0sImp0aSI6" + "ImI0YjE2ZjI3Yjc5NTQwOGZiNTNiZGNlZTQ2MzU4NTJhIiwidHYiOjMsImNsaWVudF9pZCI6" + "ImNsaWVudC1maXh0dXJlIiwicmVzIjoiaHR0cHM6Ly9ub2pvaW4uZXhhbXBsZS9tY3AiLCJn" + "cmFudF9pZCI6ImdyYW50LWZpeHR1cmUifQ." + "Dt3OykV2yZUIddX_PgGOetAyNUHYOHNS1LdDS73bmjo" + ), +} +JOSE_SCOPES = { + security.SESSION_TOKEN_TYPE: [security.WEB_SESSION_SCOPE], + security.API_TOKEN_TYPE: [security.API_ACCESS_SCOPE], + security.MCP_TOKEN_TYPE: [security.MCP_READ_SCOPE, security.MCP_WRITE_SCOPE], +} +MCP_EXTRA_CLAIMS = { + "client_id": "client-fixture", + "res": "https://nojoin.example/mcp", + "grant_id": "grant-fixture", +} +TOKEN_TYPES = list(JOSE_TOKENS) + +SCHEMA_STATEMENTS = [ + """ + CREATE TABLE users ( + id INTEGER PRIMARY KEY, + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL, + username VARCHAR(255) NOT NULL, + hashed_password VARCHAR(255) NOT NULL DEFAULT '', + is_active BOOLEAN NOT NULL DEFAULT 1, + is_superuser BOOLEAN NOT NULL DEFAULT 0, + force_password_change BOOLEAN NOT NULL DEFAULT 0, + role VARCHAR(32) NOT NULL DEFAULT 'user', + token_version INTEGER NOT NULL DEFAULT 0, + settings JSON, + has_seen_demo_recording BOOLEAN NOT NULL DEFAULT 0, + invitation_id INTEGER + ) + """, + """ + CREATE TABLE revoked_jwts ( + jti VARCHAR(64) PRIMARY KEY, + user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + token_type VARCHAR(32) NOT NULL, + expires_at DATETIME NOT NULL, + revoked_at DATETIME NOT NULL, + reason VARCHAR(64) + ) + """, +] + + +@pytest.fixture +def anyio_backend() -> str: + return "asyncio" + + +@pytest.fixture +def jose_keyring(monkeypatch, tmp_path): + """A keyring holding the key the python-jose fixture tokens were signed with.""" + monkeypatch.delenv("SECRET_KEY", raising=False) + + class _StubPathManager: + user_data_directory = tmp_path + + monkeypatch.setattr(security, "path_manager", _StubPathManager()) + (tmp_path / ".secret_keys.json").write_text( + json.dumps({"active": JOSE_KID, "keys": {JOSE_KID: JOSE_KEY}}), + encoding="utf-8", + ) + yield tmp_path + + +@pytest.fixture +async def db_with_alice(): + engine = create_async_engine( + "sqlite+aiosqlite://", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + future=True, + ) + async with engine.begin() as conn: + for stmt in SCHEMA_STATEMENTS: + await conn.execute(text(stmt)) + now = utc_now() + await conn.execute( + text( + "INSERT INTO users (id, created_at, updated_at, username, token_version)" + " VALUES (1, :ts, :ts, 'alice', 3)" + ), + {"ts": now}, + ) + factory = sessionmaker(engine, class_=AsyncSession, expire_on_commit=False) + async with factory() as session: + yield session + await engine.dispose() + + +def _b64url(data: bytes) -> str: + return base64.urlsafe_b64encode(data).rstrip(b"=").decode("ascii") + + +def _claims(token_type: str) -> dict[str, Any]: + now = int(time.time()) + return { + "exp": now + 300, + "iat": now, + "sub": "alice", + "token_type": token_type, + "scopes": JOSE_SCOPES[token_type], + "jti": "f" * 32, + "tv": 3, + } + + +def _hs256_token(header: dict[str, Any], claims: dict[str, Any], secret: bytes) -> str: + """Sign by hand, so no library refuses the key or the header we forge.""" + signing_input = ( + f"{_b64url(json.dumps(header).encode())}.{_b64url(json.dumps(claims).encode())}" + ) + signature = hmac.new(secret, signing_input.encode("ascii"), hashlib.sha256) + return f"{signing_input}.{_b64url(signature.digest())}" + + +def _rsa_private_key() -> rsa.RSAPrivateKey: + return rsa.generate_private_key(public_exponent=65537, key_size=2048) + + +def _forge_hs256_with_der_public_key() -> str: + # GHSA-3qf3-8w2g-rqmx: an HS256 token whose HMAC secret is a DER-encoded + # RSA public key. Nojoin only ever verifies with keyring secrets, so this + # must fail signature verification. + der = ( + _rsa_private_key() + .public_key() + .public_bytes( + serialization.Encoding.DER, + serialization.PublicFormat.SubjectPublicKeyInfo, + ) + ) + header = {"alg": "HS256", "kid": JOSE_KID, "typ": "JWT"} + return _hs256_token(header, _claims(security.SESSION_TOKEN_TYPE), der) + + +def _forge_alg_none() -> str: + header = {"alg": "none", "kid": JOSE_KID, "typ": "JWT"} + body = json.dumps(_claims(security.SESSION_TOKEN_TYPE)).encode() + return f"{_b64url(json.dumps(header).encode())}.{_b64url(body)}." + + +def _forge_rs256() -> str: + return jwt.encode( + _claims(security.SESSION_TOKEN_TYPE), + _rsa_private_key(), + algorithm="RS256", + headers={"kid": JOSE_KID}, + ) + + +def _forge_list_kid() -> str: + # A non-string kid used to reach the keyring lookup and raise TypeError + # (an unhandled 500); it is now refused as a malformed header. + header = {"alg": "HS256", "kid": [JOSE_KID], "typ": "JWT"} + return _hs256_token(header, _claims(security.SESSION_TOKEN_TYPE), JOSE_KEY.encode()) + + +def _forge_unknown_kid() -> str: + header = {"alg": "HS256", "kid": "k_unknown", "typ": "JWT"} + return _hs256_token(header, _claims(security.SESSION_TOKEN_TYPE), JOSE_KEY.encode()) + + +def _forge_future_iat() -> str: + # Correctly signed, but issued in the future: PyJWT validates iat, with + # no leeway since the api process both issues and verifies every token. + claims = _claims(security.SESSION_TOKEN_TYPE) + claims["iat"] += 600 + header = {"alg": "HS256", "kid": JOSE_KID, "typ": "JWT"} + return _hs256_token(header, claims, JOSE_KEY.encode()) + + +FORGED_TOKENS = { + "hs256-der-public-key": _forge_hs256_with_der_public_key, + "alg-none": _forge_alg_none, + "rs256": _forge_rs256, + "list-kid": _forge_list_kid, + "unknown-kid": _forge_unknown_kid, + "future-iat": _forge_future_iat, + "malformed": lambda: "not-a-jwt", +} + + +@pytest.mark.parametrize("token_type", TOKEN_TYPES) +def test_python_jose_tokens_still_verify(jose_keyring, token_type): + decoded = security.decode_access_token(JOSE_TOKENS[token_type]) + + assert decoded["sub"] == "alice" + assert decoded["token_type"] == token_type + assert decoded["scopes"] == JOSE_SCOPES[token_type] + assert decoded["tv"] == 3 + assert isinstance(decoded["jti"], str) and len(decoded["jti"]) == 32 + if token_type == security.MCP_TOKEN_TYPE: + assert {k: decoded[k] for k in MCP_EXTRA_CLAIMS} == MCP_EXTRA_CLAIMS + + +@pytest.mark.anyio +@pytest.mark.parametrize("token_type", TOKEN_TYPES) +async def test_python_jose_tokens_still_authenticate( + jose_keyring, db_with_alice, token_type +): + user, payload = await deps.get_authenticated_token_details( + db_with_alice, + JOSE_TOKENS[token_type], + allowed_token_types={token_type}, + required_scopes_by_type={token_type: set(JOSE_SCOPES[token_type])}, + ) + + assert user.username == "alice" + assert payload["token_type"] == token_type + + +@pytest.mark.parametrize("token_type", TOKEN_TYPES) +def test_new_tokens_round_trip_with_string_sub_and_jti(jose_keyring, token_type): + extra = MCP_EXTRA_CLAIMS if token_type == security.MCP_TOKEN_TYPE else None + token = security.create_access_token( + "alice", + token_type=token_type, + scopes=JOSE_SCOPES[token_type], + token_version=3, + extra_claims=extra, + ) + + header = jwt.get_unverified_header(token) + decoded = security.decode_access_token(token) + + assert header["alg"] == security.ALGORITHM + assert header["kid"] == JOSE_KID + assert isinstance(decoded["sub"], str) and decoded["sub"] == "alice" + assert isinstance(decoded["jti"], str) and decoded["jti"] + # utc_now() is naive UTC; the encoded iat must be the real epoch time, + # not the naive value read in the host's local timezone. + assert abs(decoded["iat"] - time.time()) < 5 + assert decoded["exp"] > decoded["iat"] + if extra: + assert {k: decoded[k] for k in extra} == extra + + +@pytest.mark.parametrize("forge", FORGED_TOKENS.values(), ids=FORGED_TOKENS.keys()) +def test_forged_tokens_are_rejected_by_decode(jose_keyring, forge): + with pytest.raises(jwt.InvalidTokenError): + security.decode_access_token(forge()) + + +@pytest.mark.anyio +@pytest.mark.parametrize("forge", FORGED_TOKENS.values(), ids=FORGED_TOKENS.keys()) +async def test_forged_tokens_are_401_before_any_database_lookup(jose_keyring, forge): + with pytest.raises(HTTPException) as excinfo: + await deps.get_authenticated_token_details( + None, # no database: a rejected token must never reach one + forge(), + allowed_token_types={security.SESSION_TOKEN_TYPE}, + ) + + assert excinfo.value.status_code == 401 + + +def test_short_secret_key_still_verifies_but_warns(monkeypatch): + monkeypatch.setenv("SECRET_KEY", "short-key") + + with pytest.warns(jwt.InsecureKeyLengthWarning): + token = security.create_access_token( + "alice", + token_type=security.SESSION_TOKEN_TYPE, + scopes=[security.WEB_SESSION_SCOPE], + expires_delta=timedelta(minutes=5), + token_version=0, + ) + with pytest.warns(jwt.InsecureKeyLengthWarning): + decoded = security.decode_access_token(token) + + assert decoded["sub"] == "alice" + + +def test_generated_keyring_keys_do_not_warn(monkeypatch, tmp_path): + monkeypatch.delenv("SECRET_KEY", raising=False) + + class _StubPathManager: + user_data_directory = tmp_path + + monkeypatch.setattr(security, "path_manager", _StubPathManager()) + + with warnings.catch_warnings(): + warnings.simplefilter("error", jwt.InsecureKeyLengthWarning) + token = security.create_access_token( + "alice", + token_type=security.SESSION_TOKEN_TYPE, + scopes=[security.WEB_SESSION_SCOPE], + expires_delta=timedelta(minutes=5), + token_version=0, + ) + assert security.decode_access_token(token)["sub"] == "alice" diff --git a/docs/SECURITY.md b/docs/SECURITY.md index e03cc13aa..4e681b2e6 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -77,9 +77,10 @@ JWT signing material is stored as a small keyring rather than a single static va - The keyring is persisted to `/.secret_keys.json` and contains an `active` key id (`kid`) plus any prior keys that are still trusted. - Every issued JWT carries a `kid` header. Verification picks the matching key from the keyring; if the `kid` is not known the token is rejected. +- Tokens are signed and verified with PyJWT using HS256 only. Verification refuses any other `alg` (including `none` and the asymmetric algorithms), a malformed header, an expired token, and a token whose `iat` lies in the future. The api process both issues and verifies every token, so no clock-skew leeway is applied. Keyring entries are always symmetric secrets, never public keys. - Operators with shell access can rotate the key by calling `backend.core.security.rotate_signing_key()`. Rotation generates a fresh `kid`, makes it active, and (by default) keeps the previous key in the ring so currently outstanding tokens keep verifying until their natural expiry. - After enough time has passed for outstanding tokens to expire, `prune_signing_keys()` removes retired keys from the keyring. Any token still signed by a removed key fails verification immediately, providing a hard cut-over. -- Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. +- Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. Use at least 32 bytes of random data (for example `openssl rand -hex 32`, the same size as a generated keyring key): RFC 7518 Section 3.2 requires an HS256 key at least as long as its 256-bit hash output. A shorter value still works, so existing deployments keep signing in, but PyJWT emits an `InsecureKeyLengthWarning` when it signs or verifies with it. - Existing single-key installs are migrated automatically: the legacy `/.secret_key` file is loaded into the keyring as `kid="legacy"` on first startup and the legacy file is renamed. ## Browser Capture Security diff --git a/requirements/base.txt b/requirements/base.txt index 79f3db93a..f3cdbf100 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -2,7 +2,11 @@ sqlmodel==0.0.47 psycopg2-binary==2.9.13 asyncpg==0.31.0 python-dotenv==1.2.3 -python-jose[cryptography]==3.5.0 +PyJWT==2.15.1 +# Imported directly by backend/core/encryption.py (Fernet). It used to arrive +# through the python-jose[cryptography] extra, so it is pinned here rather than +# left to a transitive resolve. +cryptography==50.0.2 argon2-cffi==25.1.0 python-multipart==0.0.32 aiofiles==25.1.0 From 3147af912c53cb0f85372272c2d48bbc9d9749b4 Mon Sep 17 00:00:00 2001 From: Mike Mulligan Date: Thu, 8 Oct 2026 18:18:37 -0400 Subject: [PATCH 02/14] test(auth): turn the DER public-key forged token into a plain wrong-key case The token was signed with the DER encoding of a freshly generated RSA public key that the server never sees, so it only ever tested a wrong HMAC secret. It could not exercise the python-jose advisory it was named after, which needs the verifier's own key to be a public key; Nojoin has none. Naming the advisory in a test id suggested otherwise. The case is kept as "wrong-key", signed with random bytes, because no other forged token reaches the HMAC signature check: unknown-kid stops at the keyring lookup and rs256 at the algorithm pin. --- backend/tests/test_jwt_verification.py | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/backend/tests/test_jwt_verification.py b/backend/tests/test_jwt_verification.py index f0f5618ef..c221307f0 100644 --- a/backend/tests/test_jwt_verification.py +++ b/backend/tests/test_jwt_verification.py @@ -7,6 +7,7 @@ import hashlib import hmac import json +import secrets import time import warnings from datetime import timedelta @@ -14,7 +15,6 @@ import jwt import pytest -from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives.asymmetric import rsa from fastapi import HTTPException from sqlalchemy import text @@ -178,20 +178,13 @@ def _rsa_private_key() -> rsa.RSAPrivateKey: return rsa.generate_private_key(public_exponent=65537, key_size=2048) -def _forge_hs256_with_der_public_key() -> str: - # GHSA-3qf3-8w2g-rqmx: an HS256 token whose HMAC secret is a DER-encoded - # RSA public key. Nojoin only ever verifies with keyring secrets, so this - # must fail signature verification. - der = ( - _rsa_private_key() - .public_key() - .public_bytes( - serialization.Encoding.DER, - serialization.PublicFormat.SubjectPublicKeyInfo, - ) - ) +def _forge_wrong_key() -> str: + # A known kid and HS256, signed with a secret that is not the keyring's: + # the one forged token here that fails at the HMAC signature check. header = {"alg": "HS256", "kid": JOSE_KID, "typ": "JWT"} - return _hs256_token(header, _claims(security.SESSION_TOKEN_TYPE), der) + return _hs256_token( + header, _claims(security.SESSION_TOKEN_TYPE), secrets.token_bytes(32) + ) def _forge_alg_none() -> str: @@ -231,7 +224,7 @@ def _forge_future_iat() -> str: FORGED_TOKENS = { - "hs256-der-public-key": _forge_hs256_with_der_public_key, + "wrong-key": _forge_wrong_key, "alg-none": _forge_alg_none, "rs256": _forge_rs256, "list-kid": _forge_list_kid, From 24392506b1c24eaae2ad532f3c58fa54827a1fce Mon Sep 17 00:00:00 2001 From: Mike Mulligan Date: Thu, 8 Oct 2026 18:18:45 -0400 Subject: [PATCH 03/14] test(auth): pin that PyJWT mints the python-jose fixtures byte for byte The fixtures prove PyJWT accepts tokens python-jose issued. This proves more: for the same claims, key and kid, PyJWT produces the identical token, because both libraries sort the header keys and use compact JSON separators. The wire format is unchanged, and a future PyJWT serialization change would fail here instead of surfacing as a mismatch between old and new tokens. The claims are read straight from the fixture's payload segment, so the check does not depend on PyJWT's own decode. --- backend/tests/test_jwt_verification.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/backend/tests/test_jwt_verification.py b/backend/tests/test_jwt_verification.py index c221307f0..0bbcf9abb 100644 --- a/backend/tests/test_jwt_verification.py +++ b/backend/tests/test_jwt_verification.py @@ -152,6 +152,10 @@ def _b64url(data: bytes) -> str: return base64.urlsafe_b64encode(data).rstrip(b"=").decode("ascii") +def _b64url_json(segment: str) -> dict[str, Any]: + return json.loads(base64.urlsafe_b64decode(segment + "=" * (-len(segment) % 4))) + + def _claims(token_type: str) -> dict[str, Any]: now = int(time.time()) return { @@ -263,6 +267,24 @@ async def test_python_jose_tokens_still_authenticate( assert payload["token_type"] == token_type +@pytest.mark.parametrize("token_type", TOKEN_TYPES) +def test_pyjwt_mints_the_python_jose_fixture_byte_for_byte(token_type): + # Same claims, key and kid as the python-jose fixture, encoded the way + # create_access_token encodes them: the wire format is unchanged, so + # compatibility does not rest on PyJWT tolerating jose's output. + header_segment, payload_segment, _ = JOSE_TOKENS[token_type].split(".") + assert _b64url_json(header_segment)["kid"] == JOSE_KID + + reminted = jwt.encode( + _b64url_json(payload_segment), + JOSE_KEY, + algorithm=security.ALGORITHM, + headers={"kid": JOSE_KID}, + ) + + assert reminted == JOSE_TOKENS[token_type] + + @pytest.mark.parametrize("token_type", TOKEN_TYPES) def test_new_tokens_round_trip_with_string_sub_and_jti(jose_keyring, token_type): extra = MCP_EXTRA_CLAIMS if token_type == security.MCP_TOKEN_TYPE else None From bcdb4a96246a7aa5d21988fd45273b105ad2541d Mon Sep 17 00:00:00 2001 From: Mike Mulligan Date: Thu, 8 Oct 2026 18:19:09 -0400 Subject: [PATCH 04/14] fix(auth): document that decode_access_token raises PyJWTError The docstring promised jwt.InvalidTokenError, but jwt.decode also raises jwt.InvalidKeyError when the key stored for the token's kid is unusable as an HMAC secret (empty, or shaped like a PEM or SSH key). InvalidKeyError is a PyJWTError, not an InvalidTokenError, so a future caller following the docstring would turn that case into a 500. Both existing callers are already safe: the auth dependency catches jwt.PyJWTError and logout catches Exception. The new test pins the 401 for a token whose kid maps to an empty key. python-jose accepted such a token, signed with the empty key, as valid. --- backend/core/security.py | 7 +++++-- backend/tests/test_jwt_verification.py | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/backend/core/security.py b/backend/core/security.py index 41ec638e1..a02a020ad 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -312,8 +312,11 @@ def create_access_token( def decode_access_token(token: str) -> dict[str, Any]: """Decode and verify a JWT using the keyring entry indicated by its ``kid``. - Raises :class:`jwt.InvalidTokenError` if the token is malformed or cannot - be verified with any known key. + Raises :class:`jwt.PyJWTError` if the token is malformed or cannot be + verified with any known key. Catch that base class: besides + :class:`jwt.InvalidTokenError`, ``jwt.decode`` raises + :class:`jwt.InvalidKeyError` (not an ``InvalidTokenError``) when the + stored key for the token's ``kid`` is unusable as an HMAC secret. """ # PyJWT parses the header strictly: a malformed token or a non-string # ``kid`` raises InvalidTokenError here, before any key lookup. diff --git a/backend/tests/test_jwt_verification.py b/backend/tests/test_jwt_verification.py index 0bbcf9abb..1364b157d 100644 --- a/backend/tests/test_jwt_verification.py +++ b/backend/tests/test_jwt_verification.py @@ -330,6 +330,27 @@ async def test_forged_tokens_are_401_before_any_database_lookup(jose_keyring, fo assert excinfo.value.status_code == 401 +@pytest.mark.anyio +async def test_unusable_stored_key_is_refused_as_a_401(jose_keyring): + # jwt.decode raises InvalidKeyError, which is a PyJWTError but not an + # InvalidTokenError, when the key stored for a kid is empty. + (jose_keyring / ".secret_keys.json").write_text( + json.dumps({"active": JOSE_KID, "keys": {JOSE_KID: JOSE_KEY, "k_empty": ""}}), + encoding="utf-8", + ) + header = {"alg": "HS256", "kid": "k_empty", "typ": "JWT"} + token = _hs256_token(header, _claims(security.SESSION_TOKEN_TYPE), b"") + + with pytest.raises(jwt.InvalidKeyError): + security.decode_access_token(token) + with pytest.raises(HTTPException) as excinfo: + await deps.get_authenticated_token_details( + None, token, allowed_token_types={security.SESSION_TOKEN_TYPE} + ) + + assert excinfo.value.status_code == 401 + + def test_short_secret_key_still_verifies_but_warns(monkeypatch): monkeypatch.setenv("SECRET_KEY", "short-key") From 7a289c0d8e08257daae1de5adda75ae5d0e3bd46 Mon Sep 17 00:00:00 2001 From: Mike Mulligan Date: Thu, 8 Oct 2026 18:19:38 -0400 Subject: [PATCH 05/14] fix(auth): refuse an empty JWT signing key at api startup An empty legacy .secret_key (a zero-byte file from a failed write or a full disk) became the active keyring entry. python-jose signed with the empty HMAC key, so every token was forgeable. PyJWT refuses an empty key with InvalidKeyError, which create_access_token does not catch: sign-in became a bare 500 and every token check a 401, with nothing pointing at the cause. Loading the keyring now refuses an empty active key, both when migrating the legacy file and when reading .secret_keys.json, with an error that names the file and says to delete it and restart to generate a new key. The api loads the keyring at the start of its lifespan instead of at the first sign-in, so the error stops startup and lands in the startup log. Nothing is written for an empty legacy file, so the restart after deleting it generates a fresh key. Decision record: - Context: the empty key was already unusable after the PyJWT migration; the question was only how it fails. - Alternatives: silently generate a fresh key (hides a disk or write problem the operator should see, and signs everyone out without a word); a logged error at the point of use (every sign-in still fails, and the message is buried among request logs). - Trade-offs: an install with an empty key no longer starts until the operator deletes one file. It could not sign anyone in before this either, and regenerating signs everyone out, which an empty key warrants. A non-active empty key is left alone: a token naming it is refused with a 401. docs/SECURITY.md and docs/DEPLOYMENT.md describe the refusal and the fix. --- backend/core/security.py | 15 +++++++++ backend/main.py | 4 +++ backend/tests/test_jwt_keyring.py | 53 +++++++++++++++++++++++++++++++ docs/DEPLOYMENT.md | 2 +- docs/SECURITY.md | 1 + 5 files changed, 74 insertions(+), 1 deletion(-) diff --git a/backend/core/security.py b/backend/core/security.py index a02a020ad..0dbc01a30 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -72,9 +72,22 @@ def _read_keyring_file() -> Optional[dict[str, Any]]: or data["active"] not in data["keys"] ): raise RuntimeError(f"JWT keyring at {keyring_file} is malformed.") + if not data["keys"][data["active"]]: + raise _empty_signing_key_error(keyring_file) return data +def _empty_signing_key_error(key_file: Path) -> RuntimeError: + # PyJWT refuses an empty HMAC key, so an empty key would fail every + # sign-in with a bare 500. Refusing it at load names the file instead. + return RuntimeError( + f"The JWT signing key in {key_file} is empty, so no token can be signed " + f"or verified. Delete {key_file} and restart Nojoin to generate a new " + "key (everyone signed in will have to sign in again), or set the " + "SECRET_KEY environment variable." + ) + + def _write_keyring_file(data: dict[str, Any]) -> None: keyring_file = _keyring_path() keyring_file.parent.mkdir(parents=True, exist_ok=True) @@ -97,6 +110,8 @@ def _bootstrap_keyring() -> dict[str, Any]: if legacy_file.exists(): legacy_value = legacy_file.read_text(encoding="utf-8").strip() + if not legacy_value: + raise _empty_signing_key_error(legacy_file) data = { "active": DEFAULT_LEGACY_KID, "keys": {DEFAULT_LEGACY_KID: legacy_value}, diff --git a/backend/main.py b/backend/main.py index d016155b1..83ba491cf 100644 --- a/backend/main.py +++ b/backend/main.py @@ -124,6 +124,7 @@ def _build_redirect_url(self, request: Request) -> str: # Import models to register them with SQLModel from backend.core.db import async_session_maker +from backend.core.security import get_signing_keyring from backend.models.user import User from backend.seed_demo import seed_demo_data from backend.services.model_preparation import enqueue_model_preparation @@ -255,6 +256,9 @@ def log_recordings_storage_warnings(*, logger_instance: logging.Logger) -> None: @asynccontextmanager async def lifespan(app: FastAPI): + # Loading the JWT keyring here, not lazily at the first sign-in, means an + # unusable signing key stops startup with a message naming its file. + get_signing_keyring() run_migrations() await ensure_owner_exists() await log_first_run_setup_pointer() diff --git a/backend/tests/test_jwt_keyring.py b/backend/tests/test_jwt_keyring.py index 9c3b511c1..d33efde16 100644 --- a/backend/tests/test_jwt_keyring.py +++ b/backend/tests/test_jwt_keyring.py @@ -1,8 +1,11 @@ +import json +import re from datetime import timedelta import jwt import pytest +from backend import main from backend.core import security @@ -103,3 +106,53 @@ class _StubPathManager: with pytest.raises(RuntimeError): security.rotate_signing_key() + + +@pytest.fixture +def anyio_backend() -> str: + return "asyncio" + + +def test_empty_legacy_secret_key_is_refused_with_the_file_named(isolated_keyring): + legacy_file = isolated_keyring / ".secret_key" + legacy_file.write_text("\n", encoding="utf-8") + + with pytest.raises(RuntimeError, match=re.escape(str(legacy_file))): + security.get_signing_keyring() + + # Nothing is persisted, so deleting the empty file and restarting + # generates a fresh key instead of reloading an empty one. + assert not (isolated_keyring / ".secret_keys.json").exists() + assert legacy_file.exists() + legacy_file.unlink() + assert security.get_active_signing_key()[1] + + +def test_empty_active_keyring_key_is_refused_with_the_file_named(isolated_keyring): + keyring_file = isolated_keyring / ".secret_keys.json" + keyring_file.write_text( + json.dumps({"active": "legacy", "keys": {"legacy": ""}}), encoding="utf-8" + ) + + with pytest.raises(RuntimeError, match=re.escape(str(keyring_file))): + security.create_access_token( + "alice", + token_type=security.SESSION_TOKEN_TYPE, + scopes=[security.WEB_SESSION_SCOPE], + token_version=0, + ) + + +@pytest.mark.anyio +async def test_api_startup_refuses_an_empty_signing_key(isolated_keyring, monkeypatch): + (isolated_keyring / ".secret_keys.json").write_text( + json.dumps({"active": "legacy", "keys": {"legacy": ""}}), encoding="utf-8" + ) + migrations: list[bool] = [] + monkeypatch.setattr(main, "run_migrations", lambda: migrations.append(True)) + + with pytest.raises(RuntimeError, match="is empty"): + async with main.lifespan(main.app): + pass + + assert migrations == [] diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 619c95cfc..fa47d035d 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -363,7 +363,7 @@ The compose stack derives internal service URLs for PostgreSQL, Redis, and Celer Keep any secrets, private mounts, or machine-specific overrides in your local `docker-compose.yml`, not in the tracked template. Every variable below is only read by a container if that container's compose service passes it in, which the shipped `docker-compose.example.yml` does for all of them. If you maintain your own compose file, a variable you set in `.env` but never declare in a service's `environment:` block is silently discarded: the stack starts and the application default applies with no warning. -Nojoin auto-generates and persists its JWT signing keyring under `data/.secret_keys.json` in the default deployment, migrating any legacy `data/.secret_key` file on startup, so no `.env` setting is required for that. +Nojoin auto-generates and persists its JWT signing keyring under `data/.secret_keys.json` in the default deployment, migrating any legacy `data/.secret_key` file on startup, so no `.env` setting is required for that. If that key file is empty, the api refuses to start and logs which file to delete; see [JWT Signing Key Rotation](SECURITY.md#jwt-signing-key-rotation). Nojoin can also auto-generate `data/.data_encryption_key`, but operators should treat that as a fallback rather than the primary persistence strategy. ### Always Set diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 4e681b2e6..b0cceba08 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -82,6 +82,7 @@ JWT signing material is stored as a small keyring rather than a single static va - After enough time has passed for outstanding tokens to expire, `prune_signing_keys()` removes retired keys from the keyring. Any token still signed by a removed key fails verification immediately, providing a hard cut-over. - Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. Use at least 32 bytes of random data (for example `openssl rand -hex 32`, the same size as a generated keyring key): RFC 7518 Section 3.2 requires an HS256 key at least as long as its 256-bit hash output. A shorter value still works, so existing deployments keep signing in, but PyJWT emits an `InsecureKeyLengthWarning` when it signs or verifies with it. - Existing single-key installs are migrated automatically: the legacy `/.secret_key` file is loaded into the keyring as `kid="legacy"` on first startup and the legacy file is renamed. +- The api loads the keyring at startup and refuses to start if the signing key is empty (an empty legacy `.secret_key`, or an empty active entry in `.secret_keys.json`, for example after a write to a full disk). The error names the file: delete it and restart to generate a new key. Everyone signed in has to sign in again, which is the right outcome, since an empty HMAC key would let anyone forge a token. ## Browser Capture Security From 4236d9a7fbd5e24e91be8c7881561bedc326cf52 Mon Sep 17 00:00:00 2001 From: Mike Mulligan Date: Thu, 8 Oct 2026 18:20:05 -0400 Subject: [PATCH 06/14] docs(security): describe the SECRET_KEY size as 256 random bits "32 bytes of random data" read as if the HMAC key itself were 32 bytes. openssl rand -hex 32 and the generated keyring keys (secrets.token_hex(32)) are both 64-character hex strings: 64 bytes of key carrying 256 random bits. This also corrects two version attributions in the body of 163adbe, which cannot be amended on a pushed branch: PyJWT refuses a future iat since 2.6.0, not 2.10, and InsecureKeyLengthWarning arrived in 2.11.0. The sub and jti string checks are from 2.10.0, as stated. Generated keyring keys are 64-character hex strings carrying 256 random bits, not "32 random bytes". --- docs/SECURITY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index b0cceba08..05907c73f 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -80,7 +80,7 @@ JWT signing material is stored as a small keyring rather than a single static va - Tokens are signed and verified with PyJWT using HS256 only. Verification refuses any other `alg` (including `none` and the asymmetric algorithms), a malformed header, an expired token, and a token whose `iat` lies in the future. The api process both issues and verifies every token, so no clock-skew leeway is applied. Keyring entries are always symmetric secrets, never public keys. - Operators with shell access can rotate the key by calling `backend.core.security.rotate_signing_key()`. Rotation generates a fresh `kid`, makes it active, and (by default) keeps the previous key in the ring so currently outstanding tokens keep verifying until their natural expiry. - After enough time has passed for outstanding tokens to expire, `prune_signing_keys()` removes retired keys from the keyring. Any token still signed by a removed key fails verification immediately, providing a hard cut-over. -- Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. Use at least 32 bytes of random data (for example `openssl rand -hex 32`, the same size as a generated keyring key): RFC 7518 Section 3.2 requires an HS256 key at least as long as its 256-bit hash output. A shorter value still works, so existing deployments keep signing in, but PyJWT emits an `InsecureKeyLengthWarning` when it signs or verifies with it. +- Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. Use a random value carrying at least 256 bits, for example `openssl rand -hex 32` (a 64-character hex string, the same form as a generated keyring key): RFC 7518 Section 3.2 requires an HS256 key at least as long as its 256-bit hash output. A shorter value still works, so existing deployments keep signing in, but PyJWT emits an `InsecureKeyLengthWarning` when it signs or verifies with it. - Existing single-key installs are migrated automatically: the legacy `/.secret_key` file is loaded into the keyring as `kid="legacy"` on first startup and the legacy file is renamed. - The api loads the keyring at startup and refuses to start if the signing key is empty (an empty legacy `.secret_key`, or an empty active entry in `.secret_keys.json`, for example after a write to a full disk). The error names the file: delete it and restart to generate a new key. Everyone signed in has to sign in again, which is the right outcome, since an empty HMAC key would let anyone forge a token. From b21b706d4515779922ff106b7b69e71cfadd3736 Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:04:58 -0400 Subject: [PATCH 07/14] fix(auth): keep the api serving when the JWT keyring cannot be loaded PyJWT refuses an empty signing key, which surfaced as a bare 500 at sign-in. The previous commit loaded the keyring at startup and stopped the api when the key was unusable. The api now logs the problem and keeps serving instead, and sign-in and token verification keep failing closed: - Startup loads the keyring and, if it cannot be used, logs an ERROR that names the file and the fix (delete it and restart, which generates a new key and signs everyone out, or set SECRET_KEY). - Signing a token with an unusable keyring raises SigningKeyUnavailableError. An exception handler turns it into a 500 whose detail points to the api log, and logs the full message. It is a 500 rather than a 503 because the frontend reads 502-504 as "backend unreachable", and the sign-in page shows the detail. - Verifying a token turns the same error into InvalidTokenError, so every token is a 401 until the keyring is fixed. - A blank .secret_keys.json (a write cut short, for example by a full disk) gets the same message as an empty key. A malformed or unreadable keyring, and a data directory the api cannot write, get their own message naming the file or directory. - The empty-key check moves from reading the file to loading the keyring, so rotate_signing_key() can replace an empty active key. Decision record Context: the api is the only surface that can tell an administrator what is wrong, so upstream keeps it serving when a startup probe fails (the recordings storage probe in main.py, nojoin_chown_tree in entrypoint_common.sh). nginx and the frontend wait on a healthy api. Alternatives: refusing to start on an unusable key, as the previous commit did (rejected: the whole UI goes down and the only diagnostic is the container log, against the convention above); generating a fresh key silently (hides the failure from the operator); reporting only at sign-in (nothing is said until someone signs in). Trade-offs: the api reports healthy while nobody can sign in. The startup log, the sign-in error and the per-attempt log line make the cause visible. --- backend/core/security.py | 89 +++++++++--- backend/main.py | 40 +++++- backend/tests/test_jwt_keyring.py | 67 +++++---- backend/tests/test_jwt_keyring_unavailable.py | 135 ++++++++++++++++++ docs/DEPLOYMENT.md | 2 +- docs/SECURITY.md | 2 +- 6 files changed, 284 insertions(+), 51 deletions(-) create mode 100644 backend/tests/test_jwt_keyring_unavailable.py diff --git a/backend/core/security.py b/backend/core/security.py index 0dbc01a30..b3c1bd176 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -55,36 +55,72 @@ def _new_kid() -> str: return f"k_{secrets.token_hex(4)}" +class SigningKeyUnavailableError(RuntimeError): + """The JWT keyring cannot be loaded, so no token can be signed or verified. + + The message names the file at fault and how to fix it, for the api log. + """ + + def _read_keyring_file() -> Optional[dict[str, Any]]: keyring_file = _keyring_path() if not keyring_file.exists(): return None try: - data = json.loads(keyring_file.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError) as exc: - raise RuntimeError( - f"Unable to load JWT keyring at {keyring_file}: {exc}" - ) from exc + content = keyring_file.read_text(encoding="utf-8") + except OSError as exc: + raise _keyring_access_error(exc) from exc + except UnicodeDecodeError as exc: + raise _malformed_keyring_error(keyring_file, str(exc)) from exc + if not content.strip(): + # A write cut short, for example by a full disk, leaves the file blank. + raise _empty_signing_key_error(keyring_file) + try: + data = json.loads(content) + except json.JSONDecodeError as exc: + raise _malformed_keyring_error(keyring_file, str(exc)) from exc if ( not isinstance(data, dict) or not isinstance(data.get("keys"), dict) or not isinstance(data.get("active"), str) or data["active"] not in data["keys"] ): - raise RuntimeError(f"JWT keyring at {keyring_file} is malformed.") - if not data["keys"][data["active"]]: - raise _empty_signing_key_error(keyring_file) + raise _malformed_keyring_error(keyring_file, "no usable active key entry") return data -def _empty_signing_key_error(key_file: Path) -> RuntimeError: +def _replace_key_file_remedy(key_file: Path) -> str: + return ( + f"Delete {key_file} and restart Nojoin to generate a new key (everyone " + "signed in will have to sign in again), or set the SECRET_KEY " + "environment variable." + ) + + +def _empty_signing_key_error(key_file: Path) -> SigningKeyUnavailableError: # PyJWT refuses an empty HMAC key, so an empty key would fail every - # sign-in with a bare 500. Refusing it at load names the file instead. - return RuntimeError( + # sign-in with a bare 500. Detecting it at load names the file instead. + return SigningKeyUnavailableError( f"The JWT signing key in {key_file} is empty, so no token can be signed " - f"or verified. Delete {key_file} and restart Nojoin to generate a new " - "key (everyone signed in will have to sign in again), or set the " - "SECRET_KEY environment variable." + f"or verified. {_replace_key_file_remedy(key_file)}" + ) + + +def _malformed_keyring_error( + keyring_file: Path, reason: str +) -> SigningKeyUnavailableError: + return SigningKeyUnavailableError( + f"The JWT keyring at {keyring_file} is malformed ({reason}), so no token " + f"can be signed or verified. {_replace_key_file_remedy(keyring_file)}" + ) + + +def _keyring_access_error(exc: Exception) -> SigningKeyUnavailableError: + data_dir = _keyring_path().parent + return SigningKeyUnavailableError( + f"Unable to read or create the JWT keyring in {data_dir} ({exc}), so no " + f"token can be signed or verified. Make sure the api can read and write " + f"{data_dir}, or set the SECRET_KEY environment variable." ) @@ -148,10 +184,17 @@ def _load_keyring() -> dict[str, Any]: return {"active": "env", "keys": {"env": env_key}} with _keyring_lock: - existing = _read_keyring_file() - if existing is not None: - return existing - return _bootstrap_keyring() + keyring = _read_keyring_file() + if keyring is None: + try: + keyring = _bootstrap_keyring() + except (OSError, UnicodeDecodeError) as exc: + raise _keyring_access_error(exc) from exc + # Checked here rather than in _read_keyring_file so that + # rotate_signing_key() can still replace an empty active key. + if not keyring["keys"][keyring["active"]]: + raise _empty_signing_key_error(_keyring_path()) + return keyring def get_signing_keyring() -> dict[str, Any]: @@ -331,12 +374,18 @@ def decode_access_token(token: str) -> dict[str, Any]: verified with any known key. Catch that base class: besides :class:`jwt.InvalidTokenError`, ``jwt.decode`` raises :class:`jwt.InvalidKeyError` (not an ``InvalidTokenError``) when the - stored key for the token's ``kid`` is unusable as an HMAC secret. + stored key for the token's ``kid`` is unusable as an HMAC secret. An + unloadable keyring also raises ``InvalidTokenError``, so every token + is refused until the keyring is fixed. """ # PyJWT parses the header strictly: a malformed token or a non-string # ``kid`` raises InvalidTokenError here, before any key lookup. kid = jwt.get_unverified_header(token).get("kid") - signing_key = get_signing_key_for_kid(kid) + try: + signing_key = get_signing_key_for_kid(kid) + except SigningKeyUnavailableError as exc: + # The api logs the cause at startup and on each sign-in attempt. + raise jwt.InvalidTokenError("JWT keyring is unavailable") from exc if signing_key is None: raise jwt.InvalidTokenError("Unknown signing key id") return jwt.decode(token, signing_key, algorithms=[ALGORITHM]) diff --git a/backend/main.py b/backend/main.py index 83ba491cf..e2ba7420c 100644 --- a/backend/main.py +++ b/backend/main.py @@ -124,7 +124,7 @@ def _build_redirect_url(self, request: Request) -> str: # Import models to register them with SQLModel from backend.core.db import async_session_maker -from backend.core.security import get_signing_keyring +from backend.core.security import SigningKeyUnavailableError, get_signing_keyring from backend.models.user import User from backend.seed_demo import seed_demo_data from backend.services.model_preparation import enqueue_model_preparation @@ -254,11 +254,40 @@ def log_recordings_storage_warnings(*, logger_instance: logging.Logger) -> None: ) +SIGNING_KEY_UNAVAILABLE_DETAIL = ( + "Sign-in is unavailable because the server could not load its JWT signing " + "key. The api log names the file to fix." +) + + +def log_signing_keyring_status(*, logger_instance: logging.Logger) -> None: + """Load the JWT keyring at boot, so an unusable one is reported at once. + + Without this the first evidence is a failed sign-in. The API still starts, + as with the recordings storage probe: sign-in and token verification fail + closed until the keyring is fixed, and the message names the file. + """ + try: + get_signing_keyring() + except SigningKeyUnavailableError as e: + logger_instance.error("%s Sign-in is unavailable until this is fixed.", e) + + +async def signing_key_unavailable_handler( + request: Request, exc: Exception +) -> JSONResponse: + # 500, not 503: the frontend reads 502-504 as "backend unreachable", while + # here the backend answered and the sign-in page shows the detail. + logger.error("Could not issue a token: %s", exc) + return JSONResponse( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + content={"detail": SIGNING_KEY_UNAVAILABLE_DETAIL}, + ) + + @asynccontextmanager async def lifespan(app: FastAPI): - # Loading the JWT keyring here, not lazily at the first sign-in, means an - # unusable signing key stops startup with a message naming its file. - get_signing_keyring() + log_signing_keyring_status(logger_instance=logger) run_migrations() await ensure_owner_exists() await log_first_run_setup_pointer() @@ -328,6 +357,9 @@ def create_app(*, app_lifespan=lifespan) -> FastAPI: app.add_middleware(TrustedHostMiddleware, allowed_hosts=get_trusted_host_list()) app.add_middleware(NormaliseMcpMountPathMiddleware) + app.add_exception_handler( + SigningKeyUnavailableError, signing_key_unavailable_handler + ) app.include_router(api_router, prefix="/api/v1") if is_mcp_enabled(): # OAuth discovery documents must live at the server root (RFC 8414 / diff --git a/backend/tests/test_jwt_keyring.py b/backend/tests/test_jwt_keyring.py index d33efde16..3dc2358ab 100644 --- a/backend/tests/test_jwt_keyring.py +++ b/backend/tests/test_jwt_keyring.py @@ -5,7 +5,6 @@ import jwt import pytest -from backend import main from backend.core import security @@ -108,16 +107,29 @@ class _StubPathManager: security.rotate_signing_key() -@pytest.fixture -def anyio_backend() -> str: - return "asyncio" +def _empty_active_keyring(directory) -> None: + (directory / ".secret_keys.json").write_text( + json.dumps({"active": "legacy", "keys": {"legacy": ""}}), encoding="utf-8" + ) + + +def _session_token() -> str: + return security.create_access_token( + "alice", + token_type=security.SESSION_TOKEN_TYPE, + scopes=[security.WEB_SESSION_SCOPE], + expires_delta=timedelta(minutes=5), + token_version=0, + ) def test_empty_legacy_secret_key_is_refused_with_the_file_named(isolated_keyring): legacy_file = isolated_keyring / ".secret_key" legacy_file.write_text("\n", encoding="utf-8") - with pytest.raises(RuntimeError, match=re.escape(str(legacy_file))): + with pytest.raises( + security.SigningKeyUnavailableError, match=re.escape(str(legacy_file)) + ): security.get_signing_keyring() # Nothing is persisted, so deleting the empty file and restarting @@ -129,30 +141,35 @@ def test_empty_legacy_secret_key_is_refused_with_the_file_named(isolated_keyring def test_empty_active_keyring_key_is_refused_with_the_file_named(isolated_keyring): + _empty_active_keyring(isolated_keyring) keyring_file = isolated_keyring / ".secret_keys.json" - keyring_file.write_text( - json.dumps({"active": "legacy", "keys": {"legacy": ""}}), encoding="utf-8" - ) - with pytest.raises(RuntimeError, match=re.escape(str(keyring_file))): - security.create_access_token( - "alice", - token_type=security.SESSION_TOKEN_TYPE, - scopes=[security.WEB_SESSION_SCOPE], - token_version=0, - ) + with pytest.raises( + security.SigningKeyUnavailableError, match=re.escape(str(keyring_file)) + ): + _session_token() -@pytest.mark.anyio -async def test_api_startup_refuses_an_empty_signing_key(isolated_keyring, monkeypatch): - (isolated_keyring / ".secret_keys.json").write_text( - json.dumps({"active": "legacy", "keys": {"legacy": ""}}), encoding="utf-8" +@pytest.mark.parametrize("content", ["", " \n"], ids=["zero-byte", "whitespace"]) +def test_blank_keyring_file_gets_the_empty_key_remedy(isolated_keyring, content): + keyring_file = isolated_keyring / ".secret_keys.json" + keyring_file.write_text(content, encoding="utf-8") + + with pytest.raises(security.SigningKeyUnavailableError) as excinfo: + security.get_signing_keyring() + + assert str(excinfo.value).startswith( + f"The JWT signing key in {keyring_file} is empty" ) - migrations: list[bool] = [] - monkeypatch.setattr(main, "run_migrations", lambda: migrations.append(True)) + assert f"Delete {keyring_file} and restart" in str(excinfo.value) - with pytest.raises(RuntimeError, match="is empty"): - async with main.lifespan(main.app): - pass - assert migrations == [] +def test_rotation_replaces_an_empty_active_key(isolated_keyring): + _empty_active_keyring(isolated_keyring) + + new_kid = security.rotate_signing_key() + + active_kid, active_key = security.get_active_signing_key() + assert active_kid == new_kid + assert len(active_key) == 64 + assert security.decode_access_token(_session_token())["sub"] == "alice" diff --git a/backend/tests/test_jwt_keyring_unavailable.py b/backend/tests/test_jwt_keyring_unavailable.py new file mode 100644 index 000000000..bb6c5c415 --- /dev/null +++ b/backend/tests/test_jwt_keyring_unavailable.py @@ -0,0 +1,135 @@ +"""An unloadable JWT keyring is reported at startup without stopping the api, +and sign-in and token verification fail closed with an actionable error.""" + +from __future__ import annotations + +import json +import logging +from datetime import timedelta +from types import SimpleNamespace + +import pytest +from httpx import ASGITransport, AsyncClient + +from backend import main +from backend.api.deps import get_db +from backend.api.v1.endpoints import login +from backend.core import security + +TRUSTED_ORIGIN = "https://nojoin.example.com" + + +class _StartupContinued(Exception): + """Raised by the first stubbed startup step that would reach Postgres.""" + + +@pytest.fixture +def anyio_backend() -> str: + return "asyncio" + + +@pytest.fixture +def keyring_file(monkeypatch, tmp_path): + """A keyring inside ``tmp_path`` whose active key is empty.""" + monkeypatch.delenv("SECRET_KEY", raising=False) + + class _StubPathManager: + user_data_directory = tmp_path + + monkeypatch.setattr(security, "path_manager", _StubPathManager()) + path = tmp_path / ".secret_keys.json" + path.write_text( + json.dumps({"active": "legacy", "keys": {"legacy": ""}}), encoding="utf-8" + ) + return path + + +@pytest.fixture +async def client(monkeypatch): + monkeypatch.setenv("WEB_APP_URL", TRUSTED_ORIGIN) + + async def no_rate_limit(*args, **kwargs): + return None + + async def alice(*args, **kwargs): + return SimpleNamespace( + username="alice", + token_version=0, + force_password_change=False, + is_superuser=False, + ) + + async def no_database(): + # Any use of this object fails, so a request that gets past token + # verification errors instead of reaching a database. + yield object() + + monkeypatch.setattr(login, "enforce_rate_limit", no_rate_limit) + monkeypatch.setattr(login, "_authenticate_user_credentials", alice) + app = main.create_app(app_lifespan=None) + app.dependency_overrides[get_db] = no_database + async with AsyncClient( + transport=ASGITransport(app=app), base_url=TRUSTED_ORIGIN + ) as async_client: + yield async_client + + +@pytest.mark.anyio +async def test_api_startup_logs_an_empty_signing_key_and_carries_on( + keyring_file, monkeypatch, caplog +): + def stop_before_the_database(): + raise _StartupContinued + + monkeypatch.setattr(main, "run_migrations", stop_before_the_database) + + with caplog.at_level(logging.ERROR, logger=main.logger.name): + with pytest.raises(_StartupContinued): + async with main.lifespan(main.app): + pass + + messages = [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + assert any( + f"The JWT signing key in {keyring_file} is empty" in m + and f"Delete {keyring_file} and restart" in m + for m in messages + ), messages + + +@pytest.mark.anyio +async def test_sign_in_with_an_empty_signing_key_fails_with_an_actionable_error( + keyring_file, client, caplog +): + with caplog.at_level(logging.ERROR, logger=main.logger.name): + response = await client.post( + "/api/v1/login/session", + data={"username": "alice", "password": "irrelevant"}, + headers={"Origin": TRUSTED_ORIGIN}, + ) + + assert response.status_code == 500 + assert response.json() == {"detail": main.SIGNING_KEY_UNAVAILABLE_DETAIL} + assert "access_token" not in response.cookies + # The client sees no server path; the log names the file to delete. + assert str(keyring_file) not in response.text + assert any(str(keyring_file) in r.getMessage() for r in caplog.records) + + +@pytest.mark.anyio +async def test_tokens_are_401_while_the_signing_key_is_empty(keyring_file, client): + keyring_file.unlink() + token = security.create_access_token( + "alice", + token_type=security.SESSION_TOKEN_TYPE, + scopes=[security.WEB_SESSION_SCOPE], + expires_delta=timedelta(minutes=5), + token_version=0, + ) + # A write cut short leaves the keyring blank. + keyring_file.write_text("", encoding="utf-8") + + response = await client.get( + "/api/v1/users/me", headers={"Authorization": f"Bearer {token}"} + ) + + assert response.status_code == 401 diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index fa47d035d..92d0b6444 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -363,7 +363,7 @@ The compose stack derives internal service URLs for PostgreSQL, Redis, and Celer Keep any secrets, private mounts, or machine-specific overrides in your local `docker-compose.yml`, not in the tracked template. Every variable below is only read by a container if that container's compose service passes it in, which the shipped `docker-compose.example.yml` does for all of them. If you maintain your own compose file, a variable you set in `.env` but never declare in a service's `environment:` block is silently discarded: the stack starts and the application default applies with no warning. -Nojoin auto-generates and persists its JWT signing keyring under `data/.secret_keys.json` in the default deployment, migrating any legacy `data/.secret_key` file on startup, so no `.env` setting is required for that. If that key file is empty, the api refuses to start and logs which file to delete; see [JWT Signing Key Rotation](SECURITY.md#jwt-signing-key-rotation). +Nojoin auto-generates and persists its JWT signing keyring under `data/.secret_keys.json` in the default deployment, migrating any legacy `data/.secret_key` file on startup, so no `.env` setting is required for that. If that key file is empty or unreadable, the api still starts but nobody can sign in, and the api log names the file to fix; see [JWT Signing Key Rotation](SECURITY.md#jwt-signing-key-rotation). Nojoin can also auto-generate `data/.data_encryption_key`, but operators should treat that as a fallback rather than the primary persistence strategy. ### Always Set diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 05907c73f..7f9833399 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -82,7 +82,7 @@ JWT signing material is stored as a small keyring rather than a single static va - After enough time has passed for outstanding tokens to expire, `prune_signing_keys()` removes retired keys from the keyring. Any token still signed by a removed key fails verification immediately, providing a hard cut-over. - Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. Use a random value carrying at least 256 bits, for example `openssl rand -hex 32` (a 64-character hex string, the same form as a generated keyring key): RFC 7518 Section 3.2 requires an HS256 key at least as long as its 256-bit hash output. A shorter value still works, so existing deployments keep signing in, but PyJWT emits an `InsecureKeyLengthWarning` when it signs or verifies with it. - Existing single-key installs are migrated automatically: the legacy `/.secret_key` file is loaded into the keyring as `kid="legacy"` on first startup and the legacy file is renamed. -- The api loads the keyring at startup and refuses to start if the signing key is empty (an empty legacy `.secret_key`, or an empty active entry in `.secret_keys.json`, for example after a write to a full disk). The error names the file: delete it and restart to generate a new key. Everyone signed in has to sign in again, which is the right outcome, since an empty HMAC key would let anyone forge a token. +- The api loads the keyring at startup. PyJWT cannot sign or verify with an empty key, so if the active key is empty (an empty legacy `.secret_key`, an empty active entry in `.secret_keys.json`, or a blank `.secret_keys.json` left by a write that was cut short, for example by a full disk) or the keyring is malformed or unreadable, the api still starts but logs an error that names the file and the fix. For an empty or malformed key the fix is to delete the named file and restart, which generates a new key; everyone signed in has to sign in again. Until it is fixed, sign-in fails with a message pointing to the api log and every token is rejected. ## Browser Capture Security From bb2a3383fec66bcee8ea80f0f590c3f3b2f31ad3 Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:06:05 -0400 Subject: [PATCH 08/14] docs(deployment): add an upgrade note for an empty JWT signing key --- docs/DEPLOYMENT.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 92d0b6444..baed5760e 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -795,6 +795,7 @@ Pinning a deployment to an exact image digest (`ghcr.io/valtora/nojoin-api@sha25 chmod -R 700 ./data ``` If you have special host-integration requirements that require group or world read access, you can configure a custom umask using the `NOJOIN_UMASK` environment variable (e.g. `NOJOIN_UMASK=0022` or `NOJOIN_UMASK=0002`). +- **Empty JWT signing key:** PyJWT, which signs and verifies sign-in tokens, cannot use an empty key. If the api log shows "The JWT signing key in … is empty", delete the file it names under `data/` (`.secret_keys.json` or `.secret_key`) and restart the api; a new key is generated and everyone signs in again. Until then sign-in fails. Installs with a usable key need no action. ### One-Time Migrations From Pre-Browser-Capture Releases The notes below describe one-time migrations that run automatically when you first upgrade across the relevant cutover. They apply only if your database or installation predates that cutover. On a clean install, or on any installation already past these cutovers, they require no action and can be treated as historical context. From f27d8d0682868fd7bc2d34f4b6eac648c6b031a3 Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:42:04 -0400 Subject: [PATCH 09/14] fix(oauth): mint the access token before spending a code or rotating a refresh token The token endpoint committed the spent authorization code, or the rotated refresh token, before it signed the access token. When the JWT keyring cannot be loaded, signing fails after that commit: the client gets a 500 and never sees the new refresh token, and its retry is treated as reuse of a rotated token, which revokes the whole grant (or, for a code, as a replayed code). Every connected MCP client then has to be reconnected once the keyring is fixed. Both grants now sign the access token right after loading the user, before any state changes, so a failure leaves the code or refresh token usable and the client's retry succeeds once the key is back. The endpoint returns that failure in the RFC 6749 error shape (server_error, 500, no-store) like its other errors, and logs the message that names the key file. --- backend/api/services/oauth_service.py | 20 +++-- backend/api/v1/endpoints/oauth.py | 14 ++++ backend/tests/test_oauth_mcp_connector.py | 89 +++++++++++++++++++++++ 3 files changed, 115 insertions(+), 8 deletions(-) diff --git a/backend/api/services/oauth_service.py b/backend/api/services/oauth_service.py index 27e7b2c51..8a5c8fa38 100644 --- a/backend/api/services/oauth_service.py +++ b/backend/api/services/oauth_service.py @@ -383,10 +383,15 @@ async def exchange_authorization_code( if not code_verifier or not _verify_pkce(record.code_challenge, code_verifier): raise OAuthError("invalid_grant", "PKCE verification failed.") - record.used_at = utc_now() user = await _load_user(db, record.user_id) - grant_id = uuid.uuid4().hex + # Mint before spending the code: if signing fails, the client can retry + # with the same code instead of having it refused as a replay. + access_token, expires_in = _build_access_token( + user, client_id=client_id, scope=record.scope, grant_id=grant_id + ) + + record.used_at = utc_now() refresh_token = await _issue_refresh_token( db, RefreshGrant( @@ -400,9 +405,6 @@ async def exchange_authorization_code( client.last_used_at = utc_now() await db.commit() - access_token, expires_in = _build_access_token( - user, client_id=client_id, scope=record.scope, grant_id=grant_id - ) return { "access_token": access_token, "token_type": "Bearer", @@ -445,6 +447,11 @@ async def refresh_access_token( raise OAuthError("invalid_grant", "Refresh token has expired.") user = await _load_user(db, record.user_id) + # Mint before rotating: if signing fails, the presented refresh token + # stays valid, so a retry is not mistaken for reuse of a rotated token. + access_token, expires_in = _build_access_token( + user, client_id=client_id, scope=record.scope, grant_id=record.grant_id + ) record.revoked_at = utc_now() record.last_used_at = utc_now() @@ -461,9 +468,6 @@ async def refresh_access_token( client.last_used_at = utc_now() await db.commit() - access_token, expires_in = _build_access_token( - user, client_id=client_id, scope=record.scope, grant_id=record.grant_id - ) return { "access_token": access_token, "token_type": "Bearer", diff --git a/backend/api/v1/endpoints/oauth.py b/backend/api/v1/endpoints/oauth.py index a0985ea9b..ea1613662 100644 --- a/backend/api/v1/endpoints/oauth.py +++ b/backend/api/v1/endpoints/oauth.py @@ -25,6 +25,7 @@ from backend.api.deps import get_current_user, get_db from backend.api.services import oauth_service from backend.api.services.oauth_service import OAuthError +from backend.core.security import SigningKeyUnavailableError from backend.models.user import User from backend.utils.config_manager import is_mcp_enabled from backend.utils.rate_limit import enforce_rate_limit @@ -271,6 +272,19 @@ async def token_endpoint( ) except OAuthError as exc: return _oauth_error_response(exc) + except SigningKeyUnavailableError as exc: + # Nothing was spent or rotated, so the client can retry the same + # grant once the keyring is fixed. The message names the key file, + # so it goes to the log, not to the client. + logger.error("Could not issue an OAuth access token: %s", exc) + return _oauth_error_response( + OAuthError( + "server_error", + "The server could not load its JWT signing key, so no access " + "token can be issued. Try again later.", + 500, + ) + ) return JSONResponse(content=payload, headers={"Cache-Control": "no-store"}) diff --git a/backend/tests/test_oauth_mcp_connector.py b/backend/tests/test_oauth_mcp_connector.py index 916e4fa6c..d4eb12ab1 100644 --- a/backend/tests/test_oauth_mcp_connector.py +++ b/backend/tests/test_oauth_mcp_connector.py @@ -4,6 +4,7 @@ from __future__ import annotations import hashlib +import logging from base64 import urlsafe_b64encode import pytest @@ -478,6 +479,94 @@ async def test_refresh_rotation_and_reuse_revocation( assert family_member.status_code == 400 +def assert_server_error_without_paths(response, data_dir) -> None: + assert response.status_code == 500 + assert response.headers["cache-control"] == "no-store" + body = response.json() + assert body["error"] == "server_error" + assert body["error_description"] + assert str(data_dir) not in response.text + + +@pytest.mark.anyio +async def test_code_exchange_during_a_keyring_outage_keeps_the_code( + client: AsyncClient, + fixed_origin, + isolated_keyring, + anonymous_rate_limit_fallback, +): + client_id = await register_claude_client(client) + verifier, challenge = make_pkce_pair() + code = await obtain_code(client, client_id, challenge) + exchange = { + "grant_type": "authorization_code", + "client_id": client_id, + "code": code, + "redirect_uri": CLAUDE_CALLBACK, + "code_verifier": verifier, + } + keyring_file = isolated_keyring / ".secret_keys.json" + # A write cut short leaves the keyring blank. + keyring_file.write_text("", encoding="utf-8") + + during = await client.post("/api/v1/oauth/token", data=exchange) + assert_server_error_without_paths(during, isolated_keyring) + + # The operator deletes the blank file, so a new key is generated, and + # the client's retry with the same code goes through. + keyring_file.unlink() + after = await client.post("/api/v1/oauth/token", data=exchange) + assert after.status_code == 200, after.text + assert after.json()["refresh_token"] + + +@pytest.mark.anyio +async def test_refresh_during_a_keyring_outage_keeps_the_refresh_token( + client: AsyncClient, + fixed_origin, + isolated_keyring, + caplog, + anonymous_rate_limit_fallback, +): + client_id = await register_claude_client(client) + verifier, challenge = make_pkce_pair() + code = await obtain_code(client, client_id, challenge) + first = ( + await client.post( + "/api/v1/oauth/token", + data={ + "grant_type": "authorization_code", + "client_id": client_id, + "code": code, + "redirect_uri": CLAUDE_CALLBACK, + "code_verifier": verifier, + }, + ) + ).json() + refresh = { + "grant_type": "refresh_token", + "client_id": client_id, + "refresh_token": first["refresh_token"], + } + keyring_file = isolated_keyring / ".secret_keys.json" + saved = keyring_file.read_text(encoding="utf-8") + keyring_file.write_text("", encoding="utf-8") + + with caplog.at_level(logging.WARNING): + during = await client.post("/api/v1/oauth/token", data=refresh) + assert_server_error_without_paths(during, isolated_keyring) + assert any(str(keyring_file) in r.getMessage() for r in caplog.records) + + # The same key is restored, and the client retries the same token. + keyring_file.write_text(saved, encoding="utf-8") + after = await client.post("/api/v1/oauth/token", data=refresh) + + assert after.status_code == 200, after.text + assert after.json()["refresh_token"] != first["refresh_token"] + assert not any("reuse detected" in r.getMessage() for r in caplog.records) + assert len((await client.get("/api/v1/oauth/grants")).json()) == 1 + + @pytest.mark.anyio async def test_grants_listing_and_revocation( client: AsyncClient, fixed_origin, isolated_keyring From 9dda16ed81707821c99390d861407f6548527700 Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:42:19 -0400 Subject: [PATCH 10/14] test(auth): sign the unusable-key case with the regular test key --- backend/tests/test_jwt_verification.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/tests/test_jwt_verification.py b/backend/tests/test_jwt_verification.py index 1364b157d..a8d26f193 100644 --- a/backend/tests/test_jwt_verification.py +++ b/backend/tests/test_jwt_verification.py @@ -339,7 +339,9 @@ async def test_unusable_stored_key_is_refused_as_a_401(jose_keyring): encoding="utf-8", ) header = {"alg": "HS256", "kid": "k_empty", "typ": "JWT"} - token = _hs256_token(header, _claims(security.SESSION_TOKEN_TYPE), b"") + token = _hs256_token( + header, _claims(security.SESSION_TOKEN_TYPE), JOSE_KEY.encode() + ) with pytest.raises(jwt.InvalidKeyError): security.decode_access_token(token) From 7c9ce77b59cee84a188e9e6b8edc586906b16976 Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:45:34 -0400 Subject: [PATCH 11/14] fix(auth): keep the startup keyring check from stopping the api The check caught only SigningKeyUnavailableError, so any other failure while loading the keyring aborted startup, unlike the recordings storage probe it follows. It now logs any other exception with its traceback and carries on. The keyring file's existence check also moves inside the error handling, so a data directory the api cannot traverse is reported as an unavailable keyring that names the file, instead of a raw PermissionError. --- backend/core/security.py | 4 ++-- backend/main.py | 5 ++++ backend/tests/test_jwt_keyring.py | 21 ++++++++++++++++ backend/tests/test_jwt_keyring_unavailable.py | 24 +++++++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/backend/core/security.py b/backend/core/security.py index b3c1bd176..59c3841f4 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -64,9 +64,9 @@ class SigningKeyUnavailableError(RuntimeError): def _read_keyring_file() -> Optional[dict[str, Any]]: keyring_file = _keyring_path() - if not keyring_file.exists(): - return None try: + if not keyring_file.exists(): + return None content = keyring_file.read_text(encoding="utf-8") except OSError as exc: raise _keyring_access_error(exc) from exc diff --git a/backend/main.py b/backend/main.py index e2ba7420c..ba7bb80e4 100644 --- a/backend/main.py +++ b/backend/main.py @@ -271,6 +271,11 @@ def log_signing_keyring_status(*, logger_instance: logging.Logger) -> None: get_signing_keyring() except SigningKeyUnavailableError as e: logger_instance.error("%s Sign-in is unavailable until this is fixed.", e) + except Exception: + # Boundary: like the storage probe, this check must not block startup. + logger_instance.exception( + "The JWT keyring check failed to run, so sign-in may be unavailable." + ) async def signing_key_unavailable_handler( diff --git a/backend/tests/test_jwt_keyring.py b/backend/tests/test_jwt_keyring.py index 3dc2358ab..9a5d91072 100644 --- a/backend/tests/test_jwt_keyring.py +++ b/backend/tests/test_jwt_keyring.py @@ -1,6 +1,7 @@ import json import re from datetime import timedelta +from pathlib import Path import jwt import pytest @@ -173,3 +174,23 @@ def test_rotation_replaces_an_empty_active_key(isolated_keyring): assert active_kid == new_kid assert len(active_key) == 64 assert security.decode_access_token(_session_token())["sub"] == "alice" + + +def test_unreadable_data_directory_is_reported_with_the_file_named( + isolated_keyring, monkeypatch +): + keyring_file = isolated_keyring / ".secret_keys.json" + real_exists = Path.exists + + def exists(path: Path, **kwargs) -> bool: + # What stat() raises when the api cannot traverse the data directory. + if path == keyring_file: + raise PermissionError(13, "Permission denied", str(path)) + return real_exists(path, **kwargs) + + monkeypatch.setattr(Path, "exists", exists) + + with pytest.raises( + security.SigningKeyUnavailableError, match=re.escape(str(keyring_file)) + ): + security.get_signing_keyring() diff --git a/backend/tests/test_jwt_keyring_unavailable.py b/backend/tests/test_jwt_keyring_unavailable.py index bb6c5c415..62e76c5ae 100644 --- a/backend/tests/test_jwt_keyring_unavailable.py +++ b/backend/tests/test_jwt_keyring_unavailable.py @@ -96,6 +96,30 @@ def stop_before_the_database(): ), messages +@pytest.mark.anyio +async def test_api_startup_carries_on_when_the_keyring_check_fails_unexpectedly( + monkeypatch, caplog +): + def unexpected_failure(): + raise RuntimeError("Could not determine home directory.") + + def stop_before_the_database(): + raise _StartupContinued + + monkeypatch.setattr(main, "get_signing_keyring", unexpected_failure) + monkeypatch.setattr(main, "run_migrations", stop_before_the_database) + + with caplog.at_level(logging.ERROR, logger=main.logger.name): + with pytest.raises(_StartupContinued): + async with main.lifespan(main.app): + pass + + assert any( + "keyring check failed to run" in r.getMessage() and r.exc_info + for r in caplog.records + ), [r.getMessage() for r in caplog.records] + + @pytest.mark.anyio async def test_sign_in_with_an_empty_signing_key_fails_with_an_actionable_error( keyring_file, client, caplog From 7bbac60249056756a866f6a487f7a6df778d7e5f Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:47:38 -0400 Subject: [PATCH 12/14] fix(auth): give each unusable key file an error that names it and its fix A legacy .secret_key that is not UTF-8 was reported as a permissions problem in the data directory, without naming the file, when the fix is to delete it. A key file that is a directory (what Docker leaves when a bind mount names a host file that does not exist) got the same permissions advice, when it has to be removed. An unreadable key file was reported by its directory rather than by name. The legacy file now gets the malformed-file error, which names it and says to delete it; a directory gets its own error naming it and explaining the bind-mount cause; and the access error names the key file and the directory the api must be able to read and write. SECURITY.md lists the fix for each case. --- backend/core/security.py | 52 ++++++++++++++++++++++--------- backend/tests/test_jwt_keyring.py | 39 +++++++++++++++++++++-- docs/SECURITY.md | 2 +- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/backend/core/security.py b/backend/core/security.py index 59c3841f4..440b3269e 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -69,23 +69,23 @@ def _read_keyring_file() -> Optional[dict[str, Any]]: return None content = keyring_file.read_text(encoding="utf-8") except OSError as exc: - raise _keyring_access_error(exc) from exc + raise _key_file_access_error(keyring_file, exc) from exc except UnicodeDecodeError as exc: - raise _malformed_keyring_error(keyring_file, str(exc)) from exc + raise _malformed_key_file_error(keyring_file, str(exc)) from exc if not content.strip(): # A write cut short, for example by a full disk, leaves the file blank. raise _empty_signing_key_error(keyring_file) try: data = json.loads(content) except json.JSONDecodeError as exc: - raise _malformed_keyring_error(keyring_file, str(exc)) from exc + raise _malformed_key_file_error(keyring_file, str(exc)) from exc if ( not isinstance(data, dict) or not isinstance(data.get("keys"), dict) or not isinstance(data.get("active"), str) or data["active"] not in data["keys"] ): - raise _malformed_keyring_error(keyring_file, "no usable active key entry") + raise _malformed_key_file_error(keyring_file, "no usable active key entry") return data @@ -106,21 +106,31 @@ def _empty_signing_key_error(key_file: Path) -> SigningKeyUnavailableError: ) -def _malformed_keyring_error( - keyring_file: Path, reason: str +def _malformed_key_file_error( + key_file: Path, reason: str ) -> SigningKeyUnavailableError: return SigningKeyUnavailableError( - f"The JWT keyring at {keyring_file} is malformed ({reason}), so no token " - f"can be signed or verified. {_replace_key_file_remedy(keyring_file)}" + f"The JWT key file {key_file} is malformed ({reason}), so no token can " + f"be signed or verified. {_replace_key_file_remedy(key_file)}" ) -def _keyring_access_error(exc: Exception) -> SigningKeyUnavailableError: - data_dir = _keyring_path().parent +def _key_file_access_error( + key_file: Path, exc: Union[OSError, UnicodeDecodeError] +) -> SigningKeyUnavailableError: + if isinstance(exc, IsADirectoryError): + return SigningKeyUnavailableError( + f"The JWT key file {key_file} is a directory, so no token can be " + "signed or verified. A Docker bind mount of a host file that does " + "not exist creates a directory like this. Remove the directory and " + "any bind mount of that file (the key lives in the data directory), " + "then restart Nojoin to generate a new key (everyone signed in will " + "have to sign in again), or set the SECRET_KEY environment variable." + ) return SigningKeyUnavailableError( - f"Unable to read or create the JWT keyring in {data_dir} ({exc}), so no " + f"Unable to read or write the JWT key file {key_file} ({exc}), so no " f"token can be signed or verified. Make sure the api can read and write " - f"{data_dir}, or set the SECRET_KEY environment variable." + f"{key_file.parent}, or set the SECRET_KEY environment variable." ) @@ -140,12 +150,23 @@ def _write_keyring_file(data: dict[str, Any]) -> None: ) +def _read_legacy_key(legacy_file: Path) -> Optional[str]: + try: + if not legacy_file.exists(): + return None + return legacy_file.read_text(encoding="utf-8").strip() + except OSError as exc: + raise _key_file_access_error(legacy_file, exc) from exc + except UnicodeDecodeError as exc: + raise _malformed_key_file_error(legacy_file, str(exc)) from exc + + def _bootstrap_keyring() -> dict[str, Any]: legacy_file = _legacy_key_path() _migrate_legacy_secret_file(legacy_file) - if legacy_file.exists(): - legacy_value = legacy_file.read_text(encoding="utf-8").strip() + legacy_value = _read_legacy_key(legacy_file) + if legacy_value is not None: if not legacy_value: raise _empty_signing_key_error(legacy_file) data = { @@ -189,7 +210,8 @@ def _load_keyring() -> dict[str, Any]: try: keyring = _bootstrap_keyring() except (OSError, UnicodeDecodeError) as exc: - raise _keyring_access_error(exc) from exc + # Writing the new keyring, or migrating a desktop-era key file. + raise _key_file_access_error(_keyring_path(), exc) from exc # Checked here rather than in _read_keyring_file so that # rotate_signing_key() can still replace an empty active key. if not keyring["keys"][keyring["active"]]: diff --git a/backend/tests/test_jwt_keyring.py b/backend/tests/test_jwt_keyring.py index 9a5d91072..8b40a3764 100644 --- a/backend/tests/test_jwt_keyring.py +++ b/backend/tests/test_jwt_keyring.py @@ -190,7 +190,40 @@ def exists(path: Path, **kwargs) -> bool: monkeypatch.setattr(Path, "exists", exists) - with pytest.raises( - security.SigningKeyUnavailableError, match=re.escape(str(keyring_file)) - ): + with pytest.raises(security.SigningKeyUnavailableError) as excinfo: security.get_signing_keyring() + + message = str(excinfo.value) + assert message.startswith( + f"Unable to read or write the JWT key file {keyring_file}" + ) + assert f"Make sure the api can read and write {isolated_keyring}" in message + + +def test_legacy_key_file_that_is_not_utf8_gets_the_delete_remedy(isolated_keyring): + legacy_file = isolated_keyring / ".secret_key" + legacy_file.write_bytes(b"\xff\xfe not text") + + with pytest.raises(security.SigningKeyUnavailableError) as excinfo: + security.get_signing_keyring() + + message = str(excinfo.value) + assert message.startswith(f"The JWT key file {legacy_file} is malformed") + assert f"Delete {legacy_file} and restart" in message + assert not (isolated_keyring / ".secret_keys.json").exists() + + +@pytest.mark.parametrize("file_name", [".secret_keys.json", ".secret_key"]) +def test_key_file_that_is_a_directory_gets_the_remove_remedy( + isolated_keyring, file_name +): + key_path = isolated_keyring / file_name + key_path.mkdir() + + with pytest.raises(security.SigningKeyUnavailableError) as excinfo: + security.get_signing_keyring() + + message = str(excinfo.value) + assert message.startswith(f"The JWT key file {key_path} is a directory") + assert "bind mount" in message + assert "Remove the directory" in message diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 7f9833399..0c0999999 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -82,7 +82,7 @@ JWT signing material is stored as a small keyring rather than a single static va - After enough time has passed for outstanding tokens to expire, `prune_signing_keys()` removes retired keys from the keyring. Any token still signed by a removed key fails verification immediately, providing a hard cut-over. - Setting the `SECRET_KEY` environment variable overrides the keyring with a single static key (intended for advanced deployments and tests). In that mode the rotation API is disabled. Use a random value carrying at least 256 bits, for example `openssl rand -hex 32` (a 64-character hex string, the same form as a generated keyring key): RFC 7518 Section 3.2 requires an HS256 key at least as long as its 256-bit hash output. A shorter value still works, so existing deployments keep signing in, but PyJWT emits an `InsecureKeyLengthWarning` when it signs or verifies with it. - Existing single-key installs are migrated automatically: the legacy `/.secret_key` file is loaded into the keyring as `kid="legacy"` on first startup and the legacy file is renamed. -- The api loads the keyring at startup. PyJWT cannot sign or verify with an empty key, so if the active key is empty (an empty legacy `.secret_key`, an empty active entry in `.secret_keys.json`, or a blank `.secret_keys.json` left by a write that was cut short, for example by a full disk) or the keyring is malformed or unreadable, the api still starts but logs an error that names the file and the fix. For an empty or malformed key the fix is to delete the named file and restart, which generates a new key; everyone signed in has to sign in again. Until it is fixed, sign-in fails with a message pointing to the api log and every token is rejected. +- The api loads the keyring at startup. PyJWT cannot sign or verify with an empty key, so if the active key is empty (an empty legacy `.secret_key`, an empty active entry in `.secret_keys.json`, or a blank `.secret_keys.json` left by a write that was cut short, for example by a full disk) or a key file is malformed, unreadable or a directory, the api still starts but logs an error that names the file and the fix. For an empty or malformed key the fix is to delete the named file and restart, which generates a new key; everyone signed in has to sign in again. For an unreadable file, the fix is to give the api read and write access to the data directory. A key file that is a directory, usually left by a Docker bind mount of a host file that did not exist, has to be removed together with that bind mount before restarting. Until it is fixed, sign-in fails with a message pointing to the api log and every token is rejected. ## Browser Capture Security From 55159ed82820c5b8c23a8308718a83891c8a1ce1 Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:48:08 -0400 Subject: [PATCH 13/14] fix(auth): treat a non-string keyring key as a malformed keyring A hand-edited .secret_keys.json whose key value is a number, boolean, list or null loaded without an error, then failed every sign-in and every authenticated request with a bare TypeError, and nothing was logged at startup. Such a keyring is now malformed: the startup check logs the error naming the file, sign-in gets the actionable 500 and tokens are 401s. --- backend/core/security.py | 2 ++ backend/tests/test_jwt_keyring.py | 18 ++++++++++++++ backend/tests/test_jwt_keyring_unavailable.py | 24 +++++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/backend/core/security.py b/backend/core/security.py index 440b3269e..56afc1275 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -86,6 +86,8 @@ def _read_keyring_file() -> Optional[dict[str, Any]]: or data["active"] not in data["keys"] ): raise _malformed_key_file_error(keyring_file, "no usable active key entry") + if not all(isinstance(key, str) for key in data["keys"].values()): + raise _malformed_key_file_error(keyring_file, "a key that is not a string") return data diff --git a/backend/tests/test_jwt_keyring.py b/backend/tests/test_jwt_keyring.py index 8b40a3764..96c3e82d9 100644 --- a/backend/tests/test_jwt_keyring.py +++ b/backend/tests/test_jwt_keyring.py @@ -227,3 +227,21 @@ def test_key_file_that_is_a_directory_gets_the_remove_remedy( assert message.startswith(f"The JWT key file {key_path} is a directory") assert "bind mount" in message assert "Remove the directory" in message + + +@pytest.mark.parametrize("value", [123, True, ["k"], None], ids=repr) +@pytest.mark.parametrize("kid", ["a", "b"], ids=["active", "retired"]) +def test_non_string_keyring_key_is_refused_with_the_file_named( + isolated_keyring, kid, value +): + keyring_file = isolated_keyring / ".secret_keys.json" + keys = {"a": "a" * 64, "b": "b" * 64} + keys[kid] = value + keyring_file.write_text(json.dumps({"active": "a", "keys": keys}), encoding="utf-8") + + with pytest.raises(security.SigningKeyUnavailableError) as excinfo: + security.get_signing_keyring() + + message = str(excinfo.value) + assert message.startswith(f"The JWT key file {keyring_file} is malformed") + assert f"Delete {keyring_file} and restart" in message diff --git a/backend/tests/test_jwt_keyring_unavailable.py b/backend/tests/test_jwt_keyring_unavailable.py index 62e76c5ae..f5a0dbe95 100644 --- a/backend/tests/test_jwt_keyring_unavailable.py +++ b/backend/tests/test_jwt_keyring_unavailable.py @@ -96,6 +96,30 @@ def stop_before_the_database(): ), messages +@pytest.mark.anyio +async def test_api_startup_logs_a_non_string_signing_key( + keyring_file, monkeypatch, caplog +): + keyring_file.write_text( + json.dumps({"active": "legacy", "keys": {"legacy": 123}}), encoding="utf-8" + ) + + def stop_before_the_database(): + raise _StartupContinued + + monkeypatch.setattr(main, "run_migrations", stop_before_the_database) + + with caplog.at_level(logging.ERROR, logger=main.logger.name): + with pytest.raises(_StartupContinued): + async with main.lifespan(main.app): + pass + + messages = [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + assert any( + f"The JWT key file {keyring_file} is malformed" in m for m in messages + ), messages + + @pytest.mark.anyio async def test_api_startup_carries_on_when_the_keyring_check_fails_unexpectedly( monkeypatch, caplog From 76162a0e04ab5ddcbefd56b036b41f34a3bc908a Mon Sep 17 00:00:00 2001 From: Mike Mulligan <130580+thebeline@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:48:18 -0400 Subject: [PATCH 14/14] docs(auth): trim the signing key upgrade note and scope the rotation comment --- backend/core/security.py | 2 +- docs/DEPLOYMENT.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/core/security.py b/backend/core/security.py index 56afc1275..edcfffb7b 100644 --- a/backend/core/security.py +++ b/backend/core/security.py @@ -215,7 +215,7 @@ def _load_keyring() -> dict[str, Any]: # Writing the new keyring, or migrating a desktop-era key file. raise _key_file_access_error(_keyring_path(), exc) from exc # Checked here rather than in _read_keyring_file so that - # rotate_signing_key() can still replace an empty active key. + # rotate_signing_key() can still replace an empty active entry. if not keyring["keys"][keyring["active"]]: raise _empty_signing_key_error(_keyring_path()) return keyring diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index baed5760e..f139a6fba 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -795,7 +795,7 @@ Pinning a deployment to an exact image digest (`ghcr.io/valtora/nojoin-api@sha25 chmod -R 700 ./data ``` If you have special host-integration requirements that require group or world read access, you can configure a custom umask using the `NOJOIN_UMASK` environment variable (e.g. `NOJOIN_UMASK=0022` or `NOJOIN_UMASK=0002`). -- **Empty JWT signing key:** PyJWT, which signs and verifies sign-in tokens, cannot use an empty key. If the api log shows "The JWT signing key in … is empty", delete the file it names under `data/` (`.secret_keys.json` or `.secret_key`) and restart the api; a new key is generated and everyone signs in again. Until then sign-in fails. Installs with a usable key need no action. +- **Empty JWT signing key:** if the api log shows "The JWT signing key in … is empty", delete the file it names under `data/` (`.secret_keys.json` or `.secret_key`) and restart the api; a new key is generated and everyone signs in again. ### One-Time Migrations From Pre-Browser-Capture Releases The notes below describe one-time migrations that run automatically when you first upgrade across the relevant cutover. They apply only if your database or installation predates that cutover. On a clean install, or on any installation already past these cutovers, they require no action and can be treated as historical context.