fix: security and settings bug batch (#517, #532, #533, #543, #544, #545, #547, #548)

CHANGELOG.md

@@ -12,6 +12,30 @@ All notable changes to this project are documented in this file. The format is b
 ## [Unreleased]
 
 ### Changed
+- **Behaviour change (#543):** with `allow_signup` off, an OAuth sign-in no
+  longer creates an account. It answers 403 `OAUTH_SIGNUP_DISABLED` unless the
+  identity is already linked or its email matches an existing account, which
+  still link and sign in.
+- **Behaviour change (#547):** `users.require_verification` is honoured. With
+  it off, no verification email is sent, the register page says so, and
+  password login no longer requires a verified email — including for accounts
+  registered while it was on. Such accounts stay unverified, and an OAuth
+  sign-in whose email matches an unverified password account is refused with
+  403 `OAUTH_ACCOUNT_NOT_VERIFIED` instead of linking to it (an account already
+  linked to that OAuth identity still signs in). With it on,
+  `POST /api/users/auth/token` now refuses an unverified account with 400
+  `LOGIN_USER_NOT_VERIFIED`, as the session login already did; it used to mint
+  bearer tokens for one.
+- **Behaviour change (#544):** `file_storage` callers without
+  `file_storage.manage` (granted to admins and to `tenant:admin`/`tenant:owner`)
+  see, download, change and delete only files they uploaded; anyone else's
+  file is a 404. Files with no recorded uploader are visible to managers only.
+- **Behaviour change (#545):** `file_storage` serves a stored type inline only
+  from an allow-list (raster images, PDF, JSON/GeoJSON, plain text, CSV, opaque
+  binaries, audio/video/font). Everything else, any `*+xml` and any malformed
+  value included, is a sandboxed attachment.
+- `POST /api/users/auth/token` returns `token_type="Bearer"`, and every bearer
+  resolver (users, keycloak, api_tokens) accepts the scheme in any case (#533).
 - **Behaviour change (#486):** for settings classes that read the environment
   (today `background_tasks`, `SM_BG_TASKS_*`), an env var now beats a value
   stored from the admin UI, and `SM_REDIS_URL` beats a stored broker/result
@@ -312,6 +336,16 @@ All notable changes to this project are documented in this file. The format is b
   `tenants` resolver it selects among the user's own memberships.
 
 ### Fixed
+- A runtime permission source that raises is no longer cached as empty until
+  invalidated: it contributes nothing for 30 seconds (one logged traceback),
+  then is retried; `invalidate_source` retries it at once (#517).
+- `background_tasks`: a prefork Celery child no longer reuses the sync engine
+  (and Postgres connections) built in the parent (#532).
+- Settings validation: `file_storage.quota_bytes` must be positive (and at most
+  2^53-1), and `background_tasks.task_time_limit` must exceed
+  `task_soft_time_limit` when both are set (#548). With the hard limit set by
+  env, an offending stored soft limit is dropped (and logged) at boot instead
+  of discarding every background_tasks override and stopping the worker.
 - Settings editor: list/dict fields are edited and stored as JSON, and
   `X | None` fields as their real type with empty meaning unset (#487).
   Secrets inside such fields are stored intact, and any field declared with

docs/modules/file_storage.md

@@ -78,10 +78,17 @@ Serving rules:
   `If-None-Match`) and `X-Content-Type-Options: nosniff`.
 - Every public response carries `Content-Security-Policy: default-src 'none';
   style-src 'unsafe-inline'; sandbox` (the security-headers middleware now
-  keeps a CSP the response already set). Active content — HTML, XHTML, SVG,
-  XML, JavaScript — is forced to `Content-Disposition: attachment` and is
-  always streamed, so stored XSS on the app origin is not possible. Other
-  types are `inline`.
+  keeps a CSP the response already set). Only an allow-list of types is
+  served `inline` (and redirected on S3): common raster images, PDF, JSON,
+  GeoJSON, plain text, CSV, opaque binaries (`application/octet-stream`, zip,
+  gzip, protobuf / vector tiles) and `audio/*` / `video/*` / `font/*` minus
+  any XML or HTML subtype
+  (`INLINE_SAFE_CONTENT_TYPES` / `INLINE_SAFE_FAMILIES`). Everything else —
+  HTML, SVG, any XML, scripts, unknown types, and malformed values such as
+  `text/html,` — is forced to `Content-Disposition: attachment` and always
+  streamed, so stored XSS on the app origin is not possible. The stored type
+  is lower-cased and stripped of parameters before the check
+  (`media_types.is_active_content`).
 - Presigning backends (S3) answer with a `302` to the presigned URL, cached
   for at most half the signature's TTL; filesystem backends stream.
 
@@ -311,14 +318,24 @@ Register it in your module's `register_settings` so the file_storage service can
 | `file_storage.upload` | `user`, `admin`, `tenant:member`/`admin`/`owner` | upload files |
 | `file_storage.download` | `user`, `admin`, `tenant:member`/`admin`/`owner` | list / get / download |
 | `file_storage.delete` | `admin`, `tenant:admin`, `tenant:owner`; also `user` when `multi_tenant` is off | delete |
-| `file_storage.manage` | `admin` | reserved for future admin operations |
+| `file_storage.manage` | `admin`, `tenant:admin`, `tenant:owner` | act on every file in the active tenant, not only one's own |
 
 With `multi_tenant` on, delete is an organisation-admin act: the platform
 `user` role, which every account holds, does not carry it (it would hand it to
 every tenant member). With `multi_tenant` off the install is the only tenant,
 so `on_startup` maps `file_storage.delete` onto `user` and ordinary users keep
 deleting their files.
 
+**Ownership.** Every authenticated route — list, get, download, thumbnail,
+publish/unpublish, delete, bulk delete and the browse screen — acts only on the
+files the caller uploaded (`created_by` is their user id), unless they hold
+`file_storage.manage`. Someone else's file answers `404`, exactly like an
+unknown id, and the browse screen's type and uploader filters list only the
+caller's own rows (the usage figure stays bucket-wide, as the quota is). The
+scope is passed explicitly by the endpoints (`deps.get_owner_scope`) to the
+service's `owner=` argument; internal callers such as branding and reapers
+pass none and stay unscoped, and anonymous `public` serving is unaffected.
+
 ## Menu
 
 | Label | URL | Icon | Section | Group | Order | Roles |

framework/core/simple_module_core/permissions.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import logging
+import time
 from collections.abc import Awaitable, Callable, Collection, Iterable
 from dataclasses import dataclass, field
 from typing import Any
@@ -95,7 +96,11 @@ class PermissionRegistry:
     every mutation.
     """
 
-    def __init__(self) -> None:
+    #: Seconds a failed source is answered as empty before it is called again.
+    SOURCE_RETRY_SECONDS = 30.0
+
+    def __init__(self, *, clock: Callable[[], float] = time.monotonic) -> None:
+        self._clock = clock
         self._groups: dict[str, PermissionGroup] = {}
         self._role_map: dict[str, set[str]] = {}
         # Admin-editor grants (DB-backed). Kept apart from ``_role_map`` (code
@@ -106,6 +111,8 @@ def __init__(self) -> None:
         self._grant_sources: list[GrantSource] = []
         self._sources: dict[str, PermissionSourceProvider] = {}
         self._source_cache: dict[str, tuple[list[str], dict[str, str]]] = {}
+        self._source_failed_at: dict[str, float] = {}  # name → clock() of last failure
+        self._all_permissions_expires: float | None = None
 
     def _invalidate(self) -> None:
         self._all_permissions_cache = None
@@ -120,21 +127,28 @@ def add_source(self, name: str, provider: PermissionSourceProvider) -> None:
         is **sync** and should read a module-maintained in-memory cache — the
         registry is consulted on every request. Its output is cached here until
         :meth:`invalidate_source` is called. A provider that raises is logged
-        and contributes nothing.
+        once and contributes nothing for ``SOURCE_RETRY_SECONDS``, then is
+        retried — a transient failure recovers on its own (GH #517).
         """
         self._sources[name] = provider
         self._source_cache.pop(name, None)
+        self._source_failed_at.pop(name, None)
         self._invalidate()
 
     def invalidate_source(self, name: str) -> None:
-        """Drop the cached output of source *name*; re-read on next access."""
+        """Drop the cached output (or failure) of source *name*; re-read on next access."""
         self._source_cache.pop(name, None)
+        self._source_failed_at.pop(name, None)
         self._invalidate()
 
     def _source_output(self, name: str) -> tuple[list[str], dict[str, str]]:
         cached = self._source_cache.get(name)
         if cached is not None:
             return cached
+        failed_at = self._source_failed_at.get(name)
+        if failed_at is not None and self._clock() - failed_at < self.SOURCE_RETRY_SECONDS:
+            # In the retry window: empty, uncalled, no traceback per request.
+            return [], {}
         keys: list[str] = []
         labels: dict[str, str] = {}
         try:
@@ -146,8 +160,12 @@ def _source_output(self, name: str) -> tuple[list[str], dict[str, str]]:
                     keys.append(key)
                     labels[key] = label
         except Exception:
-            logger.exception("Permission source %r failed; contributing nothing", name)
-            keys, labels = [], {}
+            logger.exception("Permission source %r failed; contributing nothing until retry", name)
+            # Remembered for the retry window only: a transient failure must
+            # not hide the source until someone invalidates it.
+            self._source_failed_at[name] = self._clock()
+            return [], {}
+        self._source_failed_at.pop(name, None)
         out = (sorted(set(keys)), labels)
         self._source_cache[name] = out
         return out
@@ -176,11 +194,18 @@ def add(self, permission: str) -> None:
     @property
     def all_permissions(self) -> list[str]:
         """All registered permission strings, sorted."""
-        if self._all_permissions_cache is None:
+        expires = self._all_permissions_expires
+        if self._all_permissions_cache is None or (expires and self._clock() >= expires):
             perms: set[str] = set()
             for group in self.groups:
                 perms.update(group.permissions)
             self._all_permissions_cache = sorted(perms)
+            # A snapshot taken while a source is failing lives only until that
+            # source's retry, so a recovery shows up without an invalidation.
+            failed = self._source_failed_at.values()
+            self._all_permissions_expires = (
+                min(failed) + self.SOURCE_RETRY_SECONDS if failed else None
+            )
         return self._all_permissions_cache
 
     @property

framework/core/tests/test_permission_sources.py

@@ -50,3 +50,84 @@ def test_admin_implicit_all_includes_sources() -> None:
     reg = PermissionRegistry()
     reg.add_source("records", lambda: ["records.product.edit"])
     assert "records.product.edit" in reg.get_permissions_for_roles(["admin"])
+
+
+class _Clock:
+    def __init__(self) -> None:
+        self.now = 1000.0
+
+    def __call__(self) -> float:
+        return self.now
+
+
+def test_failed_source_recovers_after_the_retry_window() -> None:
+    calls = {"n": 0}
+
+    def flaky() -> list[str]:
+        calls["n"] += 1
+        if calls["n"] == 1:
+            raise RuntimeError("db not ready")
+        return ["flaky.read"]
+
+    clock = _Clock()
+    reg = PermissionRegistry(clock=clock)
+    reg.add_source("flaky", flaky)
+    assert reg.all_permissions == []
+    assert not reg.has("flaky.read")  # inside the window: not called again
+    assert calls["n"] == 1
+    clock.now += PermissionRegistry.SOURCE_RETRY_SECONDS
+    assert reg.has("flaky.read")  # no invalidate_source needed
+    assert "flaky.read" in reg.all_permissions
+    assert calls["n"] == 2
+
+
+def test_permanent_failure_logs_once_per_window(caplog) -> None:
+    calls = {"n": 0}
+
+    def broken() -> list[str]:
+        calls["n"] += 1
+        raise RuntimeError("misconfigured")
+
+    clock = _Clock()
+    reg = PermissionRegistry(clock=clock)
+    reg.add_source("broken", broken)
+    with caplog.at_level(logging.ERROR):
+        for _ in range(50):
+            assert reg.all_permissions == []
+        assert (calls["n"], len(caplog.records)) == (1, 1)
+        assert caplog.records[0].exc_info is not None
+        clock.now += PermissionRegistry.SOURCE_RETRY_SECONDS + 1
+        for _ in range(50):
+            assert reg.all_permissions == []
+    assert (calls["n"], len(caplog.records)) == (2, 2)
+
+
+def test_invalidate_source_retries_a_failed_source_at_once() -> None:
+    state = {"ok": False}
+
+    def source() -> list[str]:
+        if not state["ok"]:
+            raise RuntimeError("not yet")
+        return ["late.read"]
+
+    reg = PermissionRegistry(clock=_Clock())
+    reg.add_source("late", source)
+    assert reg.all_permissions == []
+    state["ok"] = True
+    reg.invalidate_source("late")
+    assert reg.all_permissions == ["late.read"]
+
+
+def test_snapshot_is_reused_while_a_source_is_failing() -> None:
+    def broken() -> list[str]:
+        raise RuntimeError("down")
+
+    clock = _Clock()
+    reg = PermissionRegistry(clock=clock)
+    reg.add("static.read")
+    reg.add_source("broken", broken)
+    first = reg.all_permissions
+    assert first == ["static.read"]
+    assert reg.all_permissions is first  # not rebuilt on every has()
+    clock.now += PermissionRegistry.SOURCE_RETRY_SECONDS
+    assert reg.all_permissions is not first  # rebuilt once the retry is due

modules/api_tokens/api_tokens/resolver.py

@@ -18,7 +18,7 @@
 from typing import Any
 
 from auth.contracts.schemas import UserContext
-from auth.contracts.tokens import AUTH_KIND_PAT, AUTH_KIND_SERVICE, is_api_token
+from auth.contracts.tokens import AUTH_KIND_PAT, AUTH_KIND_SERVICE, is_api_token, parse_bearer
 from simple_module_core.tenancy import is_tenant_role
 from simple_module_db import ALL_TENANTS_OPTION
 from sqlalchemy import or_, select, update
@@ -42,11 +42,8 @@
 
 
 def _bearer(request: Request) -> str | None:
-    header = request.headers.get("authorization", "")
-    if not header.startswith("Bearer "):
-        return None
-    credential = header[7:]
-    if not is_api_token(credential) or not is_well_formed(credential):
+    credential = parse_bearer(request.headers.get("authorization"))
+    if credential is None or not is_api_token(credential) or not is_well_formed(credential):
         return None
     return credential
 

modules/api_tokens/tests/test_api_tokens_resolver.py

@@ -5,6 +5,7 @@
 import uuid
 from datetime import UTC, datetime, timedelta
 
+import pytest
 from _api_tokens_support import FLAGS_URL, create_pat, load_token, update_token
 from api_tokens.constants import TokenKind
 from api_tokens.tokens import generate
@@ -21,6 +22,16 @@ async def test_valid_pat_reads_but_cannot_write(actor, bearer):
     assert put.status_code == 403
 
 
+@pytest.mark.parametrize("scheme", ["Bearer", "bearer", "BEARER"])
+async def test_scheme_is_case_insensitive(actor, bearer, scheme):
+    """GH #533: RFC 9110 auth schemes are case-insensitive."""
+    async with actor(admin=True) as a:
+        token = (await create_pat(a.client, ["feature_flags.view"]))["token"]
+
+    resp = await bearer("GET", FLAGS_URL, token, headers={"Authorization": f"{scheme} {token}"})
+    assert resp.status_code == 200
+
+
 async def test_revoke_takes_effect_on_the_next_request(actor, bearer):
     async with actor(admin=True) as a:
         created = await create_pat(a.client, ["feature_flags.view"])

modules/auth/auth/contracts/tokens.py

@@ -31,6 +31,22 @@
 API_TOKEN_PREFIXES: tuple[str, ...] = (PAT_PREFIX, SERVICE_KEY_PREFIX)
 
 
+def parse_bearer(header: str | None) -> str | None:
+    """The credential of an ``Authorization: Bearer <credential>`` header, else None.
+
+    The auth scheme is case-insensitive (RFC 9110 §11.1): clients that build
+    the header from ``/auth/token``'s ``token_type`` send ``bearer``, and must
+    authenticate exactly like ``Bearer``. Surrounding and separating whitespace
+    is tolerated; a header with no credential, or a different scheme, is None.
+    """
+    if not header:
+        return None
+    parts = header.split(None, 1)
+    if len(parts) != 2 or parts[0].lower() != "bearer":
+        return None
+    return parts[1].strip() or None
+
+
 def is_api_token(credential: str) -> bool:
     """Whether a bearer value is an ``api_tokens`` credential (by prefix only)."""
     return credential.startswith(API_TOKEN_PREFIXES)
@@ -45,4 +61,5 @@ def is_api_token(credential: str) -> bool:
     "PAT_PREFIX",
     "SERVICE_KEY_PREFIX",
     "is_api_token",
+    "parse_bearer",
 ]

modules/auth/tests/test_auth_parse_bearer.py

@@ -0,0 +1,21 @@
+"""``parse_bearer``: the one place an ``Authorization: Bearer`` header is read (GH #533)."""
+
+from __future__ import annotations
+
+import pytest
+from auth.contracts.tokens import parse_bearer
+
+
+@pytest.mark.parametrize(
+    "header",
+    ["Bearer abc", "bearer abc", "BEARER abc", "BeArEr abc", "  Bearer   abc  ", "bearer\tabc"],
+)
+def test_scheme_is_case_insensitive_and_whitespace_tolerant(header):
+    assert parse_bearer(header) == "abc"
+
+
+@pytest.mark.parametrize(
+    "header", [None, "", "Bearer", "Bearer   ", "Basic abc", "Bearerabc", "Token abc"]
+)
+def test_anything_else_is_not_a_bearer_credential(header):
+    assert parse_bearer(header) is None