From 105a44ee3ce1fa0ffa3db8d8528b5b847bd6c1bb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 16:52:52 +0200 Subject: [PATCH 01/12] feat(hosting): groundwork for batch 1 - request hooks mixin, CORS and rate-limit registries, new host settings, middleware slots Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CLAUDE.md | 4 +- docs/framework/middleware.md | 5 + .../2026-10-21-hosting-batch-1-design.md | 86 ++++++++++++++ framework/core/simple_module_core/__init__.py | 3 + .../core/simple_module_core/_request_hooks.py | 103 ++++++++++++++++ framework/core/simple_module_core/cors.py | 71 +++++++++++ .../simple_module_core/diagnostics/_module.py | 2 + framework/core/simple_module_core/module.py | 54 +-------- .../core/simple_module_core/path_rules.py | 110 ++++++++++++++++++ .../core/simple_module_core/rate_limit.py | 56 ++++++++- .../core/tests/test_path_rule_registries.py | 70 +++++++++++ .../simple_module_hosting/_phase_helpers.py | 11 +- .../simple_module_hosting/_registrations.py | 29 +++++ .../simple_module_hosting/app_builder.py | 12 +- .../simple_module_hosting/host_settings.py | 39 +++++++ .../tests/test_build_time_settings_marked.py | 20 +++- 16 files changed, 615 insertions(+), 60 deletions(-) create mode 100644 docs/superpowers/specs/2026-10-21-hosting-batch-1-design.md create mode 100644 framework/core/simple_module_core/_request_hooks.py create mode 100644 framework/core/simple_module_core/cors.py create mode 100644 framework/core/simple_module_core/path_rules.py create mode 100644 framework/core/tests/test_path_rule_registries.py diff --git a/CLAUDE.md b/CLAUDE.md index fdc1e9204..e4a1516ae 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -209,8 +209,8 @@ every Tailwind utility, so keep tokens in `theme.css` and rules in `styles.css` `docs/framework/permissions.md`. **Middleware** (Starlette `add_middleware` is LIFO). Execution order per -request: `(ProxyHeaders if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → -BodyLimit → GZip → SecurityHeaders → Session → → Tenant +request: `(ForwardedHost if trusted proxy + allowlist) → (ProxyHeaders if +SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → CORS → BodyLimit → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → RateLimit → Setup → Maintenance → CommitBeforeResponse → app`. Rules that follow from it: diff --git a/docs/framework/middleware.md b/docs/framework/middleware.md index 1515c7779..55fc9d588 100644 --- a/docs/framework/middleware.md +++ b/docs/framework/middleware.md @@ -31,18 +31,23 @@ app.add_middleware(CorrelationIdMiddleware) if settings.trusted_proxy: app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=...) +# ForwardedHost is added after ProxyHeaders so it runs first (still sees the proxy peer) # Added last → executed first (outermost wrapper) ``` ## Execution order (per request) ``` +ForwardedHost (only if SM_TRUSTED_PROXY and a forwarded-host allowlist are set) + ↓ ProxyHeaders (only if SM_TRUSTED_PROXY is set) ↓ CorrelationId ↓ RequestLogging ↓ +CORS (preflights answered here, before auth / rate limit) + ↓ BodyLimit (413 on an oversized body) ↓ GZip diff --git a/docs/superpowers/specs/2026-10-21-hosting-batch-1-design.md b/docs/superpowers/specs/2026-10-21-hosting-batch-1-design.md new file mode 100644 index 000000000..8a8ae2707 --- /dev/null +++ b/docs/superpowers/specs/2026-10-21-hosting-batch-1-design.md @@ -0,0 +1,86 @@ +# Hosting feature batch 1: CORS, forwarded host, per-route rate limits, DB timeouts, OpenAPI security + +Approved 2026-10-21 for #527, #528, #529, #531, #534. Defaults below were accepted. + +## Shared (S0) + +- Move the request-related `register_*` hooks out of `framework/core/simple_module_core/module.py` + (299 lines) into a `_request_hooks.py` mixin; add no-op `register_cors_policies(registry)` and + `register_rate_limits(registry)`. +- Core registries `CorsPolicyRegistry` and `RateLimitRegistry` (exact / prefix / regex matching, + reusing the `PublicRoute` matcher as `BodyLimitRegistry` does), wired through + `_registrations.py` / `app_builder.py` (≤300 lines) onto `app.state`. +- All new `HostSettings` fields with restart markers (`test_build_time_settings_marked.py`). +- Middleware slots in `install_middleware`; update the order docstring, `test_middleware_order.py`, + CLAUDE.md § Middleware and `docs/framework/middleware.md`. + +## #527 CORS + +- Host settings (restart-required): `cors_allow_origins: list[str] = []` (empty → no API policy), + `cors_allow_credentials: bool = True`, `cors_allow_methods = [GET, POST, PUT, PATCH, DELETE]`, + `cors_allow_headers = [Authorization, Content-Type, X-CSRF-Token]`, `cors_expose_headers = []`, + `cors_max_age: int = 600`; a validator rejects `*` with credentials. Host policy on prefix `/api/`. +- `CorsPolicy` frozen (origins, credentials, methods, headers, expose, max_age). Most specific match + wins (exact, then regex/suffix, then longest prefix; ties keep the first and warn at boot). +- `hosting/_cors.py` dispatches to one pre-built Starlette `CORSMiddleware` per policy. +- Order: ProxyHeaders → ForwardedHost → CorrelationId → RequestLogging → **CORS** → BodyLimit → … + (preflights never reach auth / rate limit / setup / maintenance). +- file_storage registers a policy for its public file path: origin `*`, no credentials, + GET/HEAD/OPTIONS, allow `Range`/`If-Range`/`If-None-Match`, expose + `Content-Range`/`Accept-Ranges`/`ETag`/`Content-Length`; `enabled=` reads a DB-backed module + setting `public_files_cors: bool = True` (no restart). Document S3 presign redirects needing + bucket CORS. + +## #528 Forwarded host + +- Host setting `forwarded_host_allowlist: list[str] = []` (exact host or `*.example.com`, optional + port), restart-required; middleware installed only when `trusted_proxy` and the allowlist are set. +- Runs before ProxyHeaders sees the peer (added after it); own `ipaddress` trust check mirroring + uvicorn's `*`/IP/CIDR rule. Leftmost `X-Forwarded-Host`; on match rewrite `host` header and + `scope["server"]`; otherwise ignore with a debug log. `Forwarded:` (RFC 7239) unsupported. +- Docs: tenant subdomains need `*.` in the allowlist; email links keep using + configured base URLs. + +## #529 Per-route rate limits + +- Core `RateLimitRule(rate, key: Literal["user","ip","user_or_ip","tenant"] = "user_or_ip", + methods=None)`; hook `register_rate_limits(registry)`; rules apply to anonymous and signed-in + requests before the existing public tiers. +- `RateLimit("10/minute", key="user")` FastAPI dependency using the shared store on `app.state.sm`; + 429 with `Retry-After`. +- Key resolution in `_rate_limit_keys.py`: user = principal id (token principal → owner); ip = + `scope.client`; user_or_ip; tenant = `state.tenant_id`, falling back to user_or_ip. + Buckets namespaced `rule::::=`. +- Behaviour change: `rate_limit_authenticated` keyed by user, not IP (off by default). + +## #531 DB timeouts + +- Host settings (restart-required, env `SM_DB_*` still wins): `db_pool_timeout: int = 30`, + `db_connect_timeout: int = 10`, `db_statement_timeout_ms: int = 0`, + `db_idle_in_tx_timeout_ms: int = 0` (0 = off). +- `simple_module_db/engine_options.py`: asyncpg `{"timeout": c, "server_settings": {...}}`; + psycopg2 `{"connect_timeout": c, "options": "-c statement_timeout=… -c + idle_in_transaction_session_timeout=…"}`; SQLite nothing. Never applied to Alembic or the pre-app + read. +- Worker: `SM_BG_TASKS_DB_POOL_SIZE` (2), `SM_BG_TASKS_DB_MAX_OVERFLOW` (3), + `SM_BG_TASKS_DB_RECYCLE` (1800), optional `SM_BG_TASKS_DB_STATEMENT_TIMEOUT_MS`; the outbox async + engine gets the same options. Document PgBouncer transaction mode rejecting startup params. + +## #534 OpenAPI security + +- `components.securitySchemes`: `bearerAuth` (http bearer) and `sessionCookie` (apiKey, cookie + `session`). Operations: `security: []` when public (PublicRouteRegistry match on the templated + path, legacy provider public paths, the openapi URL); otherwise + `[{bearerAuth: []}, {sessionCookie: []}]`. +- Doctor SM031 (WARNING): an `/api` route with no permission/allow gate and no public match; + `Authenticated()` marker (new gate kind, still refuses tokens where appropriate) silences it. + Runs where a built app is available (dev boot; check `make doctor`). + +## Tests (acceptance) + +Preflight from an allowlisted origin succeeds with credentials and never 401s; non-allowlisted +origin gets no CORS headers; cross-origin Range GET on a public file → 206 with exposed +`Content-Range`; forged forwarded host ignored, allowlisted one used by `url_for`; two users on one +IP get separate 10/minute budgets, anonymous falls back to IP; timeouts produce the right connect +args per driver (Postgres `pg_sleep` test marked `postgres`); exported schema lists security per +operation; SM031 fires and is silenced by the marker. diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index baa9f3cc2..97735eab9 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -2,6 +2,7 @@ from simple_module_core.audit_links import AuditLink, AuditLinkRegistry, LabelResolver from simple_module_core.body_limits import BodyLimitRegistry +from simple_module_core.cors import CorsPolicy, CorsPolicyRegistry from simple_module_core.csp import CspSourceError, CspSourceRegistry from simple_module_core.design_packs import DesignPack, DesignPackRegistry from simple_module_core.diagnostics import ( @@ -66,6 +67,8 @@ "AuditLinkRegistry", "BodyLimitRegistry", "CircularDependencyError", + "CorsPolicy", + "CorsPolicyRegistry", "CspSourceError", "CspSourceRegistry", "DesignPack", diff --git a/framework/core/simple_module_core/_request_hooks.py b/framework/core/simple_module_core/_request_hooks.py new file mode 100644 index 000000000..8e1f8a1de --- /dev/null +++ b/framework/core/simple_module_core/_request_hooks.py @@ -0,0 +1,103 @@ +"""Request-pipeline registration hooks, split out of ``ModuleBase``. + +Everything a module declares about how HTTP requests reach it -- anonymous +routes, body ceilings, CSP origins, CORS policies, rate limits, middleware and +exception handlers. ``ModuleBase`` inherits this mixin, so ``from +simple_module_core.module import ModuleBase`` and every override are unchanged. +All hooks are no-ops by default and run once at boot, in dependency order. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from fastapi import FastAPI + + from simple_module_core.body_limits import BodyLimitRegistry + from simple_module_core.cors import CorsPolicyRegistry + from simple_module_core.csp import CspSourceRegistry + from simple_module_core.public_routes import PublicRouteRegistry + from simple_module_core.rate_limit import RateLimitRegistry + + +class RequestHooksMixin: + def register_public_routes(self, registry: PublicRouteRegistry) -> None: + """Declare routes that must bypass authentication (anonymous access). + + ``AuthMiddleware`` gates every request behind the active auth provider. + Override this hook to exempt read-only or webhook routes that are meant + to be reached without a session — e.g. a STAC / OGC API surface:: + + def register_public_routes(self, registry): + registry.add_prefix("/api/gis/stac") + registry.add_regex(r"/api/gis/datasets/[^/]+/tilejson$", methods={"GET"}) + + Rules are method-aware, so a GET read route nested under a prefix that + also carries POST/PATCH mutations can be exempted without opening the + mutations. Called once at boot, in dependency order. + """ + + def register_body_limits(self, registry: BodyLimitRegistry) -> None: + """Per-route request-body ceilings; see docs/framework/request-guards.md.""" + + def register_csp_sources(self, registry: CspSourceRegistry) -> None: + """Declare external origins this module's frontend loads assets from. + + The host ships a strict Content-Security-Policy; a module whose + pages pull a stylesheet, font, or API from another origin must + declare it here or the browser blocks the request:: + + def register_csp_sources(self, registry): + registry.add("style-src", "https://rsms.me") + registry.add("font-src", "https://rsms.me") + + Fetch directives only; invalid declarations raise at boot. Runs once, + *before* DB settings hydrate — for an origin read from settings use + ``registry.add_provider(lambda app: [(directive, source)])`` (sync, + in-memory state only, evaluated per response). + """ + + def register_middleware(self, app: FastAPI) -> None: + """Add middleware to the application. + + Called after core middleware (session, security headers) positioning + is established but before the app starts. + """ + + def register_exception_handlers(self, app: FastAPI) -> None: + """Register custom exception handlers (after the framework's own).""" + + def register_cors_policies(self, registry: CorsPolicyRegistry) -> None: + """Declare cross-origin policies for this module's routes. + + The host already allows its configured origins on ``/api/``; override + this for a surface with different rules, e.g. publicly served files:: + + def register_cors_policies(self, registry): + registry.add_prefix( + "/files/public/", + CorsPolicy(origins=("*",), allow_credentials=False, methods=("GET", "HEAD")), + enabled=lambda: self._public_cors_on(), + ) + + The most specific path match wins (exact, regex, longest prefix); + ``enabled`` is evaluated per request. A wildcard origin cannot be + combined with credentials. Called once at boot, in dependency order. + """ + + def register_rate_limits(self, registry: RateLimitRegistry) -> None: + """Declare per-route rate limits for this module's endpoints. + + :: + + def register_rate_limits(self, registry): + registry.add_exact( + "/api/orders/export", RateLimitRule("5/minute", key="user") + ) + + ``key`` picks who shares a budget: the signed-in user, the client IP, + ``user_or_ip`` (user when signed in, else IP) or the active tenant. + The most specific path match wins. Called once at boot, in dependency + order. + """ diff --git a/framework/core/simple_module_core/cors.py b/framework/core/simple_module_core/cors.py new file mode 100644 index 000000000..7f753d4ae --- /dev/null +++ b/framework/core/simple_module_core/cors.py @@ -0,0 +1,71 @@ +"""CORS policy registry: modules declare which origins may call which paths. + +The host registers a policy for ``/api/`` from its ``cors_*`` settings; a module +with a public, cross-origin surface (e.g. publicly served files) adds its own +through :meth:`~simple_module_core.module.ModuleBase.register_cors_policies`:: + + def register_cors_policies(self, registry): + registry.add_prefix( + "/files/public/", + CorsPolicy(origins=("*",), allow_credentials=False, methods=("GET", "HEAD")), + enabled=lambda: app_settings.public_files_cors, + ) + +The most specific match wins (exact, then regex, then longest prefix); a tie +keeps the first registration and logs a warning. ``enabled`` is evaluated per +request, so a DB-backed toggle needs no restart. +""" + +from __future__ import annotations + +from collections.abc import Callable +from dataclasses import dataclass + +from simple_module_core.path_rules import KIND_EXACT, KIND_PREFIX, KIND_REGEX, PathRuleSet + +__all__ = ["CorsPolicy", "CorsPolicyRegistry"] + +WILDCARD = "*" + + +@dataclass(frozen=True, slots=True) +class CorsPolicy: + origins: tuple[str, ...] + allow_credentials: bool = False + methods: tuple[str, ...] = ("GET",) + headers: tuple[str, ...] = () + expose_headers: tuple[str, ...] = () + max_age: int = 600 + + def __post_init__(self) -> None: + if self.allow_credentials and WILDCARD in self.origins: + raise ValueError("A CORS policy cannot combine origin '*' with credentials") + if self.max_age < 0: + raise ValueError("max_age must be >= 0") + + +class CorsPolicyRegistry: + def __init__(self) -> None: + self._rules: PathRuleSet[CorsPolicy] = PathRuleSet("CORS policy") + + def add_exact( + self, path: str, policy: CorsPolicy, *, enabled: Callable[[], bool] | None = None + ) -> None: + self._rules.add(KIND_EXACT, path, policy, enabled=enabled) + + def add_prefix( + self, prefix: str, policy: CorsPolicy, *, enabled: Callable[[], bool] | None = None + ) -> None: + self._rules.add(KIND_PREFIX, prefix, policy, enabled=enabled) + + def add_regex( + self, pattern: str, policy: CorsPolicy, *, enabled: Callable[[], bool] | None = None + ) -> None: + self._rules.add(KIND_REGEX, pattern, policy, enabled=enabled) + + def match(self, method: str, path: str) -> CorsPolicy | None: + """The governing policy for this request, or ``None`` (no CORS headers).""" + return self._rules.match(method, path) + + def __len__(self) -> int: + return len(self._rules) diff --git a/framework/core/simple_module_core/diagnostics/_module.py b/framework/core/simple_module_core/diagnostics/_module.py index 972436304..eac982288 100644 --- a/framework/core/simple_module_core/diagnostics/_module.py +++ b/framework/core/simple_module_core/diagnostics/_module.py @@ -107,6 +107,8 @@ def _check_empty_modules(self, modules: list[ModuleBase]) -> list[Diagnostic]: "register_health_checks", "register_public_routes", "register_body_limits", + "register_cors_policies", + "register_rate_limits", "register_csp_sources", "register_exception_handlers", "register_settings", diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index 115834250..65e413232 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -6,14 +6,13 @@ from pathlib import Path from typing import TYPE_CHECKING +from simple_module_core._request_hooks import RequestHooksMixin from simple_module_core.module_meta import ModuleMeta if TYPE_CHECKING: from fastapi import APIRouter, FastAPI from simple_module_core.audit_links import AuditLinkRegistry - from simple_module_core.body_limits import BodyLimitRegistry - from simple_module_core.csp import CspSourceRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagRegistry @@ -21,11 +20,10 @@ from simple_module_core.invalidation import InvalidationBus from simple_module_core.menu import MenuRegistry from simple_module_core.permissions import PermissionRegistry - from simple_module_core.public_routes import PublicRouteRegistry from simple_module_core.setup_steps import SetupRegistry -class ModuleBase(ABC): +class ModuleBase(RequestHooksMixin, ABC): """Base class for all modules. Subclasses override only the methods they need. @@ -52,6 +50,8 @@ class ModuleBase(ABC): - register_event_handlers(bus) - register_invalidations(bus, app) - register_health_checks(registry) + - request hooks from ``_request_hooks.RequestHooksMixin`` (public routes, + body limits, CORS policies, rate limits, CSP sources) Phase 5: Database - init_db() -> DatabaseState @@ -152,42 +152,6 @@ def register_setup_steps(self, registry: SetupRegistry) -> None: Called once at boot, in dependency order. """ - def register_public_routes(self, registry: PublicRouteRegistry) -> None: - """Declare routes that must bypass authentication (anonymous access). - - ``AuthMiddleware`` gates every request behind the active auth provider. - Override this hook to exempt read-only or webhook routes that are meant - to be reached without a session — e.g. a STAC / OGC API surface:: - - def register_public_routes(self, registry): - registry.add_prefix("/api/gis/stac") - registry.add_regex(r"/api/gis/datasets/[^/]+/tilejson$", methods={"GET"}) - - Rules are method-aware, so a GET read route nested under a prefix that - also carries POST/PATCH mutations can be exempted without opening the - mutations. Called once at boot, in dependency order. - """ - - def register_body_limits(self, registry: BodyLimitRegistry) -> None: - """Per-route request-body ceilings; see docs/framework/request-guards.md.""" - - def register_csp_sources(self, registry: CspSourceRegistry) -> None: - """Declare external origins this module's frontend loads assets from. - - The host ships a strict Content-Security-Policy; a module whose - pages pull a stylesheet, font, or API from another origin must - declare it here or the browser blocks the request:: - - def register_csp_sources(self, registry): - registry.add("style-src", "https://rsms.me") - registry.add("font-src", "https://rsms.me") - - Fetch directives only; invalid declarations raise at boot. Runs once, - *before* DB settings hydrate — for an origin read from settings use - ``registry.add_provider(lambda app: [(directive, source)])`` (sync, - in-memory state only, evaluated per response). - """ - def register_design_packs(self, registry: DesignPackRegistry) -> None: """Declare design packs this module ships for the public site. @@ -230,16 +194,6 @@ def register_audit_links(self, registry): boot, in dependency order. """ - def register_middleware(self, app: FastAPI) -> None: - """Add middleware to the application. - - Called after core middleware (session, security headers) positioning - is established but before the app starts. - """ - - def register_exception_handlers(self, app: FastAPI) -> None: - """Register custom exception handlers (after the framework's own).""" - # ── Asset contribution (templates & static files) ───────── def template_dirs(self) -> list[Path]: diff --git a/framework/core/simple_module_core/path_rules.py b/framework/core/simple_module_core/path_rules.py new file mode 100644 index 000000000..3f9c17db6 --- /dev/null +++ b/framework/core/simple_module_core/path_rules.py @@ -0,0 +1,110 @@ +"""Most-specific-wins path matching shared by the CORS and rate-limit registries. + +``BodyLimitRegistry`` takes the first matching rule; the registries built on this +helper instead pick the *most specific* one, so registration order between +modules (which follows dependency order, not intent) cannot let a broad prefix +shadow a narrow exact path. Specificity is exact > regex > longest prefix. + +Standalone on purpose: ``public_routes`` imports ``rate_limit``, so this cannot +reuse :class:`~simple_module_core.public_routes.PublicRoute` without a cycle. +""" + +from __future__ import annotations + +import logging +import re +from collections.abc import Callable, Iterable +from dataclasses import dataclass, field + +__all__ = ["KIND_EXACT", "KIND_PREFIX", "KIND_REGEX", "PathRuleSet"] + +logger = logging.getLogger(__name__) + +KIND_EXACT = "exact" +KIND_REGEX = "regex" +KIND_PREFIX = "prefix" +_KINDS = (KIND_EXACT, KIND_REGEX, KIND_PREFIX) +# Higher beats lower; prefixes then compare by length. +_RANK = {KIND_PREFIX: 0, KIND_REGEX: 1, KIND_EXACT: 2} + + +@dataclass(slots=True) +class _Entry[T]: + kind: str + pattern: str + value: T + methods: frozenset[str] | None + enabled: Callable[[], bool] | None + regex: re.Pattern[str] | None = field(default=None) + + def matches(self, method: str, path: str) -> bool: + if self.methods is not None and method.upper() not in self.methods: + return False + if self.kind == KIND_EXACT: + return path == self.pattern + if self.kind == KIND_PREFIX: + return path.startswith(self.pattern) + assert self.regex is not None + return self.regex.match(path) is not None + + def specificity(self) -> tuple[int, int]: + return (_RANK[self.kind], len(self.pattern) if self.kind == KIND_PREFIX else 0) + + +class PathRuleSet[T]: + """An ordered set of ``(kind, pattern, methods) -> value`` rules.""" + + def __init__(self, what: str = "rule") -> None: + self._what = what + self._entries: list[_Entry[T]] = [] + + def add( + self, + kind: str, + pattern: str, + value: T, + *, + methods: Iterable[str] | None = None, + enabled: Callable[[], bool] | None = None, + ) -> None: + if kind not in _KINDS: + raise ValueError(f"Unknown match kind {kind!r}; expected one of {_KINDS}") + method_set = None if methods is None else frozenset(m.upper() for m in methods) + entry = _Entry( + kind, + pattern, + value, + method_set, + enabled, + re.compile(pattern) if kind == KIND_REGEX else None, + ) + for other in self._entries: + if other.kind == kind and other.pattern == pattern and _overlap(other, entry): + logger.warning( + "Duplicate %s for %s %r; the first registered wins", + self._what, + kind, + pattern, + ) + break + self._entries.append(entry) + + def match(self, method: str, path: str) -> T | None: + """The value of the most specific enabled rule for *method* + *path*.""" + best: _Entry[T] | None = None + for entry in self._entries: + if not entry.matches(method, path): + continue + if entry.enabled is not None and not entry.enabled(): + continue + # Strict ``>`` keeps the first registered on a tie. + if best is None or entry.specificity() > best.specificity(): + best = entry + return None if best is None else best.value + + def __len__(self) -> int: + return len(self._entries) + + +def _overlap(a: _Entry, b: _Entry) -> bool: + return a.methods is None or b.methods is None or bool(a.methods & b.methods) diff --git a/framework/core/simple_module_core/rate_limit.py b/framework/core/simple_module_core/rate_limit.py index f77aac5a4..27428515d 100644 --- a/framework/core/simple_module_core/rate_limit.py +++ b/framework/core/simple_module_core/rate_limit.py @@ -18,11 +18,15 @@ import time from collections.abc import Callable from dataclasses import dataclass -from typing import Protocol +from typing import Literal, Protocol + +from simple_module_core.path_rules import KIND_EXACT, KIND_PREFIX, KIND_REGEX, PathRuleSet __all__ = [ "InProcessWindowStore", "RateLimitError", + "RateLimitRegistry", + "RateLimitRule", "RateSpec", "WindowResult", "WindowStore", @@ -167,3 +171,53 @@ def _sweep(self, now: float) -> None: # Everything is live: shed the oldest half rather than grow unbounded. keep = sorted(self._windows.items(), key=lambda kv: kv[1][0], reverse=True) self._windows = dict(keep[: self._MAX_KEYS // 2]) + + +RateKey = Literal["user", "ip", "user_or_ip", "tenant"] + + +@dataclass(frozen=True, slots=True) +class RateLimitRule: + """A per-route limit: ``rate`` per ``key``, optionally for some methods only.""" + + rate: str + key: RateKey = "user_or_ip" + methods: frozenset[str] | None = None + + def __post_init__(self) -> None: + if parse_rate(self.rate) is None: + raise RateLimitError("A RateLimitRule needs a real rate, not a disabled one") + if self.methods is not None: + object.__setattr__(self, "methods", frozenset(m.upper() for m in self.methods)) + + +class RateLimitRegistry: + """Per-route rules from ``register_rate_limits``; most specific match wins. + + Exact beats regex beats the longest prefix; a tie keeps the first + registration and logs a warning. ``enabled`` is evaluated per request. + """ + + def __init__(self) -> None: + self._rules: PathRuleSet[RateLimitRule] = PathRuleSet("rate-limit rule") + + def add_exact( + self, path: str, rule: RateLimitRule, *, enabled: Callable[[], bool] | None = None + ) -> None: + self._rules.add(KIND_EXACT, path, rule, methods=rule.methods, enabled=enabled) + + def add_prefix( + self, prefix: str, rule: RateLimitRule, *, enabled: Callable[[], bool] | None = None + ) -> None: + self._rules.add(KIND_PREFIX, prefix, rule, methods=rule.methods, enabled=enabled) + + def add_regex( + self, pattern: str, rule: RateLimitRule, *, enabled: Callable[[], bool] | None = None + ) -> None: + self._rules.add(KIND_REGEX, pattern, rule, methods=rule.methods, enabled=enabled) + + def match(self, method: str, path: str) -> RateLimitRule | None: + return self._rules.match(method, path) + + def __len__(self) -> int: + return len(self._rules) diff --git a/framework/core/tests/test_path_rule_registries.py b/framework/core/tests/test_path_rule_registries.py new file mode 100644 index 000000000..e1d827f8b --- /dev/null +++ b/framework/core/tests/test_path_rule_registries.py @@ -0,0 +1,70 @@ +"""Most-specific-wins matching for the CORS and rate-limit registries.""" + +from __future__ import annotations + +import logging + +import pytest +from simple_module_core.cors import CorsPolicy, CorsPolicyRegistry +from simple_module_core.rate_limit import RateLimitError, RateLimitRegistry, RateLimitRule + + +def _p(origin: str) -> CorsPolicy: + return CorsPolicy(origins=(origin,)) + + +def test_exact_beats_regex_beats_longest_prefix() -> None: + reg = CorsPolicyRegistry() + reg.add_prefix("/api/", _p("short")) + reg.add_prefix("/api/files/", _p("long")) + reg.add_regex(r"/api/files/\d+$", _p("regex")) + reg.add_exact("/api/files/7", _p("exact")) + assert reg.match("GET", "/api/files/7").origins == ("exact",) + assert reg.match("GET", "/api/files/8").origins == ("regex",) + assert reg.match("GET", "/api/files/x").origins == ("long",) + assert reg.match("GET", "/api/other").origins == ("short",) + assert reg.match("GET", "/nope") is None + + +def test_order_does_not_matter_for_specificity() -> None: + reg = CorsPolicyRegistry() + reg.add_exact("/a/b", _p("exact")) + reg.add_prefix("/a/", _p("prefix")) + assert reg.match("GET", "/a/b").origins == ("exact",) + + +def test_tie_keeps_first_and_warns(caplog: pytest.LogCaptureFixture) -> None: + reg = CorsPolicyRegistry() + reg.add_prefix("/x/", _p("first")) + with caplog.at_level(logging.WARNING): + reg.add_prefix("/x/", _p("second")) + assert reg.match("GET", "/x/y").origins == ("first",) + assert "Duplicate" in caplog.text + + +def test_disabled_entry_is_skipped() -> None: + reg = CorsPolicyRegistry() + reg.add_prefix("/a/", _p("base")) + reg.add_exact("/a/b", _p("off"), enabled=lambda: False) + assert reg.match("GET", "/a/b").origins == ("base",) + + +def test_wildcard_with_credentials_rejected() -> None: + with pytest.raises(ValueError): + CorsPolicy(origins=("*",), allow_credentials=True) + + +def test_rate_rules_are_method_aware() -> None: + reg = RateLimitRegistry() + post = RateLimitRule("5/minute", methods=frozenset({"post"})) + any_ = RateLimitRule("100/minute") + reg.add_prefix("/api/orders", any_) + reg.add_exact("/api/orders", post) + assert reg.match("POST", "/api/orders") is post + assert reg.match("GET", "/api/orders") is any_ + assert post.methods == frozenset({"POST"}) + + +def test_rate_rule_rejects_disabled_rate() -> None: + with pytest.raises(RateLimitError): + RateLimitRule("off") diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 06371f87d..2b1783fd7 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -122,7 +122,8 @@ def install_middleware( """Install the full middleware pipeline. Order matters: last added = first executed. Execution order: - (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging + (ForwardedHost, if trusted_proxy and forwarded_host_allowlist) → + (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging → CORS → BodyLimit → GZip → Security → Session → [module] → (Tenant, if multi_tenant) → Locale → Inertia → InertiaCache → RateLimit → Setup → Maintenance → CommitBeforeResponse. @@ -211,6 +212,10 @@ def install_middleware( max_bytes=settings.max_request_body_bytes, registry=getattr(app.state, "body_limits", None), ) + # SLOT: CORS. Add the CORS dispatcher here (batch-1 slice for #527) so it + # executes between RequestLogging and BodyLimit: preflights are answered + # before auth, rate limit, setup and maintenance can reject them. Reads + # ``app.state.cors_policies`` (CorsPolicyRegistry). No-op until then. app.add_middleware(RequestLoggingMiddleware) app.add_middleware(CorrelationIdMiddleware) # Outermost: rewrite scheme/client from X-Forwarded-* before anything else @@ -221,6 +226,10 @@ def install_middleware( # explicit trust setting — never trust forwarded headers by default. if settings.trusted_proxy: app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=settings.trusted_proxy) + # SLOT: ForwardedHost. Added after ProxyHeaders so it executes before it + # (batch-1 slice for #528), while the TCP peer is still the proxy. Install + # only when ``settings.trusted_proxy`` and ``settings.forwarded_host_allowlist`` + # are both set. No-op until then. def attach_public_routes(app: FastAPI, settings: Settings, registry) -> None: diff --git a/framework/hosting/simple_module_hosting/_registrations.py b/framework/hosting/simple_module_hosting/_registrations.py index 5c584e1f7..f5fcdf166 100644 --- a/framework/hosting/simple_module_hosting/_registrations.py +++ b/framework/hosting/simple_module_hosting/_registrations.py @@ -8,12 +8,35 @@ from __future__ import annotations import inspect +from dataclasses import dataclass, field from typing import TYPE_CHECKING +from simple_module_core.body_limits import BodyLimitRegistry +from simple_module_core.cors import CorsPolicyRegistry +from simple_module_core.rate_limit import RateLimitRegistry + if TYPE_CHECKING: from fastapi import FastAPI +@dataclass(slots=True) +class RequestRegistries: + """The request-pipeline registries modules fill via ``_request_hooks``. + + ``attach`` publishes them on ``app.state`` -- ``body_limits``, + ``cors_policies`` and ``rate_limit_rules`` -- where the middleware reads them. + """ + + body_limits: BodyLimitRegistry = field(default_factory=BodyLimitRegistry) + cors: CorsPolicyRegistry = field(default_factory=CorsPolicyRegistry) + rate_limits: RateLimitRegistry = field(default_factory=RateLimitRegistry) + + def attach(self, app: FastAPI) -> None: + app.state.body_limits = self.body_limits + app.state.cors_policies = self.cors + app.state.rate_limit_rules = self.rate_limits + + def run_module_registrations( modules: list, *, @@ -30,6 +53,8 @@ def run_module_registrations( audit_link_registry, csp_registry, body_limit_registry=None, + cors_registry=None, + rate_limit_registry=None, ) -> None: """Invoke each module's registration hooks, in dependency order. @@ -50,6 +75,10 @@ def run_module_registrations( mod.register_public_routes(public_route_registry) if body_limit_registry is not None: mod.register_body_limits(body_limit_registry) + if cors_registry is not None: + mod.register_cors_policies(cors_registry) + if rate_limit_registry is not None: + mod.register_rate_limits(rate_limit_registry) setup_registry.set_owner(mod.meta.name) mod.register_setup_steps(setup_registry) # csp before design packs — the documented lifecycle order diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 943fa2828..a2b245767 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -5,7 +5,7 @@ import logging from fastapi import Depends, FastAPI -from simple_module_core import BodyLimitRegistry, CspSourceRegistry +from simple_module_core import CspSourceRegistry from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.diagnostics import print_diagnostics @@ -43,7 +43,7 @@ from simple_module_hosting._preapp_config import merge_host_settings from simple_module_hosting._project_root import resolve_project_root as _resolve_project_root from simple_module_hosting._rate_limit import build_store -from simple_module_hosting._registrations import run_module_registrations +from simple_module_hosting._registrations import RequestRegistries, run_module_registrations from simple_module_hosting._secret_key import assert_not_placeholder from simple_module_hosting._settings_registration import ( check_settings_registration, @@ -184,7 +184,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: # ── Phase 5: Module registrations ────────────────────── csp_registry = CspSourceRegistry() - body_limit_registry = BodyLimitRegistry() + request_registries = RequestRegistries() run_module_registrations( modules, app=app, @@ -199,9 +199,11 @@ def create_app(settings: Settings | None = None) -> FastAPI: design_pack_registry=design_pack_registry, audit_link_registry=audit_link_registry, csp_registry=csp_registry, - body_limit_registry=body_limit_registry, + body_limit_registry=request_registries.body_limits, + cors_registry=request_registries.cors, + rate_limit_registry=request_registries.rate_limits, ) - app.state.body_limits = body_limit_registry + request_registries.attach(app) attach_public_routes(app, settings, public_route_registry) report_tenant_resolution(app, settings, diagnostics_state) # SM025 diff --git a/framework/hosting/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index 0fdddc47b..a35803723 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/framework/hosting/simple_module_hosting/host_settings.py @@ -18,6 +18,7 @@ from pydantic import Field, field_validator, model_validator from pydantic_core import PydanticCustomError from pydantic_settings import BaseSettings, SettingsConfigDict +from simple_module_core.cors import WILDCARD as CORS_WILDCARD from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER from simple_module_core.stored_settings import RELATED_FIELDS_CTX @@ -28,6 +29,8 @@ _RESTART_GUARD = {"requires_restart": True, "group": "Request guards"} +_RESTART_CORS = {"requires_restart": True, "group": "CORS"} + _RESTART_API = {"requires_restart": True, "group": "API"} # Consumed while create_app builds the tenant middleware, the strict tenant @@ -173,6 +176,24 @@ class HostSettings(BaseSettings): """Opt-in per-IP limit for signed-in traffic, same syntax. Blank (default) leaves authenticated requests unlimited.""" + forwarded_host_allowlist: list[str] = Field(default=[], json_schema_extra=_RESTART_GUARD) + """Hosts a trusted proxy may substitute through ``X-Forwarded-Host``: an exact + host or ``*.example.com``, each with an optional port. Empty (default) ignores + the header. Only honoured together with ``trusted_proxy``.""" + + cors_allow_origins: list[str] = Field(default=[], json_schema_extra=_RESTART_CORS) + """Origins allowed to call ``/api/`` cross-origin. Empty (default) installs no + host API policy. ``*`` is refused while ``cors_allow_credentials`` is on.""" + cors_allow_credentials: bool = Field(default=True, json_schema_extra=_RESTART_CORS) + cors_allow_methods: list[str] = Field( + default=["GET", "POST", "PUT", "PATCH", "DELETE"], json_schema_extra=_RESTART_CORS + ) + cors_allow_headers: list[str] = Field( + default=["Authorization", "Content-Type", "X-CSRF-Token"], json_schema_extra=_RESTART_CORS + ) + cors_expose_headers: list[str] = Field(default=[], json_schema_extra=_RESTART_CORS) + cors_max_age: int = Field(default=600, ge=0, json_schema_extra=_RESTART_CORS) + api_title: str = Field(default="SimpleModule", json_schema_extra=_RESTART_API) """``info.title`` of the OpenAPI document (built once at boot).""" api_version: str = Field(default_factory=_default_api_version, json_schema_extra=_RESTART_API) @@ -186,6 +207,14 @@ class HostSettings(BaseSettings): db_max_overflow: int = Field(default=20, json_schema_extra=_RESTART_DB) db_pool_pre_ping: bool = Field(default=True, json_schema_extra=_RESTART_DB) db_pool_recycle: int = Field(default=1800, json_schema_extra=_RESTART_DB) + db_pool_timeout: int = Field(default=30, ge=1, json_schema_extra=_RESTART_DB) + """Seconds a request waits for a pooled connection before failing.""" + db_connect_timeout: int = Field(default=10, ge=1, json_schema_extra=_RESTART_DB) + """Seconds to wait for a new connection to open (Postgres drivers only).""" + db_statement_timeout_ms: int = Field(default=0, ge=0, json_schema_extra=_RESTART_DB) + """Postgres ``statement_timeout`` in milliseconds; ``0`` disables.""" + db_idle_in_tx_timeout_ms: int = Field(default=0, ge=0, json_schema_extra=_RESTART_DB) + """Postgres ``idle_in_transaction_session_timeout`` in ms; ``0`` disables.""" @field_validator("rate_limit_public", "rate_limit_authenticated", mode="after") @classmethod @@ -242,6 +271,16 @@ def _check_default_tenant(cls, value: str) -> str: raise ValueError(f"default_tenant {value!r} is not a valid tenant id") return value + @model_validator(mode="after") + def _check_cors_wildcard(self) -> HostSettings: + if self.cors_allow_credentials and CORS_WILDCARD in self.cors_allow_origins: + raise PydanticCustomError( + "cors_wildcard_credentials", + "cors_allow_origins '*' cannot be combined with cors_allow_credentials", + {RELATED_FIELDS_CTX: ["cors_allow_origins", "cors_allow_credentials"]}, + ) + return self + @model_validator(mode="after") def _check_default_locale_supported(self) -> HostSettings: if self.i18n_default_locale not in self.i18n_supported_locales: diff --git a/framework/hosting/tests/test_build_time_settings_marked.py b/framework/hosting/tests/test_build_time_settings_marked.py index d18e84ab2..8d6c07516 100644 --- a/framework/hosting/tests/test_build_time_settings_marked.py +++ b/framework/hosting/tests/test_build_time_settings_marked.py @@ -26,6 +26,24 @@ # setup_logging once, before create_app runs. _READ_BY_HOST_ENTRYPOINT = frozenset({"log_level", "log_format"}) +# Declared by hosting batch 1 groundwork (S0) before the feature slices that read +# them in create_app. Each slice deletes its own fields from this set. +_NOT_YET_CONSUMED = frozenset( + { + "forwarded_host_allowlist", + "cors_allow_origins", + "cors_allow_credentials", + "cors_allow_methods", + "cors_allow_headers", + "cors_expose_headers", + "cors_max_age", + "db_pool_timeout", + "db_connect_timeout", + "db_statement_timeout_ms", + "db_idle_in_tx_timeout_ms", + } +) + class _RecordingSettings(Settings): reads: ClassVar[set[str]] = set() @@ -66,7 +84,7 @@ def test_every_build_time_field_is_marked(build_time_fields: set[str]) -> None: def test_every_marked_field_is_read_at_build_time(build_time_fields: set[str]) -> None: - stale = sorted(_marked() - build_time_fields) + stale = sorted(_marked() - build_time_fields - _NOT_YET_CONSUMED) assert not stale, f"marked requires_restart but create_app never reads them: {stale}" From 05b70c8b18b3ccf186a746b57b9c6b3db3682036 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 16:58:50 +0200 Subject: [PATCH 02/12] feat(hosting): per-route rate limits - register_rate_limits rules and RateLimit dependency (#529) rate_limit_authenticated is now keyed per user instead of per IP. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 7 + CLAUDE.md | 9 +- docs/framework/lifecycle.md | 7 + docs/framework/request-guards.md | 34 +++ .../core/simple_module_core/path_rules.py | 7 +- .../core/simple_module_core/rate_limit.py | 5 + .../simple_module_hosting/_rate_limit.py | 209 +++++------------- .../simple_module_hosting/_rate_limit_keys.py | 62 ++++++ .../_rate_limit_store.py | 126 +++++++++++ .../simple_module_hosting/rate_limit_deps.py | 45 ++++ .../tests/test_rate_limit_rules_redis.py | 25 +++ .../tests/test_rate_limit_rules_registry.py | 150 +++++++++++++ 12 files changed, 535 insertions(+), 151 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/_rate_limit_keys.py create mode 100644 framework/hosting/simple_module_hosting/_rate_limit_store.py create mode 100644 framework/hosting/simple_module_hosting/rate_limit_deps.py create mode 100644 framework/hosting/tests/test_rate_limit_rules_redis.py create mode 100644 framework/hosting/tests/test_rate_limit_rules_registry.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 160318854..2f15b7f9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,11 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Added +- Per-route rate limits (#529): modules declare `RateLimitRule(rate, key=...)` in the new + `register_rate_limits(registry)` hook, and handlers use the + `RateLimit("10/minute", key="user")` dependency. Keys: `user`, `ip`, `user_or_ip`, + `tenant`; both apply to anonymous and signed-in requests and answer `429` with + `Retry-After`. - New `mail` module (`simple_module_mail`) owns outgoing mail: the SMTP/console transport, its settings (`/admin/settings/`, package `mail`) and the on-demand `mail.mailer` health check behind "Test mailer connection". Modules send through @@ -39,6 +44,8 @@ All notable changes to this project are documented in this file. The format is b it, which removes the `users` ↔ `audit_log` import cycle. ### Changed +- **Behaviour change:** `rate_limit_authenticated` (off by default) now keeps one budget per + signed-in user instead of one per client IP, so users behind one NAT no longer share it (#529). - `audit_log` and `dashboard` no longer depend on `Users` (meta `depends_on` and package dependency); `background_tasks` drops its unused `Users` dependency (#501). `api_tokens` moves `Users` to `optional_depends_on` and diff --git a/CLAUDE.md b/CLAUDE.md index e4a1516ae..d007fa06b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,7 +162,8 @@ every Tailwind utility, so keep tokens in `theme.css` and rules in `styles.css` `docs/framework/lifecycle.md`): `register_settings`, `register_menu_items`, `register_permissions`, `register_feature_flags`, `register_event_handlers`, `register_invalidations`, `register_health_checks`, `register_setup_steps`, -`register_public_routes`, `register_body_limits`, `register_csp_sources`, +`register_public_routes`, `register_body_limits`, `register_rate_limits`, +`register_csp_sources`, `register_design_packs`, `register_audit_links`, `register_exception_handlers`, `register_middleware`, `register_routes(api_router, view_router)`, `register_admin_routes(admin_router)` @@ -220,7 +221,11 @@ Maintenance → CommitBeforeResponse → app`. Rules that follow from it: page, set `Cache-Control`/ETag on the *document* response only. - `RateLimit` runs after auth, so by default it throttles only anonymous traffic to public routes (`rate_limit_public`; `rate_limit_authenticated` is - off by default; Redis via `SM_REDIS_URL`, else per-worker). + off by default and keyed per user; Redis via `SM_REDIS_URL`, else per-worker). + Per-route budgets for anonymous *and* signed-in traffic: `register_rate_limits` + rules (`RateLimitRule(rate, key="user"|"ip"|"user_or_ip"|"tenant")`) or the + `RateLimit("10/minute", key="user")` dependency from + `simple_module_hosting.rate_limit_deps`. - Request bodies over `max_request_body_bytes` (10 MiB) are refused before any module sees them; per-path overrides via `register_body_limits`. diff --git a/docs/framework/lifecycle.md b/docs/framework/lifecycle.md index 7e7b7ca07..d8efa29c1 100644 --- a/docs/framework/lifecycle.md +++ b/docs/framework/lifecycle.md @@ -14,6 +14,7 @@ register_invalidations register_health_checks register_public_routes register_body_limits +register_rate_limits register_csp_sources register_design_packs register_audit_links @@ -161,6 +162,12 @@ def register_body_limits(self, registry) -> None: See [request-guards.md](request-guards.md). +## `register_rate_limits(registry)` + +Per-route rate limits for anonymous and signed-in requests, keyed by user, IP +or tenant: `registry.add_prefix("/api/x/", RateLimitRule("10/minute", key="user"))`. +See [request-guards.md](request-guards.md). + ## `register_csp_sources(registry)` Whitelist external origins your frontend loads assets from — a font CDN, a diff --git a/docs/framework/request-guards.md b/docs/framework/request-guards.md index 4307e6dbe..78e7b0f25 100644 --- a/docs/framework/request-guards.md +++ b/docs/framework/request-guards.md @@ -109,6 +109,40 @@ def register_public_routes(self, registry) -> None: registry.add_prefix("/api/gis/stac", rate="off") # exempt this rule ``` +### Per-route limits: `register_rate_limits` + +Public-route `rate=` only covers anonymous traffic. To bound a signed-in +action (a POST that sends mail, an export), register a rule; it applies to +anonymous and signed-in requests, before the tiers above: + +```python +from simple_module_core.rate_limit import RateLimitRule + + +def register_rate_limits(self, registry) -> None: + registry.add_exact("/api/invites", RateLimitRule("10/minute", key="user", methods={"POST"})) + registry.add_prefix("/api/reports/", RateLimitRule("30/hour", key="tenant")) +``` + +`key` picks whose budget it is: `user` (the principal id; an API token counts as +its owner), `ip` (the client address, after `ProxyHeaders`), `user_or_ip` (the +default) or `tenant` (the resolved tenant). Anonymous requests, and a tenant +that did not resolve, fall back to the IP. Two users behind one NAT each get the +full rate. The most specific rule wins (exact, regex, longest prefix); `enabled=` +is evaluated per request. Buckets are named +`rule::::=`. + +For a single handler, use the dependency (same store and key resolution; `429` +with `Retry-After`): + +```python +from simple_module_hosting.rate_limit_deps import RateLimit + +@router.post("/send", dependencies=[Depends(RateLimit("10/minute", key="user"))]) +``` + +`rate_limit_authenticated` is now counted per user, not per IP. + ### Storage - **Redis** when `SM_REDIS_URL` is set (needs the `redis` package, which diff --git a/framework/core/simple_module_core/path_rules.py b/framework/core/simple_module_core/path_rules.py index 3f9c17db6..21329c100 100644 --- a/framework/core/simple_module_core/path_rules.py +++ b/framework/core/simple_module_core/path_rules.py @@ -91,6 +91,11 @@ def add( def match(self, method: str, path: str) -> T | None: """The value of the most specific enabled rule for *method* + *path*.""" + found = self.match_entry(method, path) + return None if found is None else found[2] + + def match_entry(self, method: str, path: str) -> tuple[str, str, T] | None: + """Like :meth:`match` but ``(kind, pattern, value)``, to name what matched.""" best: _Entry[T] | None = None for entry in self._entries: if not entry.matches(method, path): @@ -100,7 +105,7 @@ def match(self, method: str, path: str) -> T | None: # Strict ``>`` keeps the first registered on a tie. if best is None or entry.specificity() > best.specificity(): best = entry - return None if best is None else best.value + return None if best is None else (best.kind, best.pattern, best.value) def __len__(self) -> int: return len(self._entries) diff --git a/framework/core/simple_module_core/rate_limit.py b/framework/core/simple_module_core/rate_limit.py index 27428515d..31f267bde 100644 --- a/framework/core/simple_module_core/rate_limit.py +++ b/framework/core/simple_module_core/rate_limit.py @@ -24,6 +24,7 @@ __all__ = [ "InProcessWindowStore", + "RateKey", "RateLimitError", "RateLimitRegistry", "RateLimitRule", @@ -219,5 +220,9 @@ def add_regex( def match(self, method: str, path: str) -> RateLimitRule | None: return self._rules.match(method, path) + def match_entry(self, method: str, path: str) -> tuple[str, str, RateLimitRule] | None: + """``(kind, pattern, rule)`` of the match, so a bucket can be named after it.""" + return self._rules.match_entry(method, path) + def __len__(self) -> int: return len(self._rules) diff --git a/framework/hosting/simple_module_hosting/_rate_limit.py b/framework/hosting/simple_module_hosting/_rate_limit.py index 0d7a6497b..e15e4e6fd 100644 --- a/framework/hosting/simple_module_hosting/_rate_limit.py +++ b/framework/hosting/simple_module_hosting/_rate_limit.py @@ -1,17 +1,21 @@ """Shared rate limiter for anonymous traffic to public routes (GH #347). -Pure ASGI, keyed on client IP. Sits *after* the auth middleware so it can tell +Pure ASGI; the public tiers are keyed on client IP. Sits *after* the auth middleware so it can tell anonymous from signed-in requests, and after ``ProxyHeaders`` so ``SM_TRUSTED_PROXY`` is honoured (``scope["client"]`` is already rewritten). Policy: +* rules from ``register_rate_limits`` (``app.state.rate_limit_rules``) apply first, to + anonymous and signed-in requests alike, each keyed by its ``RateLimitRule.key`` + (``_rate_limit_keys``); the tiers below still apply afterwards; * a request is limited by ``rate_limit_public`` only when the auth middleware judged it anonymous-allowed (``scope["state"]["auth_public"]``: framework defaults, the registry and the provider legacy paths; the registry alone if no auth provider is installed) **and** nobody is signed in. Health/static are never limited; * a matching rule's own ``rate=`` replaces that default for the rule; -* ``rate_limit_authenticated`` (off by default) limits signed-in traffic; +* ``rate_limit_authenticated`` (off by default) limits signed-in traffic, one budget per + user (anonymous-or-id-less principals fall back to their IP); * every other request is untouched. Counting goes to Redis (``SM_REDIS_URL``, atomic Lua INCR + PEXPIRE) so all @@ -22,133 +26,26 @@ from __future__ import annotations import logging -import time -from typing import Any from simple_module_core.rate_limit import ( InProcessWindowStore, + RateLimitRule, RateSpec, - WindowResult, WindowStore, parse_rate, ) from starlette.types import ASGIApp, Receive, Scope, Send +from simple_module_hosting._rate_limit_keys import rule_bucket, rule_key +from simple_module_hosting._rate_limit_store import _LUA, RedisWindowStore, build_store from simple_module_hosting._request_guard_responses import send_guard_response +__all__ = ["_LUA", "RateLimitMiddleware", "RedisWindowStore", "build_store"] + logger = logging.getLogger("simple_module.request_guard") _MESSAGE = "Too many requests. Please slow down and try again shortly." -_KEY_PREFIX = "sm:rl:" _OPERATIONAL_PREFIXES = ("/health", "/static/") -# Re-warn about a dead Redis at most this often, not once per request. -_WARN_EVERY = 60.0 -# After a Redis error, skip Redis entirely for this long. Without it a blackholed -# Redis costs every anonymous request the full socket timeout. -_BACKOFF = 5.0 - -_LUA = """ -local c = redis.call('INCR', KEYS[1]) -local t = redis.call('PTTL', KEYS[1]) --- t < 0 heals a counter that somehow lost its TTL, which would block that IP forever. --- ARGV[2] == '1' is a sliding window: every hit restarts it. -if c == 1 or t < 0 or ARGV[2] == '1' then - redis.call('PEXPIRE', KEYS[1], ARGV[1]) - t = tonumber(ARGV[1]) -end -return {c, t} -""" - -_RESET_LUA = "redis.call('DEL', KEYS[1]) return 0" - -# Read-only twin of ``_LUA``: a missing key is an empty window. -_PEEK_LUA = """ -local c = redis.call('GET', KEYS[1]) -if not c then return {0, 0} end -return {tonumber(c), redis.call('PTTL', KEYS[1])} -""" - - -def _parse(reply: Any, empty_retry: int) -> tuple[int, int]: - """``(count, retry_after)`` from a script's ``{count, pttl}`` reply. - - *empty_retry* is the answer when the key has no TTL left to report. - """ - count, pttl = int(reply[0]), int(reply[1]) - retry = max(1, -(-pttl // 1000)) if pttl > 0 else empty_retry - return count, retry - - -class RedisWindowStore: - """Fixed-window counters in Redis; shared across workers and processes. - - Never takes requests down: on a Redis error the hit is counted in a per-process - fallback store instead (bounded, weaker than the shared budget but not "no - limit"), and Redis is re-probed after ``_BACKOFF`` seconds, so recovery is - automatic and nothing latches. - """ - - def __init__(self, client: Any, *, clock=time.monotonic) -> None: - self._client = client - self._clock = clock - self._last_warn = float("-inf") - self._down_until = float("-inf") - self._fallback = InProcessWindowStore() - - @classmethod - def from_url(cls, url: str) -> RedisWindowStore: - import redis.asyncio as aioredis - - # Short timeouts: this is on the request path, and redis-py defaults to - # none, so a Redis that accepts connections but stalls would hang us. - client = aioredis.from_url(url, socket_connect_timeout=0.5, socket_timeout=0.5) - return cls(client) - - async def _eval(self, script: str, key: str, *args: Any) -> Any: - """Run *script* on *key*; ``None`` means "answer from the per-worker fallback".""" - if self._clock() < self._down_until: - return None - try: - return await self._client.eval(script, 1, _KEY_PREFIX + key, *args) - except Exception as exc: - now = self._clock() - self._down_until = now + _BACKOFF - if now - self._last_warn >= _WARN_EVERY: - self._last_warn = now - logger.warning( - "Rate limiter Redis unavailable (%s); using per-worker counters", exc - ) - return None - - async def hit(self, key: str, spec: RateSpec, *, sliding: bool = False) -> WindowResult: - args = (spec.period * 1000, "1") if sliding else (spec.period * 1000,) - reply = await self._eval(_LUA, key, *args) - if reply is None: - return await self._fallback.hit(key, spec, sliding=sliding) - count, retry = _parse(reply, spec.period) - return WindowResult(allowed=count <= spec.limit, retry_after=retry, count=count) - - async def peek(self, key: str, spec: RateSpec) -> WindowResult: - reply = await self._eval(_PEEK_LUA, key) - if reply is None: - return await self._fallback.peek(key, spec) - count, retry = _parse(reply, 0) - return WindowResult(allowed=count < spec.limit, retry_after=retry, count=count) - - async def reset(self, key: str) -> None: - # The fallback too: hits taken during an outage live there. - await self._fallback.reset(key) - await self._eval(_RESET_LUA, key) - - -def build_store(redis_url: str | None) -> WindowStore: - """Redis when configured and importable, else per-process counters.""" - if redis_url: - try: - return RedisWindowStore.from_url(redis_url) - except Exception as exc: # missing redis package, malformed URL - logger.warning("Rate limiter cannot use Redis (%s); using per-worker counters", exc) - return InProcessWindowStore() class RateLimitMiddleware: @@ -165,12 +62,12 @@ def __init__( self.authenticated_rate = parse_rate(authenticated_rate) self.store = store if store is not None else InProcessWindowStore() - def _policy(self, scope: Scope) -> tuple[str, RateSpec] | None: + def _policy(self, scope: Scope) -> tuple[str, RateSpec, str] | None: authed = scope.get("state", {}).get("user") is not None if authed: if self.authenticated_rate is None: return None - return "auth", self.authenticated_rate + return "auth", self.authenticated_rate, "user_or_ip" registry = getattr(scope["app"].state, "public_routes", None) rule = registry.match(scope["method"], scope["path"]) if registry is not None else None flag = scope.get("state", {}).get("auth_public") @@ -182,45 +79,61 @@ def _policy(self, scope: Scope) -> tuple[str, RateSpec] | None: return None # health probes and static assets are not an API surface if rule is not None and rule.rate is not None: spec = rule.rate_spec - # Methods are part of the bucket: two rules with one pattern but different - # verbs/rates must not share a counter (Redis fixes the window length on - # the first hit, so a shared key would apply the wrong period). - methods = ",".join(sorted(rule.methods)) if rule.methods else "*" - bucket = f"rule:{rule.kind}:{methods}:{rule.pattern}" - return (bucket, spec) if spec is not None else None + bucket = rule_bucket(rule.kind, rule.pattern, rule.methods) + return (bucket, spec, "ip") if spec is not None else None if self.public_rate is None: return None - return "public", self.public_rate + return "public", self.public_rate, "ip" - async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: - if scope["type"] != "http": - await self.app(scope, receive, send) - return - policy = self._policy(scope) - if policy is None: - await self.app(scope, receive, send) - return - bucket, spec = policy - client = scope.get("client") - ip = client[0] if client else "unknown" - result = await self.store.hit(f"{bucket}:{ip}", spec) + def _rule(self, scope: Scope) -> tuple[str, RateLimitRule] | None: + registry = getattr(scope["app"].state, "rate_limit_rules", None) + found = registry.match_entry(scope["method"], scope["path"]) if registry else None + if found is None: + return None + kind, pattern, rule = found + return rule_bucket(kind, pattern, rule.methods), rule + + async def _admit(self, scope: Scope, bucket: str, spec: RateSpec, key_kind: str): + """Count one hit; ``None`` when allowed, else the 429 inputs.""" + key = rule_key(scope, key_kind) + result = await self.store.hit(f"{bucket}:{key}", spec) if result.allowed: - await self.app(scope, receive, send) - return + return None logger.warning( - "Rate limit exceeded: %s %s ip=%s bucket=%s limit=%s", + "Rate limit exceeded: %s %s %s bucket=%s limit=%s", scope["method"], scope["path"], - ip, + key, bucket, spec, ) - await send_guard_response( - scope, - receive, - send, - 429, - _MESSAGE, - headers={"Retry-After": str(result.retry_after)}, - extra={"retry_after": result.retry_after}, - ) + return result.retry_after + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + checks: list[tuple[str, RateSpec, str]] = [] + matched = self._rule(scope) + if matched is not None: + bucket, rule = matched + spec = parse_rate(rule.rate) + if spec is not None: + checks.append((bucket, spec, rule.key)) + policy = self._policy(scope) + if policy is not None: + checks.append(policy) + for bucket, spec, key_kind in checks: + retry_after = await self._admit(scope, bucket, spec, key_kind) + if retry_after is not None: + await send_guard_response( + scope, + receive, + send, + 429, + _MESSAGE, + headers={"Retry-After": str(retry_after)}, + extra={"retry_after": retry_after}, + ) + return + await self.app(scope, receive, send) diff --git a/framework/hosting/simple_module_hosting/_rate_limit_keys.py b/framework/hosting/simple_module_hosting/_rate_limit_keys.py new file mode 100644 index 000000000..390e4d3f4 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_rate_limit_keys.py @@ -0,0 +1,62 @@ +"""Who a rate-limit budget belongs to: key resolution and bucket names (#529). + +Works on the ASGI ``scope`` so the middleware and the :func:`RateLimit` dependency +(``request.scope``) share one resolver. By the time either runs, ``AuthMiddleware`` +has set ``state["user"]`` (an API-token principal already carries its owner's id, a +service key its own ``service:``), ``TenantMiddleware`` has set +``state["tenant_id"]`` and ``ProxyHeaders`` has rewritten ``scope["client"]``. +""" + +from __future__ import annotations + +from collections.abc import Iterable + +from starlette.types import Scope + +_UNKNOWN_IP = "unknown" +_ALL_METHODS = "*" +KEY_USER = "user" +KEY_IP = "ip" +KEY_USER_OR_IP = "user_or_ip" +KEY_TENANT = "tenant" + + +def client_ip(scope: Scope) -> str: + client = scope.get("client") + return client[0] if client else _UNKNOWN_IP + + +def _state(scope: Scope, name: str) -> object | None: + return (scope.get("state") or {}).get(name) + + +def _user_id(scope: Scope) -> str | None: + user = _state(scope, "user") + uid = getattr(user, "id", None) if user is not None else None + return None if uid is None or str(uid) == "" else str(uid) + + +def rule_key(scope: Scope, key: str) -> str: + """``"="`` for *key*; a missing user/tenant falls back to the IP.""" + if key == KEY_TENANT: + tenant = _state(scope, "tenant_id") + if tenant: + return f"{KEY_TENANT}={tenant}" + elif key in (KEY_USER, KEY_USER_OR_IP): + uid = _user_id(scope) + if uid is not None: + return f"{KEY_USER}={uid}" + return f"{KEY_IP}={client_ip(scope)}" + + +def methods_label(methods: Iterable[str] | None) -> str: + return ",".join(sorted(methods)) if methods else _ALL_METHODS + + +def rule_bucket(kind: str, pattern: str, methods: Iterable[str] | None) -> str: + """``rule:::``; the key is appended by the caller. + + Methods are part of the bucket: two rules with one pattern but different verbs or + rates must not share a counter (Redis fixes the window on the first hit). + """ + return f"rule:{kind}:{methods_label(methods)}:{pattern}" diff --git a/framework/hosting/simple_module_hosting/_rate_limit_store.py b/framework/hosting/simple_module_hosting/_rate_limit_store.py new file mode 100644 index 000000000..afebf57c9 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_rate_limit_store.py @@ -0,0 +1,126 @@ +"""Window stores behind the rate limiter: Redis when configured, else per-process.""" + +from __future__ import annotations + +import logging +import time +from typing import Any + +from simple_module_core.rate_limit import ( + InProcessWindowStore, + RateSpec, + WindowResult, + WindowStore, +) + +logger = logging.getLogger("simple_module.request_guard") + +_KEY_PREFIX = "sm:rl:" +# Re-warn about a dead Redis at most this often, not once per request. +_WARN_EVERY = 60.0 +# After a Redis error, skip Redis entirely for this long. Without it a blackholed +# Redis costs every anonymous request the full socket timeout. +_BACKOFF = 5.0 + +_LUA = """ +local c = redis.call('INCR', KEYS[1]) +local t = redis.call('PTTL', KEYS[1]) +-- t < 0 heals a counter that somehow lost its TTL, which would block that IP forever. +-- ARGV[2] == '1' is a sliding window: every hit restarts it. +if c == 1 or t < 0 or ARGV[2] == '1' then + redis.call('PEXPIRE', KEYS[1], ARGV[1]) + t = tonumber(ARGV[1]) +end +return {c, t} +""" + +_RESET_LUA = "redis.call('DEL', KEYS[1]) return 0" + +# Read-only twin of ``_LUA``: a missing key is an empty window. +_PEEK_LUA = """ +local c = redis.call('GET', KEYS[1]) +if not c then return {0, 0} end +return {tonumber(c), redis.call('PTTL', KEYS[1])} +""" + + +def _parse(reply: Any, empty_retry: int) -> tuple[int, int]: + """``(count, retry_after)`` from a script's ``{count, pttl}`` reply. + + *empty_retry* is the answer when the key has no TTL left to report. + """ + count, pttl = int(reply[0]), int(reply[1]) + retry = max(1, -(-pttl // 1000)) if pttl > 0 else empty_retry + return count, retry + + +class RedisWindowStore: + """Fixed-window counters in Redis; shared across workers and processes. + + Never takes requests down: on a Redis error the hit is counted in a per-process + fallback store instead (bounded, weaker than the shared budget but not "no + limit"), and Redis is re-probed after ``_BACKOFF`` seconds, so recovery is + automatic and nothing latches. + """ + + def __init__(self, client: Any, *, clock=time.monotonic) -> None: + self._client = client + self._clock = clock + self._last_warn = float("-inf") + self._down_until = float("-inf") + self._fallback = InProcessWindowStore() + + @classmethod + def from_url(cls, url: str) -> RedisWindowStore: + import redis.asyncio as aioredis + + # Short timeouts: this is on the request path, and redis-py defaults to + # none, so a Redis that accepts connections but stalls would hang us. + client = aioredis.from_url(url, socket_connect_timeout=0.5, socket_timeout=0.5) + return cls(client) + + async def _eval(self, script: str, key: str, *args: Any) -> Any: + """Run *script* on *key*; ``None`` means "answer from the per-worker fallback".""" + if self._clock() < self._down_until: + return None + try: + return await self._client.eval(script, 1, _KEY_PREFIX + key, *args) + except Exception as exc: + now = self._clock() + self._down_until = now + _BACKOFF + if now - self._last_warn >= _WARN_EVERY: + self._last_warn = now + logger.warning( + "Rate limiter Redis unavailable (%s); using per-worker counters", exc + ) + return None + + async def hit(self, key: str, spec: RateSpec, *, sliding: bool = False) -> WindowResult: + args = (spec.period * 1000, "1") if sliding else (spec.period * 1000,) + reply = await self._eval(_LUA, key, *args) + if reply is None: + return await self._fallback.hit(key, spec, sliding=sliding) + count, retry = _parse(reply, spec.period) + return WindowResult(allowed=count <= spec.limit, retry_after=retry, count=count) + + async def peek(self, key: str, spec: RateSpec) -> WindowResult: + reply = await self._eval(_PEEK_LUA, key) + if reply is None: + return await self._fallback.peek(key, spec) + count, retry = _parse(reply, 0) + return WindowResult(allowed=count < spec.limit, retry_after=retry, count=count) + + async def reset(self, key: str) -> None: + # The fallback too: hits taken during an outage live there. + await self._fallback.reset(key) + await self._eval(_RESET_LUA, key) + + +def build_store(redis_url: str | None) -> WindowStore: + """Redis when configured and importable, else per-process counters.""" + if redis_url: + try: + return RedisWindowStore.from_url(redis_url) + except Exception as exc: # missing redis package, malformed URL + logger.warning("Rate limiter cannot use Redis (%s); using per-worker counters", exc) + return InProcessWindowStore() diff --git a/framework/hosting/simple_module_hosting/rate_limit_deps.py b/framework/hosting/simple_module_hosting/rate_limit_deps.py new file mode 100644 index 000000000..cb6ddadbb --- /dev/null +++ b/framework/hosting/simple_module_hosting/rate_limit_deps.py @@ -0,0 +1,45 @@ +"""``RateLimit("10/minute", key="user")``: a per-route limit as a FastAPI dependency (#529). + +Counts in the host's shared store (``app.state.sm.rate_limit_store``: Redis when +``SM_REDIS_URL`` is set, else per worker) with the same key resolution as +``register_rate_limits`` rules. Use it where a limit belongs to one handler; +use the registry for a path or prefix. +""" + +from __future__ import annotations + +from fastapi import HTTPException, Request +from simple_module_core.rate_limit import RateKey, RateSpec, parse_rate + +from simple_module_hosting._rate_limit_keys import KEY_USER_OR_IP, methods_label, rule_key + +_TOO_MANY = 429 +_DETAIL = "Too many requests. Please slow down and try again shortly." +_RETRY_AFTER = "Retry-After" + +__all__ = ["RateLimit"] + + +class RateLimit: + """Dependency raising 429 with ``Retry-After`` past *rate* per *key*.""" + + def __init__(self, rate: str, *, key: RateKey = KEY_USER_OR_IP) -> None: + spec: RateSpec | None = parse_rate(rate) + if spec is None: + raise ValueError("RateLimit needs a real rate, not a disabled one") + self.rate = rate + self.spec = spec + self.key = key + + async def __call__(self, request: Request) -> None: + route = request.scope.get("route") + path = getattr(route, "path", None) or request.url.path + bucket = f"dep:{methods_label([request.method])}:{path}:{self.rate}" + store = request.app.state.sm.rate_limit_store + result = await store.hit(f"{bucket}:{rule_key(request.scope, self.key)}", self.spec) + if not result.allowed: + raise HTTPException( + status_code=_TOO_MANY, + detail=_DETAIL, + headers={_RETRY_AFTER: str(result.retry_after)}, + ) diff --git a/framework/hosting/tests/test_rate_limit_rules_redis.py b/framework/hosting/tests/test_rate_limit_rules_redis.py new file mode 100644 index 000000000..e1736a603 --- /dev/null +++ b/framework/hosting/tests/test_rate_limit_rules_redis.py @@ -0,0 +1,25 @@ +"""Per-route rules counted in a real Redis: workers share one budget (#529).""" + +from __future__ import annotations + +import uuid + +from simple_module_core.rate_limit import RateSpec +from simple_module_hosting._rate_limit_store import RedisWindowStore + + +async def test_two_stores_share_one_user_budget(redis_server) -> None: + # Two stores = two workers; a per-test bucket name keeps xdist workers apart. + bucket = f"rule:prefix:POST:/api/x:user=alice-{uuid.uuid4().hex}" + spec = RateSpec(3, 60) + first = RedisWindowStore.from_url(redis_server) + second = RedisWindowStore.from_url(redis_server) + try: + results = [await first.hit(bucket, spec), await second.hit(bucket, spec)] + results += [await first.hit(bucket, spec), await second.hit(bucket, spec)] + other = await second.hit(bucket.replace("alice", "bob"), spec) + finally: + await first.reset(bucket) + assert [r.allowed for r in results] == [True, True, True, False] + assert 1 <= results[-1].retry_after <= 60 + assert other.allowed diff --git a/framework/hosting/tests/test_rate_limit_rules_registry.py b/framework/hosting/tests/test_rate_limit_rules_registry.py new file mode 100644 index 000000000..cae7b0d29 --- /dev/null +++ b/framework/hosting/tests/test_rate_limit_rules_registry.py @@ -0,0 +1,150 @@ +"""Per-route rate-limit rules and the ``RateLimit`` dependency (#529).""" + +from __future__ import annotations + +from types import SimpleNamespace + +import httpx +from fastapi import Depends, FastAPI +from simple_module_core.rate_limit import ( + InProcessWindowStore, + RateKey, + RateLimitRegistry, + RateLimitRule, +) +from simple_module_hosting._rate_limit import RateLimitMiddleware +from simple_module_hosting.rate_limit_deps import RateLimit +from starlette.applications import Starlette +from starlette.responses import JSONResponse +from starlette.routing import Route +from starlette.types import ASGIApp, Receive, Scope, Send + +_USER_HEADER = b"x-user" +_TENANT_HEADER = b"x-tenant" +_RATE = "10/minute" + + +class _Ident: + """Stand-in for Auth + Tenant middleware: identity comes from headers.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] == "http": + headers = dict(scope["headers"]) + state = scope.setdefault("state", {}) + if _USER_HEADER in headers: + state["user"] = SimpleNamespace(id=headers[_USER_HEADER].decode()) + if _TENANT_HEADER in headers: + state["tenant_id"] = headers[_TENANT_HEADER].decode() + await self.app(scope, receive, send) + + +def _app(registry: RateLimitRegistry) -> Starlette: + async def ok(request): + return JSONResponse({"ok": True}) + + app = Starlette(routes=[Route("/api/things", ok, methods=["GET", "POST"])]) + app.state.rate_limit_rules = registry + app.add_middleware(RateLimitMiddleware, public_rate=None) + app.add_middleware(_Ident) + return app + + +def _client(app: ASGIApp) -> httpx.AsyncClient: + transport = httpx.ASGITransport(app=app, client=("9.9.9.9", 1)) + return httpx.AsyncClient(transport=transport, base_url="http://t") + + +def _registry(rule: RateLimitRule) -> RateLimitRegistry: + reg = RateLimitRegistry() + reg.add_prefix("/api/things", rule) + return reg + + +async def _burst(c: httpx.AsyncClient, user: str | None, n: int, method: str = "POST") -> list[int]: + headers = {_USER_HEADER.decode(): user} if user else {} + return [(await c.request(method, "/api/things", headers=headers)).status_code for _ in range(n)] + + +async def test_two_users_on_one_ip_each_get_their_own_budget() -> None: + reg = _registry(RateLimitRule(_RATE, key="user", methods=frozenset({"POST"}))) + async with _client(_app(reg)) as c: + alice = await _burst(c, "alice", 11) + bob = await _burst(c, "bob", 10) + r = await c.post("/api/things", headers={_USER_HEADER.decode(): "alice"}) + get = await c.get("/api/things", headers={_USER_HEADER.decode(): "alice"}) + assert alice[:10] == [200] * 10 and alice[10] == 429 + assert bob == [200] * 10 + assert r.status_code == 429 and 1 <= int(r.headers["retry-after"]) <= 60 + assert get.status_code == 200, "a POST-only rule must not limit GET" + + +async def test_anonymous_falls_back_to_ip() -> None: + reg = _registry(RateLimitRule(_RATE, key="user")) + async with _client(_app(reg)) as c: + codes = await _burst(c, None, 11) + signed_in = await _burst(c, "carol", 1) + assert codes[-1] == 429 and codes[:10] == [200] * 10 + assert signed_in == [200] + + +async def test_ip_key_shares_one_budget_across_users() -> None: + reg = _registry(RateLimitRule("2/minute", key="ip")) + async with _client(_app(reg)) as c: + assert await _burst(c, "a", 2) == [200, 200] + assert await _burst(c, "b", 1) == [429] + + +async def test_tenant_key_pools_users_of_a_tenant_and_falls_back() -> None: + reg = _registry(RateLimitRule("2/minute", key="tenant")) + async with _client(_app(reg)) as c: + + async def hit(user: str, tenant: str | None) -> int: + h = {_USER_HEADER.decode(): user} + if tenant: + h[_TENANT_HEADER.decode()] = tenant + return (await c.post("/api/things", headers=h)).status_code + + assert [await hit("a", "t1"), await hit("b", "t1"), await hit("c", "t1")] == [ + 200, + 200, + 429, + ] + assert await hit("a", "t2") == 200 + # No tenant resolved: falls back to the IP bucket, not a shared "None" tenant. + assert [await hit("a", None), await hit("b", None), await hit("c", None)] == [ + 200, + 200, + 429, + ] + + +async def test_rule_precedes_the_public_tier() -> None: + reg = _registry(RateLimitRule("1/minute", key="ip")) + async with _client(_app(reg)) as c: + assert await _burst(c, None, 2, "GET") == [200, 429] + + +def _dep_app(key: RateKey) -> FastAPI: + app = FastAPI() + app.state.sm = SimpleNamespace(rate_limit_store=InProcessWindowStore()) + + @app.post("/dep", dependencies=[Depends(RateLimit("3/minute", key=key))]) + async def dep() -> dict[str, bool]: + return {"ok": True} + + app.add_middleware(_Ident) + return app + + +async def test_dependency_limits_per_user_with_retry_after() -> None: + async with _client(_dep_app("user")) as c: + h = {_USER_HEADER.decode(): "alice"} + codes = [(await c.post("/dep", headers=h)).status_code for _ in range(3)] + over = await c.post("/dep", headers=h) + other = await c.post("/dep", headers={_USER_HEADER.decode(): "bob"}) + assert codes == [200] * 3 + assert over.status_code == 429 and int(over.headers["retry-after"]) >= 1 + assert other.status_code == 200 From 501c2350fdd644560650082b6ef1967c2af3d654 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 16:58:56 +0200 Subject: [PATCH 03/12] feat(hosting): honour allowlisted X-Forwarded-Host from a trusted proxy (#528) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 4 + docs/framework/middleware.md | 4 + docs/framework/multi-tenancy.md | 2 +- .../simple_module_hosting/_forwarded_host.py | 124 +++++++++++++++++ .../simple_module_hosting/_phase_helpers.py | 13 +- .../tests/test_build_time_settings_marked.py | 1 - .../hosting/tests/test_forwarded_host.py | 128 ++++++++++++++++++ .../hosting/tests/test_middleware_order.py | 1 + .../tests/test_subdomains_forwarded_host.py | 66 +++++++++ 9 files changed, 337 insertions(+), 6 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/_forwarded_host.py create mode 100644 framework/hosting/tests/test_forwarded_host.py create mode 100644 modules/tenants/tests/test_subdomains_forwarded_host.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 160318854..115f2dd35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Added +- `forwarded_host_allowlist` host setting (#528): behind a trusted proxy (`SM_TRUSTED_PROXY`), + `X-Forwarded-Host` replaces the request host (and `scope["server"]`) when it matches an exact host or + `*.example.com` entry, so `url_for` and subdomain tenancy see the public host. Empty (default) ignores + the header; tenant subdomains need `*.` in the list. - New `mail` module (`simple_module_mail`) owns outgoing mail: the SMTP/console transport, its settings (`/admin/settings/`, package `mail`) and the on-demand `mail.mailer` health check behind "Test mailer connection". Modules send through diff --git a/docs/framework/middleware.md b/docs/framework/middleware.md index 55fc9d588..6902b4049 100644 --- a/docs/framework/middleware.md +++ b/docs/framework/middleware.md @@ -99,6 +99,10 @@ It is still load-bearing for anything else that reads `request.url.scheme` or ca Set it to a comma-separated list of proxy IPs/CIDRs, or `*` to trust any peer — correct when the container is only reachable through one proxy, wrong when anything else can connect. +### `ForwardedHostMiddleware` *(opt-in)* + +Installed only when **both** `SM_TRUSTED_PROXY` and the `forwarded_host_allowlist` host setting (restart-required) are set. uvicorn's `ProxyHeadersMiddleware` fixes scheme and client but not the host, so behind a proxy that rewrites `Host`, `request.url_for(...)` and the tenants subdomain resolver see the upstream name. This middleware replaces the `host` header and `scope["server"]` with the leftmost `X-Forwarded-Host` value when the TCP peer is trusted (same `*` / IP / CIDR rule as uvicorn) **and** the value matches the allowlist: an exact host or `*.example.com` (any subdomain, never the apex), each with an optional `:port` (an entry without a port accepts any port). Anything else is ignored and logged at debug, so a forged header cannot poison absolute URLs. It is added after `ProxyHeaders` so it runs first and still sees the proxy's own address. `Forwarded:` (RFC 7239) is not supported. Email links are unaffected: they are built from configured base URLs (for example `tenants.public_base_url`), never from the request. + ### `CorrelationIdMiddleware` Reads the `X-Correlation-ID` header (or generates a UUID4 hex) and makes the value available three ways: diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 814ca116d..1fb17edfd 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -227,7 +227,7 @@ carries no tenant never blanks a row already stamped. does — and its answer is final. The `tenants` resolver takes, in order: the subdomain (with its `subdomain_base` setting — the one source that also works for anonymous visitors, on public routes), the tenant header (members only), -then the session's choice validated against a membership (#363). Without one it falls back to the principal's +then the session's choice validated against a membership (#363). Behind a proxy that rewrites `Host`, the subdomain is read from `X-Forwarded-Host` only if `SM_TRUSTED_PROXY` and `forwarded_host_allowlist` are set and the allowlist contains `*.` (see [middleware](middleware.md)). Without one it falls back to the principal's `tenant_id` claim, and for **anonymous** requests only, the configured `tenant_header`. An authenticated user can never pick a tenant by header. The `users` module's `User` is platform-global and has no `tenant_id` diff --git a/framework/hosting/simple_module_hosting/_forwarded_host.py b/framework/hosting/simple_module_hosting/_forwarded_host.py new file mode 100644 index 000000000..8970c0234 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_forwarded_host.py @@ -0,0 +1,124 @@ +"""Honour ``X-Forwarded-Host`` from a trusted proxy, for allowlisted hosts only (#528). + +uvicorn's ``ProxyHeadersMiddleware`` corrects scheme and client address but not +the host, so behind a proxy that rewrites ``Host`` to the upstream name, +``request.url_for(...)`` and the tenants subdomain resolver see the internal +name. This middleware swaps the ``host`` header (and ``scope["server"]``) for the +forwarded one, but only when the peer is a trusted proxy *and* the value is on +the operator's allowlist: an unlisted host is ignored, so a forged header cannot +poison absolute URLs or pick a tenant. + +It runs before ProxyHeaders (which replaces ``scope["client"]`` with the +forwarded client) so the peer it checks is still the proxy's TCP address. +""" + +from __future__ import annotations + +import ipaddress +import logging +import re +from collections.abc import Iterable + +from starlette.types import ASGIApp, Receive, Scope, Send + +logger = logging.getLogger(__name__) + +_WILDCARD = "*" +_SUBDOMAIN_PREFIX = "*." +_HOST_HEADER = b"host" +_FORWARDED_HOST_HEADER = b"x-forwarded-host" +_DEFAULT_PORTS = {"https": 443, "wss": 443} +_FALLBACK_PORT = 80 +_HOST_PATTERN = re.compile(r"^(?P[a-z0-9]([a-z0-9._-]*[a-z0-9])?)(:(?P\d{1,5}))?$") + + +def _split_host(value: str) -> tuple[str, str | None] | None: + match = _HOST_PATTERN.match(value.strip().lower()) + return (match["host"], match["port"]) if match else None + + +class TrustedPeers: + """Mirror of uvicorn's trusted-hosts rule: ``*``, IPs and CIDRs, comma-separated.""" + + def __init__(self, spec: str) -> None: + self._always = spec.strip() == _WILDCARD + self._literals: set[str] = set() + self._networks: list[ipaddress.IPv4Network | ipaddress.IPv6Network] = [] + for item in spec.split(","): + item = item.strip() + if not item or item == _WILDCARD: + continue + try: + self._networks.append(ipaddress.ip_network(item, strict=False)) + except ValueError: + self._literals.add(item) # e.g. a unix-socket peer name + + def __contains__(self, peer: str | None) -> bool: + if self._always: + return True + if not peer: + return False + if peer in self._literals: + return True + try: + address = ipaddress.ip_address(peer) + except ValueError: + return False + return any(address in network for network in self._networks) + + +class HostAllowlist: + """Exact hosts or ``*.example.com`` (any depth, never the apex), optional port.""" + + def __init__(self, entries: Iterable[str]) -> None: + self._exact: list[tuple[str, str | None]] = [] + self._suffixes: list[tuple[str, str | None]] = [] + for entry in entries: + wildcard = entry.strip().lower().startswith(_SUBDOMAIN_PREFIX) + parsed = _split_host(entry.strip()[len(_SUBDOMAIN_PREFIX) :] if wildcard else entry) + if parsed is None: + logger.warning("Ignoring invalid forwarded_host_allowlist entry %r", entry) + continue + host, port = parsed + (self._suffixes if wildcard else self._exact).append((host, port)) + + def allows(self, host: str, port: str | None) -> bool: + def port_ok(allowed: str | None) -> bool: + return allowed is None or allowed == port + + if any(host == name and port_ok(p) for name, p in self._exact): + return True + return any(host.endswith("." + name) and port_ok(p) for name, p in self._suffixes) + + +class ForwardedHostMiddleware: + def __init__(self, app: ASGIApp, *, trusted_hosts: str, allowlist: Iterable[str]) -> None: + self.app = app + self._peers = TrustedPeers(trusted_hosts) + self._allowed = HostAllowlist(allowlist) + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] in ("http", "websocket"): + client = scope.get("client") + if client and client[0] in self._peers: + self._rewrite(scope) + await self.app(scope, receive, send) + + def _rewrite(self, scope: Scope) -> None: + raw = ",".join( + v.decode("latin-1") for k, v in scope["headers"] if k == _FORWARDED_HOST_HEADER + ) + if not raw: + return + parsed = _split_host(raw.split(",")[0]) + if parsed is None or not self._allowed.allows(*parsed): + logger.debug("Ignoring X-Forwarded-Host %r: not on the allowlist", raw) + return + host, port = parsed + value = host if port is None else f"{host}:{port}" + scope["headers"] = [ + *((k, v) for k, v in scope["headers"] if k != _HOST_HEADER), + (_HOST_HEADER, value.encode("latin-1")), + ] + default = _DEFAULT_PORTS.get(scope.get("scheme", ""), _FALLBACK_PORT) + scope["server"] = (host, int(port) if port else default) diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 2b1783fd7..2a96b20ae 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -32,6 +32,7 @@ request_validation_error_handler, unhandled_exception_handler, ) +from simple_module_hosting._forwarded_host import ForwardedHostMiddleware from simple_module_hosting._inertia_cache import InertiaCacheMiddleware from simple_module_hosting._module_routes import wire_module_routes from simple_module_hosting._rate_limit import RateLimitMiddleware @@ -226,10 +227,14 @@ def install_middleware( # explicit trust setting — never trust forwarded headers by default. if settings.trusted_proxy: app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=settings.trusted_proxy) - # SLOT: ForwardedHost. Added after ProxyHeaders so it executes before it - # (batch-1 slice for #528), while the TCP peer is still the proxy. Install - # only when ``settings.trusted_proxy`` and ``settings.forwarded_host_allowlist`` - # are both set. No-op until then. + # Added after ProxyHeaders so it executes before it, while the TCP peer is + # still the proxy (#528). Needs both a trusted proxy and an allowlist. + if settings.forwarded_host_allowlist and settings.trusted_proxy: + app.add_middleware( + ForwardedHostMiddleware, + trusted_hosts=settings.trusted_proxy, + allowlist=settings.forwarded_host_allowlist, + ) def attach_public_routes(app: FastAPI, settings: Settings, registry) -> None: diff --git a/framework/hosting/tests/test_build_time_settings_marked.py b/framework/hosting/tests/test_build_time_settings_marked.py index 8d6c07516..01bcebc90 100644 --- a/framework/hosting/tests/test_build_time_settings_marked.py +++ b/framework/hosting/tests/test_build_time_settings_marked.py @@ -30,7 +30,6 @@ # them in create_app. Each slice deletes its own fields from this set. _NOT_YET_CONSUMED = frozenset( { - "forwarded_host_allowlist", "cors_allow_origins", "cors_allow_credentials", "cors_allow_methods", diff --git a/framework/hosting/tests/test_forwarded_host.py b/framework/hosting/tests/test_forwarded_host.py new file mode 100644 index 000000000..40d0f118b --- /dev/null +++ b/framework/hosting/tests/test_forwarded_host.py @@ -0,0 +1,128 @@ +"""X-Forwarded-Host from a trusted proxy, honoured for allowlisted hosts only (#528).""" + +from __future__ import annotations + +import httpx +import pytest +from fastapi import FastAPI, Request +from simple_module_hosting._forwarded_host import ( + ForwardedHostMiddleware, + HostAllowlist, + TrustedPeers, +) +from simple_module_hosting.app_builder import create_app +from simple_module_hosting.settings import Settings + +_PROBE_PATH = "/forwarded-host-probe" +_PROBE_NAME = "forwarded_host_probe" +_PROXY_IP = "10.1.2.3" +_STRANGER_IP = "198.51.100.9" +_PUBLIC_HOST = "app.example.com" +_FORWARDED = "x-forwarded-host" + + +def _names(app) -> tuple[str, ...]: + return tuple(m.cls.__name__ for m in app.user_middleware) + + +def _settings(**overrides) -> Settings: + base = { + "database_url": "sqlite+aiosqlite:///:memory:", + "environment": "testing", + "secret_key": "x" * 32, + "multi_tenant": False, + } + return Settings(**(base | overrides)) + + +def _probe_app(**middleware) -> FastAPI: + app = FastAPI() + + @app.get(_PROBE_PATH, name=_PROBE_NAME) + async def probe(request: Request) -> dict[str, str]: + return {"url": str(request.url_for(_PROBE_NAME)), "host": request.url.hostname or ""} + + return ForwardedHostMiddleware(app, **middleware) # ty: ignore[invalid-return-type] + + +async def _get(app, forwarded: str, *, peer: str = _PROXY_IP) -> dict[str, str]: + transport = httpx.ASGITransport(app=app, client=(peer, 4242)) + async with httpx.AsyncClient(transport=transport, base_url="http://internal:8000") as client: + resp = await client.get(_PROBE_PATH, headers={_FORWARDED: forwarded}) + return resp.json() + + +def _mw(allowlist: list[str], trusted: str = _PROXY_IP): + return _probe_app(trusted_hosts=trusted, allowlist=allowlist) + + +class TestWiring: + def test_absent_by_default(self) -> None: + assert "ForwardedHostMiddleware" not in _names(create_app(_settings())) + + def test_needs_both_proxy_and_allowlist(self) -> None: + only_proxy = create_app(_settings(trusted_proxy="*")) + only_list = create_app(_settings(forwarded_host_allowlist=[_PUBLIC_HOST])) + assert "ForwardedHostMiddleware" not in _names(only_proxy) + assert "ForwardedHostMiddleware" not in _names(only_list) + + def test_runs_before_proxy_headers(self) -> None: + app = create_app(_settings(trusted_proxy="*", forwarded_host_allowlist=[_PUBLIC_HOST])) + names = _names(app) + assert names[:2] == ("ForwardedHostMiddleware", "ProxyHeadersMiddleware") + + +class TestRewrite: + async def test_allowlisted_host_builds_public_urls(self) -> None: + body = await _get(_mw([_PUBLIC_HOST]), _PUBLIC_HOST) + assert body == {"url": f"http://{_PUBLIC_HOST}{_PROBE_PATH}", "host": _PUBLIC_HOST} + + async def test_forged_host_not_on_allowlist_is_ignored(self) -> None: + body = await _get(_mw([_PUBLIC_HOST]), "evil.example.net") + assert body["host"] == "internal" + + async def test_untrusted_peer_is_ignored(self) -> None: + body = await _get(_mw([_PUBLIC_HOST]), _PUBLIC_HOST, peer=_STRANGER_IP) + assert body["host"] == "internal" + + async def test_leftmost_value_is_used(self) -> None: + body = await _get(_mw([_PUBLIC_HOST]), f"{_PUBLIC_HOST}, evil.example.net") + assert body["host"] == _PUBLIC_HOST + + async def test_leftmost_forged_value_is_not_rescued_by_a_later_good_one(self) -> None: + body = await _get(_mw([_PUBLIC_HOST]), f"evil.example.net, {_PUBLIC_HOST}") + assert body["host"] == "internal" + + async def test_wildcard_matches_subdomains_not_the_apex(self) -> None: + mw = _mw(["*.example.com"]) + assert (await _get(mw, "acme.example.com"))["host"] == "acme.example.com" + assert (await _get(mw, "example.com"))["host"] == "internal" + + async def test_port_is_kept_and_checked(self) -> None: + mw = _mw([f"{_PUBLIC_HOST}:8443"]) + assert (await _get(mw, f"{_PUBLIC_HOST}:8443"))["url"].startswith( + f"http://{_PUBLIC_HOST}:8443" + ) + assert (await _get(mw, f"{_PUBLIC_HOST}:9"))["host"] == "internal" + + @pytest.mark.parametrize("value", ["a b.example.com", "app.example.com/x", "u@app.example.com"]) + async def test_malformed_values_are_ignored(self, value: str) -> None: + assert (await _get(_mw(["*.example.com"]), value))["host"] == "internal" + + +class TestPeers: + @pytest.mark.parametrize( + ("spec", "peer", "trusted"), + [ + ("*", _STRANGER_IP, True), + ("10.0.0.0/8", _PROXY_IP, True), + ("10.0.0.0/8", _STRANGER_IP, False), + ("192.0.2.1, 10.1.2.3", _PROXY_IP, True), + ("10.0.0.0/8", None, False), + ], + ) + def test_trusted_hosts_rule(self, spec: str, peer: str | None, trusted: bool) -> None: + assert (peer in TrustedPeers(spec)) is trusted + + def test_allowlist_skips_invalid_entries(self) -> None: + assert not HostAllowlist(["bad entry/", ""]).allows("app.example.com", None) diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index e66eb60e3..d1c1af1ad 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -2,6 +2,7 @@ CLAUDE.md spells out the pipeline: + (ForwardedHost → ProxyHeaders, opt-in; pinned in test_forwarded_host.py) CorrelationId → RequestLogging → BodyLimit → GZip → Security → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → RateLimit → Setup → Maintenance diff --git a/modules/tenants/tests/test_subdomains_forwarded_host.py b/modules/tenants/tests/test_subdomains_forwarded_host.py new file mode 100644 index 000000000..3a60d665a --- /dev/null +++ b/modules/tenants/tests/test_subdomains_forwarded_host.py @@ -0,0 +1,66 @@ +"""#528: behind a trusted proxy, the allowlisted forwarded host picks the subdomain tenant.""" + +from __future__ import annotations + +import httpx +from fastapi import Depends +from simple_module_db import MultiTenantMixin, create_module_base, tenant_context +from simple_module_db.deps import get_db +from simple_module_hosting._forwarded_host import ForwardedHostMiddleware +from sqlalchemy import select +from sqlmodel import Field +from tenants.host_resolver import forget_hosts + +_Base = create_module_base("fwdhost") +_PROXY_IP = "10.9.8.7" +_INTERNAL = "internal.local" +_ALLOWLIST = ["*.example.com"] + + +class _Page(_Base, MultiTenantMixin, table=True): # ty: ignore[unsupported-base] + __tablename__ = "fwdhost_page" + id: int | None = Field(default=None, primary_key=True) + title: str = Field(max_length=50) + + +async def _site(app, user_client): + app.state.tenants.settings.subdomain_base = "example.com" + forget_hosts() + async with app.state.sm.db.engine.begin() as conn: + await conn.run_sync(_Base.metadata.create_all) + + async def titles(db=Depends(get_db)): + return (await db.execute(select(_Page.title))).scalars().all() + + app.add_api_route("/public/fwd-pages", titles, methods=["GET"]) + app.state.public_routes.add_prefix("/public/", methods={"GET"}) + async with user_client("o@x.io") as (owner, _): + tenant = (await owner.post("/api/tenants/", json={"name": "acme", "slug": "acme"})).json() + async with app.state.sm.db.session_factory() as db: + with tenant_context(tenant["id"]): + db.add(_Page(title="acme-home")) + await db.commit() + + +def _via_proxy(app, peer: str) -> httpx.AsyncClient: + wrapped = ForwardedHostMiddleware(app, trusted_hosts=_PROXY_IP, allowlist=_ALLOWLIST) + transport = httpx.ASGITransport(app=wrapped, client=(peer, 1)) + return httpx.AsyncClient(transport=transport, base_url=f"http://{_INTERNAL}") + + +async def test_wildcard_allowlist_resolves_the_subdomain_tenant(app, user_client): + await _site(app, user_client) + try: + headers = {"X-Forwarded-Host": "acme.example.com"} + async with _via_proxy(app, _PROXY_IP) as client: + assert (await client.get("/public/fwd-pages", headers=headers)).json() == ["acme-home"] + # Same header from an untrusted peer: ignored, so no tenant is bound. + async with _via_proxy(app, "198.51.100.1") as client: + assert (await client.get("/public/fwd-pages", headers=headers)).status_code == 403 + # A host outside the allowlist never reaches the resolver. + async with _via_proxy(app, _PROXY_IP) as client: + forged = {"X-Forwarded-Host": "acme.evil.net"} + assert (await client.get("/public/fwd-pages", headers=forged)).status_code == 403 + finally: + app.state.tenants.settings.subdomain_base = "" + forget_hosts() From 34ba396f5b4cbfe43031477c8dc6eba9a7750c0c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 17:00:11 +0200 Subject: [PATCH 04/12] feat(hosting): configurable CORS with per-path policies; public files readable cross-origin (#527) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 6 ++ CLAUDE.md | 8 +- docs/.vitepress/config.ts | 1 + docs/framework/cors.md | 48 ++++++++++++ docs/modules/file_storage.md | 7 ++ .../hosting/simple_module_hosting/_cors.py | 78 +++++++++++++++++++ .../simple_module_hosting/_phase_helpers.py | 13 +++- .../tests/test_build_time_settings_marked.py | 6 -- .../hosting/tests/test_hosting_cors_api.py | 61 +++++++++++++++ .../hosting/tests/test_hosting_cors_unset.py | 19 +++++ .../hosting/tests/test_middleware_order.py | 4 +- .../file_storage/file_storage/constants.py | 6 ++ modules/file_storage/file_storage/cors.py | 43 ++++++++++ modules/file_storage/file_storage/module.py | 9 +++ modules/file_storage/file_storage/settings.py | 8 ++ .../tests/test_file_storage_public_cors.py | 51 ++++++++++++ skills/simple-module-creating/SKILL.md | 1 + 17 files changed, 357 insertions(+), 12 deletions(-) create mode 100644 docs/framework/cors.md create mode 100644 framework/hosting/simple_module_hosting/_cors.py create mode 100644 framework/hosting/tests/test_hosting_cors_api.py create mode 100644 framework/hosting/tests/test_hosting_cors_unset.py create mode 100644 modules/file_storage/file_storage/cors.py create mode 100644 modules/file_storage/tests/test_file_storage_public_cors.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 160318854..048497d57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Added +- Configurable CORS (#527). `cors_allow_origins` (plus `cors_allow_credentials`, + `cors_allow_methods`, `cors_allow_headers`, `cors_expose_headers`, `cors_max_age`; + restart-required) registers a policy on `/api/`; modules add policies with the new + `register_cors_policies` hook. The dispatcher runs before `BodyLimit`, so preflights + never reach auth and a 413 keeps its CORS headers. `file_storage` serves public + files cross-origin (Range-capable) unless the new `public_files_cors` setting is off. - New `mail` module (`simple_module_mail`) owns outgoing mail: the SMTP/console transport, its settings (`/admin/settings/`, package `mail`) and the on-demand `mail.mailer` health check behind "Test mailer connection". Modules send through diff --git a/CLAUDE.md b/CLAUDE.md index e4a1516ae..5eb3bd2d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,7 +162,8 @@ every Tailwind utility, so keep tokens in `theme.css` and rules in `styles.css` `docs/framework/lifecycle.md`): `register_settings`, `register_menu_items`, `register_permissions`, `register_feature_flags`, `register_event_handlers`, `register_invalidations`, `register_health_checks`, `register_setup_steps`, -`register_public_routes`, `register_body_limits`, `register_csp_sources`, +`register_public_routes`, `register_body_limits`, `register_cors_policies`, +`register_csp_sources`, `register_design_packs`, `register_audit_links`, `register_exception_handlers`, `register_middleware`, `register_routes(api_router, view_router)`, `register_admin_routes(admin_router)` @@ -223,6 +224,11 @@ Maintenance → CommitBeforeResponse → app`. Rules that follow from it: off by default; Redis via `SM_REDIS_URL`, else per-worker). - Request bodies over `max_request_body_bytes` (10 MiB) are refused before any module sees them; per-path overrides via `register_body_limits`. +- CORS is opt-in per path: `cors_allow_origins` registers a policy on `/api/`, + and a module adds its own via `register_cors_policies(registry)` + (`CorsPolicy`, most specific match wins). The dispatcher runs before + BodyLimit, so preflights skip auth and a 413 keeps its CORS headers. + `docs/framework/cors.md`. Per-middleware detail: `docs/framework/middleware.md`, `docs/framework/request-guards.md`. diff --git a/docs/.vitepress/config.ts b/docs/.vitepress/config.ts index 69dc90d19..65b903188 100644 --- a/docs/.vitepress/config.ts +++ b/docs/.vitepress/config.ts @@ -125,6 +125,7 @@ export default defineConfig({ { text: "Principal resolvers", link: "/framework/principal-resolvers" }, { text: "User directory", link: "/framework/user-directory" }, { text: "Public routes", link: "/framework/public-routes" }, + { text: "CORS", link: "/framework/cors" }, { text: "Events", link: "/framework/events" }, { text: "Cache invalidation", link: "/framework/invalidation" }, { text: "Multi-tenancy", link: "/framework/multi-tenancy" }, diff --git a/docs/framework/cors.md b/docs/framework/cors.md new file mode 100644 index 000000000..34cab3990 --- /dev/null +++ b/docs/framework/cors.md @@ -0,0 +1,48 @@ +# CORS + +Cross-origin access is opt-in per path. A request no policy matches passes +through untouched and gets no CORS headers. + +## Host API policy + +Set `cors_allow_origins` (Settings admin screen, group CORS, or +`SM_CORS_ALLOW_ORIGINS`) and the host registers one policy on the `/api/` prefix. +Empty (the default) registers nothing. + +| Setting | Default | +|---|---| +| `cors_allow_origins` | `[]` (no policy) | +| `cors_allow_credentials` | `true` | +| `cors_allow_methods` | `GET, POST, PUT, PATCH, DELETE` | +| `cors_allow_headers` | `Authorization, Content-Type, X-CSRF-Token` | +| `cors_expose_headers` | `[]` | +| `cors_max_age` | `600` | + +`*` is refused while `cors_allow_credentials` is on. All of these are +restart-required: the middleware is built at boot. + +## Module policies + +A module with a public, cross-origin surface declares it in +`register_cors_policies(registry)`: + +```python +def register_cors_policies(self, registry): + registry.add_regex( + PUBLIC_PATH_PATTERN, + CorsPolicy(origins=("*",), allow_credentials=False, methods=("GET", "HEAD")), + enabled=lambda: self._toggle_is_on(), + ) +``` + +`add_exact`, `add_prefix` and `add_regex` match on the request path. The most +specific match wins (exact, then regex, then longest prefix); a tie keeps the +first registration and logs a warning. `enabled=` is evaluated per request, so a +DB-backed toggle needs no restart. + +## Where it runs + +`CorsMiddleware` sits between `RequestLogging` and `BodyLimit`. A preflight is +answered before auth, rate limiting, the setup gate or maintenance mode can +reject it, and a `413` from `BodyLimit` still carries the CORS headers the +browser needs to read it. One Starlette `CORSMiddleware` is built per policy. diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index 6bacb022a..815b7fab1 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -64,6 +64,13 @@ Set it with `public=true` on upload or `PATCH /files/{id}`; both need so consumers never build the URL themselves — an `` on a public page can use it, or `.../public/{id}/thumbnail?w=256`. +Public file reads are readable cross-origin: any origin, no credentials, `GET`/`HEAD`/`OPTIONS`, +`Range`, `If-Range` and `If-None-Match` allowed, and `Content-Range`, `Accept-Ranges`, `ETag` and +`Content-Length` exposed, so a page on another site can seek in a video or PDF. The DB-backed +setting `public_files_cors` (default on) turns this off without a restart. With the S3 backend a +read can redirect to a presigned bucket URL; the browser then applies the bucket's own CORS +configuration, which you must set on the bucket. + The public routes are exempt from `AuthMiddleware` through `register_public_routes` (GET only; uploads, PATCH and deletes stay gated). Serving rules: diff --git a/framework/hosting/simple_module_hosting/_cors.py b/framework/hosting/simple_module_hosting/_cors.py new file mode 100644 index 000000000..08d663bfb --- /dev/null +++ b/framework/hosting/simple_module_hosting/_cors.py @@ -0,0 +1,78 @@ +"""CORS dispatch (#527). + +One Starlette ``CORSMiddleware`` is built per distinct :class:`CorsPolicy`; each +request is routed to the one whose policy the registry matches for its method and +path (most specific wins). A request no policy covers passes through untouched, +so a path nobody opted in never gains CORS headers. + +Installed between RequestLogging and BodyLimit, so a preflight is answered before +auth, rate limit, setup or maintenance can reject it, and a 413 from BodyLimit +still carries the CORS headers the browser needs to read it. +""" + +from __future__ import annotations + +from simple_module_core.cors import CorsPolicy, CorsPolicyRegistry +from starlette.middleware.cors import CORSMiddleware +from starlette.types import ASGIApp, Receive, Scope, Send + +from simple_module_hosting.host_settings import HostSettings + +HOST_API_PREFIX = "/api/" + + +def host_policy(settings: HostSettings) -> CorsPolicy | None: + """The ``/api/`` policy from ``cors_*`` settings; ``None`` when no origin is set.""" + # Every field is read even when unset: they are restart-required, and the + # build-time-settings guard learns that from the reads. + fields = ( + settings.cors_allow_origins, + settings.cors_allow_credentials, + settings.cors_allow_methods, + settings.cors_allow_headers, + settings.cors_expose_headers, + settings.cors_max_age, + ) + origins, credentials, methods, headers, expose, max_age = fields + if not origins: + return None + return CorsPolicy( + origins=tuple(origins), + allow_credentials=credentials, + methods=tuple(methods), + headers=tuple(headers), + expose_headers=tuple(expose), + max_age=max_age, + ) + + +class CorsMiddleware: + def __init__(self, app: ASGIApp, *, registry: CorsPolicyRegistry) -> None: + self.app = app + self.registry = registry + self._handlers: dict[CorsPolicy, ASGIApp] = {} + + def _handler(self, policy: CorsPolicy) -> ASGIApp: + handler = self._handlers.get(policy) + if handler is None: + handler = CORSMiddleware( + self.app, + allow_origins=list(policy.origins), + allow_credentials=policy.allow_credentials, + allow_methods=list(policy.methods), + allow_headers=list(policy.headers), + expose_headers=list(policy.expose_headers), + max_age=policy.max_age, + ) + self._handlers[policy] = handler + return handler + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http" or len(self.registry) == 0: + await self.app(scope, receive, send) + return + policy = self.registry.match(scope["method"], scope["path"]) + if policy is None: + await self.app(scope, receive, send) + return + await self._handler(policy)(scope, receive, send) diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 2b1783fd7..369d317b4 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -26,6 +26,7 @@ from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware from simple_module_hosting._body_limit import BodyLimitMiddleware +from simple_module_hosting._cors import HOST_API_PREFIX, CorsMiddleware, host_policy from simple_module_hosting._error_handlers import ( http_exception_handler, not_found_error_handler, @@ -212,10 +213,14 @@ def install_middleware( max_bytes=settings.max_request_body_bytes, registry=getattr(app.state, "body_limits", None), ) - # SLOT: CORS. Add the CORS dispatcher here (batch-1 slice for #527) so it - # executes between RequestLogging and BodyLimit: preflights are answered - # before auth, rate limit, setup and maintenance can reject them. Reads - # ``app.state.cors_policies`` (CorsPolicyRegistry). No-op until then. + # CORS executes between RequestLogging and BodyLimit: preflights are answered + # before auth, rate limit, setup and maintenance can reject them, and a 413 + # still carries the headers. The host's ``/api/`` policy joins the modules'. + cors_registry = app.state.cors_policies + api_policy = host_policy(settings) + if api_policy is not None: + cors_registry.add_prefix(HOST_API_PREFIX, api_policy) + app.add_middleware(CorsMiddleware, registry=cors_registry) app.add_middleware(RequestLoggingMiddleware) app.add_middleware(CorrelationIdMiddleware) # Outermost: rewrite scheme/client from X-Forwarded-* before anything else diff --git a/framework/hosting/tests/test_build_time_settings_marked.py b/framework/hosting/tests/test_build_time_settings_marked.py index 8d6c07516..4e5e9d257 100644 --- a/framework/hosting/tests/test_build_time_settings_marked.py +++ b/framework/hosting/tests/test_build_time_settings_marked.py @@ -31,12 +31,6 @@ _NOT_YET_CONSUMED = frozenset( { "forwarded_host_allowlist", - "cors_allow_origins", - "cors_allow_credentials", - "cors_allow_methods", - "cors_allow_headers", - "cors_expose_headers", - "cors_max_age", "db_pool_timeout", "db_connect_timeout", "db_statement_timeout_ms", diff --git a/framework/hosting/tests/test_hosting_cors_api.py b/framework/hosting/tests/test_hosting_cors_api.py new file mode 100644 index 000000000..0f26bd4a4 --- /dev/null +++ b/framework/hosting/tests/test_hosting_cors_api.py @@ -0,0 +1,61 @@ +"""Configurable CORS on the host API (#527): preflight, allowlist, 413 headers.""" + +from __future__ import annotations + +import httpx +import pytest +from simple_module_hosting.settings import Settings + +ALLOWED = "https://app.example.com" +OTHER = "https://evil.example.com" +BODY_LIMIT = 1000 + + +@pytest.fixture +def settings(settings: Settings) -> Settings: + return settings.model_copy( + update={"cors_allow_origins": [ALLOWED], "max_request_body_bytes": BODY_LIMIT} + ) + + +def _preflight(origin: str) -> dict[str, str]: + return { + "Origin": origin, + "Access-Control-Request-Method": "POST", + "Access-Control-Request-Headers": "authorization,content-type", + } + + +async def test_preflight_from_allowed_origin_never_reaches_auth(client: httpx.AsyncClient): + resp = await client.options("/api/anything", headers=_preflight(ALLOWED)) + assert resp.status_code == 200 + assert resp.headers["access-control-allow-origin"] == ALLOWED + assert resp.headers["access-control-allow-credentials"] == "true" + assert "authorization" in resp.headers["access-control-allow-headers"].lower() + + +async def test_other_origin_gets_no_allow_origin_on_api(client: httpx.AsyncClient): + resp = await client.get("/api/anything", headers={"Origin": OTHER}) + assert "access-control-allow-origin" not in resp.headers + pre = await client.options("/api/anything", headers=_preflight(OTHER)) + assert "access-control-allow-origin" not in pre.headers + + +async def test_simple_request_from_allowed_origin_carries_headers(client: httpx.AsyncClient): + resp = await client.get("/api/anything", headers={"Origin": ALLOWED}) + assert resp.headers["access-control-allow-origin"] == ALLOWED + + +async def test_paths_outside_api_are_untouched(client: httpx.AsyncClient): + resp = await client.get("/health", headers={"Origin": ALLOWED}) + assert "access-control-allow-origin" not in resp.headers + + +async def test_413_from_body_limit_still_carries_cors(client: httpx.AsyncClient): + resp = await client.post( + "/api/anything", + content=b"x" * (BODY_LIMIT + 1), + headers={"Origin": ALLOWED, "Accept": "application/json"}, + ) + assert resp.status_code == 413 + assert resp.headers["access-control-allow-origin"] == ALLOWED diff --git a/framework/hosting/tests/test_hosting_cors_unset.py b/framework/hosting/tests/test_hosting_cors_unset.py new file mode 100644 index 000000000..be6e9b362 --- /dev/null +++ b/framework/hosting/tests/test_hosting_cors_unset.py @@ -0,0 +1,19 @@ +"""With no ``cors_allow_origins`` the host registers no API policy (#527).""" + +from __future__ import annotations + +import httpx + +ORIGIN = "https://app.example.com" + + +async def test_no_origins_means_no_cors_headers(client: httpx.AsyncClient): + resp = await client.options( + "/api/anything", + headers={"Origin": ORIGIN, "Access-Control-Request-Method": "GET"}, + ) + assert "access-control-allow-origin" not in resp.headers + + +async def test_no_host_policy_registered(app): + assert app.state.cors_policies.match("GET", "/api/anything") is None diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index e66eb60e3..3608d10e1 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -2,7 +2,7 @@ CLAUDE.md spells out the pipeline: - CorrelationId → RequestLogging → BodyLimit → GZip → Security → Session → + CorrelationId → RequestLogging → CORS → BodyLimit → GZip → Security → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → RateLimit → Setup → Maintenance → CommitBeforeResponse → app @@ -80,6 +80,7 @@ _EXPECTED_MULTI_TENANT = ( "CorrelationIdMiddleware", "RequestLoggingMiddleware", + "CorsMiddleware", "BodyLimitMiddleware", "GZipMiddleware", "SecurityHeadersMiddleware", @@ -101,6 +102,7 @@ _EXPECTED_SINGLE_TENANT = ( "CorrelationIdMiddleware", "RequestLoggingMiddleware", + "CorsMiddleware", "BodyLimitMiddleware", "GZipMiddleware", "SecurityHeadersMiddleware", diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index 6bcce228f..fa7aa86ab 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -238,3 +238,9 @@ class I18nKey: MENU_ORDER: Final = 40 ADMIN_ROLE: Final = "admin" USER_ROLE: Final = "user" + +# CORS for anonymous public-file reads (#527). Range-capable so a cross-origin +#