diff --git a/CHANGELOG.md b/CHANGELOG.md index 94bd54379..1cbacc795 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,31 @@ All notable changes to this project are documented in this file. The format is b - `host/client_app` no longer re-declares `@simple-module-py/ui`'s runtime dependencies, nor `@base-ui/react`, `@radix-ui/react-slot`, `autoprefixer` or `postcss` (nothing loaded them) (#521). ### 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`. +- `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. +- 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. +- PostgreSQL pool, connect, statement and idle-in-transaction timeout settings for web and worker engines, with worker pool and statement overrides (#531). +- The OpenAPI export declares `bearerAuth` and `sessionCookie` security schemes and a + `security` entry per operation (`[]` for public routes), so generated clients know where + credentials go (#534). New `Authenticated()` marker dependency + (`simple_module_hosting.token_guard`) for routes any signed-in user may call, and diagnostic + `SM031` (WARNING, dev boot) for `/api` routes with no gate and no public match. Marked + `Authenticated()`: `GET/POST /api/tenants/`, `POST /api/tenants/{tenant_id}/switch`, + `DELETE /api/tenants/current/membership`, `POST /api/tenants/invitations/accept`, + `GET/PATCH /api/users/me`, `POST /api/users/me/password`, + `POST /api/users/me/sessions/revoke-all`. Tokens are still refused on these. - Release process (#510): `release.yml` runs the whole PR suite (`pr.yml` via `workflow_call`) against the dispatched commit and `build` needs it, so nothing is built or published from a red commit. The duplicated bump + lockfile steps @@ -76,6 +101,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). - `scripts/bump_version.py` also bumps every `modules/*/package.json`; all `package.json` versions are now in lockstep with the Python packages (they were 0.1.0, keycloak 0.0.0) (#510). diff --git a/CLAUDE.md b/CLAUDE.md index e08533da1..e45ca00d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,7 +162,9 @@ 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_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)` @@ -209,8 +211,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: @@ -220,9 +222,18 @@ 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`. +- 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`. @@ -465,7 +476,8 @@ settings not on `app.state`, `SM013`–`SM016` locale drift, `SM017` module pages without `package.json`/`tsconfig.json`, `SM018` Inertia call to `/api`, `SM019` views with no menu item or permission, `SM022`/`SM023` CSS in the wrong file, `SM024` unique key without `tenant_id`, `SM028` heavy import in -`tasks.py`. +`tasks.py`, `SM031` `/api` route with no permission/`Authenticated()` gate and not public (dev boot +only: it needs the built app). ## Tests & fixtures diff --git a/docs/.vitepress/config.ts b/docs/.vitepress/config.ts index 6cfbca30e..b07f52074 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: "Deprecations", link: "/framework/deprecations" }, { text: "Events", link: "/framework/events" }, { text: "Cache invalidation", link: "/framework/invalidation" }, diff --git a/docs/database/tuning.md b/docs/database/tuning.md new file mode 100644 index 000000000..92b770659 --- /dev/null +++ b/docs/database/tuning.md @@ -0,0 +1,23 @@ +# Database pool and timeout tuning + +The application PostgreSQL engine uses `SM_DB_POOL_TIMEOUT=30` seconds to wait +for a pooled connection and `SM_DB_CONNECT_TIMEOUT=10` seconds to connect. +`SM_DB_STATEMENT_TIMEOUT_MS=0` and `SM_DB_IDLE_IN_TX_TIMEOUT_MS=0` omit their +PostgreSQL startup parameters; positive values set limits in milliseconds. +All four settings require a process restart. SQLite ignores these settings, +including pool timeout. Alembic and the pre-app settings read do not use them. + +Set asyncpg timeouts through the host settings, not URL query parameters. +Asyncpg receives server settings; psycopg/psycopg2 receive libpq startup options. +PgBouncer transaction pooling rejects startup parameters such as these timeout +settings. Leave the millisecond settings at zero and configure limits on the +server or database role when using that mode. + +Background workers inherit the resolved host timeouts. Their sync engine and +each outbox thread's async engine have separate pools, defaulting to size 2, +overflow 3 and recycling after 1800 seconds. Size the database connection budget +for all processes and outbox threads. Override these through +`SM_BG_TASKS_DB_POOL_SIZE`, `SM_BG_TASKS_DB_MAX_OVERFLOW` and +`SM_BG_TASKS_DB_RECYCLE`. Optional `SM_BG_TASKS_DB_STATEMENT_TIMEOUT_MS` +overrides the host statement limit, including zero to omit it. Restart workers +after changing these settings. 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/framework/lifecycle.md b/docs/framework/lifecycle.md index a7b076dcf..e63433608 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/middleware.md b/docs/framework/middleware.md index 1515c7779..f6f3b6540 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 @@ -94,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 rightmost `X-Forwarded-Host` value (the one the trusted proxy added) 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/docs/framework/permissions.md b/docs/framework/permissions.md index 400997da8..3e2f4e0ec 100644 --- a/docs/framework/permissions.md +++ b/docs/framework/permissions.md @@ -115,6 +115,10 @@ dependency) applies a fail-closed rule to every route: refuses tokens with 403, gated or not; - `Depends(TokenAllowed())` from `simple_module_hosting.token_guard` admits tokens explicitly, without a permission; +- `Depends(Authenticated())` marks "any signed-in user may call this" (401 with + no principal). It satisfies diagnostic `SM031` and the OpenAPI export but is + **not** a token opt-in: alone, a token is still refused; add `TokenAllowed()` + to admit one too; - every other route refuses tokens with 403 — except a public route, where the token's principal is dropped and the request runs anonymously (no user, no permissions, and the tenant an anonymous request would resolve to). 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/docs/guide/openapi-clients.md b/docs/guide/openapi-clients.md index 70dc55283..98f0835bf 100644 --- a/docs/guide/openapi-clients.md +++ b/docs/guide/openapi-clients.md @@ -27,6 +27,24 @@ The bare `/openapi.json` is gone. It used to be served in production too; use `/api/openapi.json`. ::: +## Security schemes + +The schema declares two `components.securitySchemes`: `bearerAuth` (`http`, +`bearer` — personal access tokens, service keys and Keycloak access tokens) and +`sessionCookie` (`apiKey` in a cookie named `session`). Every operation carries +`security`: `[]` when anonymous callers are let through (a `register_public_routes` +rule matched on the templated path with each `{param}` replaced by a placeholder +segment, the auth provider's legacy public paths, or the schema URL itself), and +`[{bearerAuth: []}, {sessionCookie: []}]` otherwise, so a generated client sends +credentials only where needed. `bearerAuth` covers provider access tokens +(e.g. Keycloak) on every protected route, while personal API tokens are accepted +only on routes marked `TokenAllowed()`. Rules that need a specific path segment (a regex +on a slug) can over- or under-report here; `AuthMiddleware` stays the authority. + +Diagnostic `SM031` warns at a dev boot about an `/api` route with no permission, +`TokenAllowed` or `Authenticated` gate that is not public (see +[diagnostic codes](/reference/diagnostic-codes)). + ## Exporting without a server ```bash diff --git a/docs/modules/background_tasks.md b/docs/modules/background_tasks.md index a68f07836..0c3d08c0f 100644 --- a/docs/modules/background_tasks.md +++ b/docs/modules/background_tasks.md @@ -527,3 +527,13 @@ def convert(file_id: int) -> None: ``` It reads SYSTEM-scope overrides through the worker's sync session and caches per process for 30 seconds (`ttl=0` always re-reads; `clear_settings_cache()` drops the cache). The underlying sync helper is `simple_module_db.stored_settings.hydrate_settings_sync`. + +## Worker database tuning + +Sync task sessions and async outbox sessions inherit host pool, connect, +statement and idle-in-transaction timeouts. Worker pools default to size 2, +overflow 3 and recycling after 1800 seconds. Set `SM_BG_TASKS_DB_POOL_SIZE`, +`SM_BG_TASKS_DB_MAX_OVERFLOW`, `SM_BG_TASKS_DB_RECYCLE` and optionally +`SM_BG_TASKS_DB_STATEMENT_TIMEOUT_MS` to tune them; restart workers afterward. +Each outbox thread owns a separate pool. PgBouncer transaction pooling rejects +timeout startup parameters; see [database tuning](../database/tuning.md). diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index 43d372b99..7633826b1 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -72,6 +72,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/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index 1876cb814..898a911a7 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -39,6 +39,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM028` | WARNING | A module's `tasks.py` imports `torch`, `tensorflow`, `ultralytics`, `osgeo`, `rasterio`, `cv2` or `sklearn` at module level, so every process that loads the module's tasks (web, each worker, beat) pays for it and a queue-restricted worker cannot avoid it. | Import the package inside the task function. See [background_tasks](/modules/background_tasks#running-a-worker-for-a-subset-of-modules). | | `SM029` | INFO | A module overrides `db_extensions()` (Postgres extensions such as `ltree`) and the configured database is SQLite, which has none. `ensure_db_extensions` raises `UnsupportedDatabaseError` when migrations run. | Point `SM_DATABASE_URL` at PostgreSQL. See [Postgres extensions and column types](/database/extensions). | | `SM030` | WARNING at a dev boot; ERROR in `make doctor` | A module's `depends_on` names a module that is not loaded (not installed, left out of `SM_MODULES_ENABLED`, or dropped because another auth provider was selected — `keycloak` drops `Users`). Outside development the boot raises `MissingDependencyError` listing every pair. | Install and enable the dependency, or leave the dependent module out. A relationship that is only about order belongs in `optional_depends_on`. See [Discovery](/framework/discovery#modulemeta). | +| `SM031` | WARNING | An `/api` route has no permission, `TokenAllowed` or `Authenticated` gate anywhere in its dependency tree (include-level dependencies count) and no public-route match, so the schema cannot say who may call it. Needs the built app: reported at a **dev boot** (and in the Doctor screen), not by `make doctor`. | Add `Depends(RequiresPermission(...))`, or `Depends(Authenticated())` when any signed-in user may call it, or register it in `register_public_routes`. | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). @@ -46,7 +47,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | Context | What runs | |---|---| -| App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. | +| App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. After the routes are wired, SM031 (unguarded `/api` routes) is added. | | App boot in non-development | Strict module discovery (raises on SM001-class failures) + the migration check (SM010) + SM025 (logged as a warning). The page/locale static suite is dev-only. | Sample dev-mode output: diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index ef20a05fc..1180e050f 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -30,6 +30,10 @@ This is the full reference. See [Configuration](/guide/configuration) for a narr | `SM_DB_MAX_OVERFLOW` | `20` | SQLAlchemy `max_overflow` (per process). | | `SM_DB_POOL_PRE_PING` | `true` | Test connections before use. | | `SM_DB_POOL_RECYCLE` | `1800` | Recycle connections after N seconds (helps with LB idle drops). | +| `SM_DB_POOL_TIMEOUT` | `30` | Seconds waiting for a pooled connection. | +| `SM_DB_CONNECT_TIMEOUT` | `10` | Driver connection timeout in seconds. | +| `SM_DB_STATEMENT_TIMEOUT_MS` | `0` | PostgreSQL statement timeout; zero omits the startup parameter. | +| `SM_DB_IDLE_IN_TX_TIMEOUT_MS` | `0` | PostgreSQL idle-in-transaction timeout; zero omits the startup parameter. | Pools are **per process**. With multiple `uvicorn --workers`, total connections = `workers × (SM_DB_POOL_SIZE + SM_DB_MAX_OVERFLOW)`; keep it under the database's @@ -135,3 +139,15 @@ Loads only the listed modules. Useful for: - **Tests** — `SM_MODULES_ENABLED=users,orders` for a minimal app. - **Worker processes** — if you run a Celery worker as a separate container, it only needs `background_tasks` and whatever modules define tasks it processes. - **Emergency disable** — hide a broken module without rebuilding the image. Remove it from the list, restart. + + +Worker database settings (restart required; env → DB → default): + +| Variable | Default | Purpose | +|---|---|---| +| `SM_BG_TASKS_DB_POOL_SIZE` | `2` | Pool size for each worker engine. | +| `SM_BG_TASKS_DB_MAX_OVERFLOW` | `3` | Overflow connections per worker engine. | +| `SM_BG_TASKS_DB_RECYCLE` | `1800` | Connection recycling interval in seconds. | +| `SM_BG_TASKS_DB_STATEMENT_TIMEOUT_MS` | unset | Override host statement timeout; zero omits it. | + +See [database tuning](../database/tuning.md) for timeout and PgBouncer details. 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..0b52a22b9 --- /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. Rightmost `X-Forwarded-Host` (added by the trusted proxy); 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 1ca54c1d7..e7c86762e 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..b45de98b1 --- /dev/null +++ b/framework/core/simple_module_core/path_rules.py @@ -0,0 +1,121 @@ +"""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, int]: + # A rule scoped to some methods beats an unscoped one on the same path, so + # the winner does not depend on which module registered first. + return ( + _RANK[self.kind], + len(self.pattern) if self.kind == KIND_PREFIX else 0, + int(self.methods is not None), + ) + + +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*.""" + 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): + 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.kind, best.pattern, 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..31f267bde 100644 --- a/framework/core/simple_module_core/rate_limit.py +++ b/framework/core/simple_module_core/rate_limit.py @@ -18,11 +18,16 @@ 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", + "RateKey", "RateLimitError", + "RateLimitRegistry", + "RateLimitRule", "RateSpec", "WindowResult", "WindowStore", @@ -167,3 +172,57 @@ 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 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/core/tests/test_path_rule_registries.py b/framework/core/tests/test_path_rule_registries.py new file mode 100644 index 000000000..fbf962f99 --- /dev/null +++ b/framework/core/tests/test_path_rule_registries.py @@ -0,0 +1,78 @@ +"""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") + + +def test_method_scoped_rule_beats_unscoped_one_regardless_of_order() -> None: + reg = RateLimitRegistry() + reg.add_exact("/api/x", RateLimitRule("100/minute")) + reg.add_exact("/api/x", RateLimitRule("5/minute", methods={"POST"})) + assert reg.match("POST", "/api/x").rate == "5/minute" + assert reg.match("GET", "/api/x").rate == "100/minute" diff --git a/framework/db/simple_module_db/engine_options.py b/framework/db/simple_module_db/engine_options.py new file mode 100644 index 000000000..33bf847dd --- /dev/null +++ b/framework/db/simple_module_db/engine_options.py @@ -0,0 +1,53 @@ +"""Driver-specific startup timeouts, shared by web and worker engines.""" + +from typing import Any + +from sqlalchemy.engine import make_url + +_ASYNCPG = "asyncpg" +_LIBPQ_DRIVERS = frozenset({"psycopg", "psycopg2"}) +_POSTGRESQL = "postgresql" +_STATEMENT = "statement_timeout" +_IDLE = "idle_in_transaction_session_timeout" +_TIMEOUT = "timeout" +_SERVER_SETTINGS = "server_settings" +_CONNECT_TIMEOUT = "connect_timeout" +_OPTIONS = "options" + + +def connect_args( + database_url: str, + *, + connect_timeout: int = 10, + statement_timeout_ms: int = 0, + idle_in_tx_timeout_ms: int = 0, +) -> dict[str, Any]: + """Build startup arguments without opening a connection; zero omits a limit.""" + url = make_url(database_url) + if url.get_backend_name() != _POSTGRESQL: + return {} + settings = { + key: str(value) + for key, value in ( + (_STATEMENT, statement_timeout_ms), + (_IDLE, idle_in_tx_timeout_ms), + ) + if value + } + driver = url.get_driver_name() + args: dict[str, Any] = {} + if driver == _ASYNCPG: + if connect_timeout: + args[_TIMEOUT] = connect_timeout + if settings: + args[_SERVER_SETTINGS] = settings + elif driver in _LIBPQ_DRIVERS: + if connect_timeout and _CONNECT_TIMEOUT not in url.query: # URL wins + args[_CONNECT_TIMEOUT] = connect_timeout + if settings: + # connect_args replace the URL's own ``options``; keep the operator's flags. + own = url.query.get(_OPTIONS) + own_flags = [own] if isinstance(own, str) else list(own or ()) + flags = [f"-c {key}={value}" for key, value in settings.items()] + args[_OPTIONS] = " ".join([*own_flags, *flags]) + return args diff --git a/framework/db/simple_module_db/session.py b/framework/db/simple_module_db/session.py index 709cd31cc..39ec0c097 100644 --- a/framework/db/simple_module_db/session.py +++ b/framework/db/simple_module_db/session.py @@ -15,6 +15,7 @@ from sqlalchemy.orm import Session from simple_module_db.callbacks import OnCommitCallback, register_on_commit +from simple_module_db.engine_options import connect_args as timeout_connect_args from simple_module_db.provider import DatabaseProvider, detect_provider from simple_module_db.tenancy import DEFAULT_TENANT_ID @@ -57,6 +58,10 @@ def init_db( max_overflow: int = 20, pool_pre_ping: bool = True, pool_recycle: int = 1800, + pool_timeout: int = 30, + connect_timeout: int = 10, + statement_timeout_ms: int = 0, + idle_in_tx_timeout_ms: int = 0, poolclass: type | None = None, sqlite_wal: bool = True, sqlite_busy_timeout_ms: int = 5000, @@ -88,7 +93,12 @@ def init_db( """ provider = detect_provider(database_url) - connect_args: dict = {} + connect_args = timeout_connect_args( + database_url, + connect_timeout=connect_timeout, + statement_timeout_ms=statement_timeout_ms, + idle_in_tx_timeout_ms=idle_in_tx_timeout_ms, + ) engine_kwargs: dict = {"echo": echo, "connect_args": connect_args} if poolclass is not None: engine_kwargs["poolclass"] = poolclass @@ -96,6 +106,7 @@ def init_db( connect_args["check_same_thread"] = False elif poolclass is None: engine_kwargs.update( + pool_timeout=pool_timeout, pool_size=pool_size, max_overflow=max_overflow, pool_pre_ping=pool_pre_ping, diff --git a/framework/db/tests/test_db_engine_options.py b/framework/db/tests/test_db_engine_options.py new file mode 100644 index 000000000..df737d0cf --- /dev/null +++ b/framework/db/tests/test_db_engine_options.py @@ -0,0 +1,106 @@ +"""Driver timeout mappings and application engine wiring (#531).""" + +import pytest +from simple_module_db.engine_options import connect_args +from simple_module_db.session import init_db +from simple_module_test.database import database_url_for_tests +from sqlalchemy import event, text +from sqlalchemy.exc import DBAPIError +from sqlalchemy.pool import NullPool, StaticPool + + +@pytest.mark.parametrize("driver", ["psycopg2", "psycopg"]) +def test_libpq(driver): + assert connect_args( + f"postgresql+{driver}://localhost/db", + connect_timeout=7, + statement_timeout_ms=1000, + idle_in_tx_timeout_ms=2000, + ) == { + "connect_timeout": 7, + "options": "-c statement_timeout=1000 -c idle_in_transaction_session_timeout=2000", + } + + +def test_asyncpg(): + assert connect_args( + "postgresql+asyncpg://localhost/db", + connect_timeout=7, + statement_timeout_ms=1000, + idle_in_tx_timeout_ms=2000, + ) == { + "timeout": 7, + "server_settings": { + "statement_timeout": "1000", + "idle_in_transaction_session_timeout": "2000", + }, + } + + +@pytest.mark.parametrize("driver", ["asyncpg", "psycopg2", "psycopg"]) +def test_zero_omits_keys(driver): + assert connect_args(f"postgresql+{driver}://localhost/db", connect_timeout=0) == {} + + +def test_sqlite(): + assert connect_args("sqlite+aiosqlite://", statement_timeout_ms=1000) == {} + + +async def test_init_db_options(): + state = init_db( + "postgresql+asyncpg://localhost/db", + pool_timeout=17, + connect_timeout=4, + statement_timeout_ms=123, + idle_in_tx_timeout_ms=456, + ) + assert state.engine.pool.timeout() == 17 + captured = {} + + @event.listens_for(state.engine.sync_engine, "do_connect") + def capture(dialect, record, args, kwargs): + captured.update(kwargs) + raise RuntimeError("captured") + + try: + with pytest.raises(RuntimeError, match="captured"): + async with state.engine.connect(): + pass + assert captured["timeout"] == 4 + assert captured["server_settings"] == { + "statement_timeout": "123", + "idle_in_transaction_session_timeout": "456", + } + finally: + await state.engine.dispose() + + +async def test_sqlite_ignores_pool_timeout(): + state = init_db("sqlite+aiosqlite://", pool_timeout=17, statement_timeout_ms=1000) + try: + assert isinstance(state.engine.pool, StaticPool) + finally: + await state.engine.dispose() + + +@pytest.mark.postgres +async def test_web_statement_timeout(): + state = init_db( + database_url_for_tests(), + statement_timeout_ms=1000, + poolclass=NullPool, + ) + try: + async with state.engine.connect() as connection: + with pytest.raises(DBAPIError, match="statement timeout"): + await connection.execute(text("SELECT pg_sleep(2)")) + finally: + await state.engine.dispose() + + +def test_libpq_url_options_and_connect_timeout_survive() -> None: + args = connect_args( + "postgresql+psycopg://u@h/db?options=-c%20search_path%3Dfoo&connect_timeout=3", + statement_timeout_ms=500, + ) + assert args == {"options": "-c search_path=foo -c statement_timeout=500"} diff --git a/framework/hosting/simple_module_hosting/_boot_checks.py b/framework/hosting/simple_module_hosting/_boot_checks.py index bd9aae65e..a8e9a4596 100644 --- a/framework/hosting/simple_module_hosting/_boot_checks.py +++ b/framework/hosting/simple_module_hosting/_boot_checks.py @@ -1,5 +1,8 @@ """Diagnostics that need the built app, so cannot run from ``make doctor``. +SM025 runs in every environment; SM031 (an unguarded ``/api`` route) needs the +finished route table, so it runs at the end of a development boot only. + The module diagnostics in Phase 2 see only module classes and the source tree. A check about what modules *registered* has to wait until after Phase 5, and unlike the source-tree checks it is meaningful in production too. @@ -12,6 +15,8 @@ from simple_module_core.diagnostics import Diagnostic, check_tenant_resolver, print_diagnostics +from simple_module_hosting._route_access import check_unguarded_api_routes + if TYPE_CHECKING: from fastapi import FastAPI from simple_module_core.services import DiagnosticsState @@ -44,4 +49,16 @@ def report_tenant_resolution( return found -__all__ = ["report_tenant_resolution"] +def report_unguarded_routes(app: FastAPI, diagnostics_state: DiagnosticsState) -> list[Diagnostic]: + """SM031 (dev boot only; needs the finished route table): print and fold into Doctor.""" + found = check_unguarded_api_routes(app) + if found: + print_diagnostics(found) + runner = diagnostics_state.runner + if runner is not None: + diagnostics_state.runner = lambda: [*runner(), *check_unguarded_api_routes(app)] + diagnostics_state.results = [*diagnostics_state.results, *found] + return found + + +__all__ = ["report_tenant_resolution", "report_unguarded_routes"] diff --git a/framework/hosting/simple_module_hosting/_cors.py b/framework/hosting/simple_module_hosting/_cors.py new file mode 100644 index 000000000..55a996d31 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_cors.py @@ -0,0 +1,76 @@ +"""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 + + +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/_forwarded_host.py b/framework/hosting/simple_module_hosting/_forwarded_host.py new file mode 100644 index 000000000..68791d3c3 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_forwarded_host.py @@ -0,0 +1,135 @@ +"""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" +_FORWARDED_PROTO_HEADER = b"x-forwarded-proto" +_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 + + +def _forwarded_proto(scope: Scope) -> str | None: + """Rightmost ``X-Forwarded-Proto``: ProxyHeaders has not rewritten the scheme yet.""" + raw = ",".join(v.decode("latin-1") for k, v in scope["headers"] if k == _FORWARDED_PROTO_HEADER) + return raw.split(",")[-1].strip().lower() or 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 + # Rightmost value: the one our trusted proxy added. Earlier values came + # from upstream hops or the client and cannot be trusted (an appending + # proxy keeps whatever the client sent at the front). + parsed = _split_host(raw.split(",")[-1]) + 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")), + ] + proto = _forwarded_proto(scope) or scope.get("scheme", "") + default = _DEFAULT_PORTS.get(proto, _FALLBACK_PORT) + scope["server"] = (host, int(port) if port else default) diff --git a/framework/hosting/simple_module_hosting/_openapi.py b/framework/hosting/simple_module_hosting/_openapi.py index df1fdf94d..4010dc5b9 100644 --- a/framework/hosting/simple_module_hosting/_openapi.py +++ b/framework/hosting/simple_module_hosting/_openapi.py @@ -1,6 +1,6 @@ """OpenAPI export plumbing (#456). -Three small pieces, kept apart from ``create_app``: +Four small pieces, kept apart from ``create_app``: * ``make_unique_id_function`` — stable ``_`` operationIds, so generated clients get method names like ``users_list_users`` instead of @@ -8,6 +8,8 @@ * ``install_openapi_filter`` — the published schema is the JSON API only. View routes are already ``include_in_schema=False``; this also drops any other non-``/api/`` path (host routes, health) so a client generator never sees them. +* ``add_security`` — ``components.securitySchemes`` (``bearerAuth``, + ``sessionCookie``) and per-operation ``security`` (``[]`` when public, #534). * ``API_OPENAPI_URL`` / ``openapi_served`` — where and when it is served. """ @@ -21,8 +23,18 @@ from fastapi import FastAPI from fastapi.routing import APIRoute +from simple_module_hosting._route_access import ( + API_PREFIX, + is_public_operation, + iter_api_operations, +) +from simple_module_hosting.session import SESSION_COOKIE_NAME + __all__ = [ "API_OPENAPI_URL", + "BEARER_SCHEME", + "SESSION_SCHEME", + "add_security", "dedupe_operation_ids", "install_openapi_filter", "make_unique_id_function", @@ -32,7 +44,8 @@ logger = logging.getLogger(__name__) API_OPENAPI_URL = "/api/openapi.json" -_API_PREFIX = "/api/" +BEARER_SCHEME = "bearerAuth" +SESSION_SCHEME = "sessionCookie" def openapi_served(*, is_development: bool, openapi_public: bool) -> bool: @@ -77,6 +90,39 @@ def dedupe_operation_ids(schema: dict) -> None: seen.add(op["operationId"]) +def _security_schemes() -> dict: + return { + # Personal access tokens, service keys, IdP (Keycloak) access tokens. + BEARER_SCHEME: {"type": "http", "scheme": "bearer"}, + SESSION_SCHEME: {"type": "apiKey", "in": "cookie", "name": SESSION_COOKIE_NAME}, + } + + +def add_security(app: FastAPI, schema: dict) -> None: + """Declare the two schemes and set ``security`` on every operation. + + ``[]`` marks an operation anonymous callers reach (OpenAPI's way of + overriding a global requirement); every other one accepts either scheme. + """ + schema.setdefault("components", {}).setdefault("securitySchemes", {}).update( + _security_schemes() + ) + paths = schema.get("paths", {}) + for op in iter_api_operations(app): + item = paths.get(op.path) + if item is None: + continue + for method in op.methods: + operation = item.get(method.lower()) + if not isinstance(operation, dict): + continue + operation["security"] = ( + [] + if is_public_operation(app, method, op.path) + else [{BEARER_SCHEME: []}, {SESSION_SCHEME: []}] + ) + + def install_openapi_filter(app: FastAPI) -> None: """Make ``app.openapi()`` list only ``/api/`` paths.""" original = app.openapi @@ -89,9 +135,10 @@ def openapi() -> dict: warnings.filterwarnings("ignore", message="Duplicate Operation ID") schema = original() schema["paths"] = { - p: v for p, v in schema.get("paths", {}).items() if p.startswith(_API_PREFIX) + p: v for p, v in schema.get("paths", {}).items() if p.startswith(API_PREFIX) } dedupe_operation_ids(schema) + add_security(app, schema) assert app.openapi_schema is not None return app.openapi_schema diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 06371f87d..17be523e4 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -26,15 +26,18 @@ from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware from simple_module_hosting._body_limit import BodyLimitMiddleware +from simple_module_hosting._cors import CorsMiddleware, host_policy from simple_module_hosting._error_handlers import ( http_exception_handler, not_found_error_handler, 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 +from simple_module_hosting._route_access import API_PREFIX from simple_module_hosting.i18n_middleware import LocaleMiddleware from simple_module_hosting.maintenance import MaintenanceMiddleware from simple_module_hosting.middleware import ( @@ -122,7 +125,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 +215,14 @@ def install_middleware( max_bytes=settings.max_request_body_bytes, registry=getattr(app.state, "body_limits", None), ) + # 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(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 @@ -221,6 +233,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) + # 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/simple_module_hosting/_rate_limit.py b/framework/hosting/simple_module_hosting/_rate_limit.py index 0d7a6497b..90bc959af 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,25 @@ 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 KEY_IP, KEY_USER_OR_IP, rule_bucket, rule_key from simple_module_hosting._request_guard_responses import send_guard_response +__all__ = ["RateLimitMiddleware"] + 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 +61,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, KEY_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 +78,63 @@ 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 + # Own namespace: a registry rule on the same pattern must not share + # this counter (one request would count twice, periods would mix). + bucket = f"public:{rule_bucket(rule.kind, rule.pattern, rule.methods)}" + return (bucket, spec, KEY_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, KEY_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..50a9f22cb --- /dev/null +++ b/framework/hosting/simple_module_hosting/_rate_limit_keys.py @@ -0,0 +1,67 @@ +"""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. + + The tenant key applies to signed-in requests only. Anonymous traffic on a tenant + subdomain resolves a tenant too, and sharing that bucket would let one anonymous + client exhaust the whole tenant's budget, so it is counted per IP instead. + """ + if key == KEY_TENANT: + tenant = _state(scope, "tenant_id") + if tenant and _user_id(scope) is not None: + 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/_registrations.py b/framework/hosting/simple_module_hosting/_registrations.py index 168f1f175..d64593505 100644 --- a/framework/hosting/simple_module_hosting/_registrations.py +++ b/framework/hosting/simple_module_hosting/_registrations.py @@ -8,14 +8,36 @@ 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.deprecation import deprecate +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, *, @@ -31,7 +53,7 @@ def run_module_registrations( design_pack_registry, audit_link_registry, csp_registry, - body_limit_registry=None, + request_registries: RequestRegistries, ) -> None: """Invoke each module's registration hooks, in dependency order. @@ -50,8 +72,9 @@ def run_module_registrations( health_registry.set_owner(mod.meta.name) mod.register_health_checks(health_registry) mod.register_public_routes(public_route_registry) - if body_limit_registry is not None: - mod.register_body_limits(body_limit_registry) + mod.register_body_limits(request_registries.body_limits) + mod.register_cors_policies(request_registries.cors) + mod.register_rate_limits(request_registries.rate_limits) 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/_route_access.py b/framework/hosting/simple_module_hosting/_route_access.py new file mode 100644 index 000000000..1a5ceb4d1 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_route_access.py @@ -0,0 +1,118 @@ +"""Who may call each ``/api`` route, read off the built app (#534). + +Feeds two consumers that must agree: the OpenAPI export (``security`` per +operation) and diagnostic ``SM031`` (an ``/api`` route nobody guarded). + +A route is **public** when ``AuthMiddleware`` would let an anonymous caller +through: the public-route registry, the auth provider's legacy public paths, or +the schema URL itself. The middleware sees a concrete request path while a route +only has a template, so each ``{param}`` is replaced by a placeholder segment +and the route counts as public if the registry matches under any placeholder. +A rule that needs a more specific segment (a regex on a slug) can therefore +over- or under-report; the schema is documentation, the middleware stays the +authority. +""" + +from __future__ import annotations + +import re +from collections.abc import Iterator +from typing import NamedTuple + +from fastapi import FastAPI +from fastapi.routing import iter_route_contexts +from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel + +from simple_module_hosting.token_guard import ( + GATE_ALLOW, + GATE_AUTHENTICATED, + GATE_PERMISSION, + dependant_gate_kinds, +) + +__all__ = [ + "API_PREFIX", + "UNGUARDED_ROUTE_CODE", + "ApiOperation", + "check_unguarded_api_routes", + "is_public_operation", + "iter_api_operations", +] + +API_PREFIX = "/api/" +UNGUARDED_ROUTE_CODE = "SM031" +_PARAM = re.compile(r"\{[^}]*\}") +_PLACEHOLDERS = ("x", "0", "00000000-0000-0000-0000-000000000000") +_GUARD_KINDS = frozenset({GATE_PERMISSION, GATE_ALLOW, GATE_AUTHENTICATED}) + + +class ApiOperation(NamedTuple): + path: str # templated, as the OpenAPI schema spells it + methods: frozenset[str] + gate_kinds: frozenset[str] + + +def iter_api_operations(app: FastAPI) -> Iterator[ApiOperation]: + """Each ``/api/`` route with a dependency tree (plain Starlette routes have none).""" + for ctx in iter_route_contexts(app.routes): + path = getattr(ctx, "path_format", None) or ctx.path + dependant = getattr(ctx, "dependant", None) + if not path or not path.startswith(API_PREFIX) or dependant is None: + continue + methods = frozenset(m.upper() for m in (ctx.methods or ()) if m.upper() != "HEAD") + yield ApiOperation(path, methods, frozenset(dependant_gate_kinds(dependant))) + + +def _legacy_public(app: FastAPI, path: str) -> bool: + provider = getattr(getattr(app.state, "auth", None), "auth_provider", None) + if provider is None: + return False + try: + prefixes, exact = provider.get_public_paths() + except Exception: + return False + return any(path.startswith(p) for p in prefixes) or path in exact + + +def is_public_operation(app: FastAPI, method: str, path: str) -> bool: + """Whether an anonymous *method* on the templated *path* is let through.""" + if path == getattr(app.state, "openapi_url", None): + return True + registry = getattr(app.state, "public_routes", None) + for placeholder in _PLACEHOLDERS: + concrete = _PARAM.sub(placeholder, path) + if registry is not None and registry.matches(method, concrete): + return True + if _legacy_public(app, concrete): + return True + return False + + +def check_unguarded_api_routes(app: FastAPI) -> list[Diagnostic]: + """SM031: an ``/api`` route with no permission/allow/authenticated gate and not public.""" + found: list[Diagnostic] = [] + for op in iter_api_operations(app): + if op.gate_kinds & _GUARD_KINDS: + continue + for method in sorted(op.methods): + if is_public_operation(app, method, op.path): + continue + found.append(_unguarded(method, op.path)) + return found + + +def _unguarded(method: str, path: str) -> Diagnostic: + return Diagnostic( + level=DiagnosticLevel.WARNING, + code=UNGUARDED_ROUTE_CODE, + message=( + f"{method} {path} has no permission, allow or authenticated gate and is not " + "a public route; any signed-in user can call it and the OpenAPI schema says nothing " + "about why" + ), + module_name="Hosting", + suggestion=( + "Add Depends(RequiresPermission(...)), or Depends(Authenticated()) when any " + "signed-in user may call it, or register it in register_public_routes" + ), + ) diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 943fa2828..711d6dbaa 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 @@ -22,7 +22,7 @@ from simple_module_db.listeners import register_listeners from simple_module_db.session import init_db -from simple_module_hosting._boot_checks import report_tenant_resolution +from simple_module_hosting._boot_checks import report_tenant_resolution, report_unguarded_routes from simple_module_hosting._db_health import register_database_check from simple_module_hosting._dev_boot import run_dev_boot from simple_module_hosting._inertia_setup import setup_inertia @@ -42,13 +42,14 @@ ) 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._rate_limit_store import build_store +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, register_host_settings, ) +from simple_module_hosting.db_options import database_options from simple_module_hosting.health import router as health_router from simple_module_hosting.i18n_manifest import build_i18n_registry from simple_module_hosting.i18n_routes import router as i18n_router @@ -167,8 +168,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: dependencies=[Depends(token_route_guard)], ) - install_openapi_filter(app) - # Read by AuthMiddleware: the schema is anonymous exactly when it is served. + install_openapi_filter(app) # AuthMiddleware reads openapi_url: anonymous iff served app.state.openapi_url = API_OPENAPI_URL if serve_openapi else None # ── Phase 4: Module settings ─────────────────────────── @@ -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,9 @@ 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, + request_registries=request_registries, ) - 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 @@ -225,10 +225,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: db_state = init_db( settings.database_url, echo=settings.debug, - pool_size=settings.db_pool_size, - max_overflow=settings.db_max_overflow, - pool_pre_ping=settings.db_pool_pre_ping, - pool_recycle=settings.db_pool_recycle, + **database_options(settings), ) # Multi-tenant installs fail closed: a tenant-scoped query with no tenant # context raises instead of reading every tenant's rows. @@ -293,5 +290,6 @@ def create_app(settings: Settings | None = None) -> FastAPI: view_prefixes=view_prefixes, rate_limit_store=rate_limit_store, ) - + if settings.is_development: + report_unguarded_routes(app, diagnostics_state) # SM031 return app diff --git a/framework/hosting/simple_module_hosting/db_options.py b/framework/hosting/simple_module_hosting/db_options.py new file mode 100644 index 000000000..47a48c6cd --- /dev/null +++ b/framework/hosting/simple_module_hosting/db_options.py @@ -0,0 +1,32 @@ +"""Map resolved host settings to the application engine (never bootstrap reads).""" + +from typing import TypedDict + +from simple_module_hosting.host_settings import HostSettings + + +class DatabaseOptions(TypedDict): + """Engine keyword types for SQLAlchemy initialization.""" + + pool_size: int + max_overflow: int + pool_pre_ping: bool + pool_recycle: int + pool_timeout: int + connect_timeout: int + statement_timeout_ms: int + idle_in_tx_timeout_ms: int + + +def database_options(settings: HostSettings) -> DatabaseOptions: + """Read all restart-required engine settings during app construction.""" + return { + "pool_size": settings.db_pool_size, + "max_overflow": settings.db_max_overflow, + "pool_pre_ping": settings.db_pool_pre_ping, + "pool_recycle": settings.db_pool_recycle, + "pool_timeout": settings.db_pool_timeout, + "connect_timeout": settings.db_connect_timeout, + "statement_timeout_ms": settings.db_statement_timeout_ms, + "idle_in_tx_timeout_ms": settings.db_idle_in_tx_timeout_ms, + } 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/simple_module_hosting/rate_limit_deps.py b/framework/hosting/simple_module_hosting/rate_limit_deps.py new file mode 100644 index 000000000..044aab931 --- /dev/null +++ b/framework/hosting/simple_module_hosting/rate_limit_deps.py @@ -0,0 +1,46 @@ +"""``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 +from simple_module_hosting.token_guard import effective_route + +_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 = effective_route(request.scope) # carries the include prefix + 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/simple_module_hosting/session.py b/framework/hosting/simple_module_hosting/session.py index afddc1e63..e5bb559b2 100644 --- a/framework/hosting/simple_module_hosting/session.py +++ b/framework/hosting/simple_module_hosting/session.py @@ -72,6 +72,9 @@ which would leave the hole this key was added to close. """ +SESSION_COOKIE_NAME = "session" +"""The session cookie's name (Starlette's default, pinned so the OpenAPI export can cite it).""" + SESSION_COOKIE_MAX_AGE_KEY = "session_cookie_max_age" """ASGI scope key an endpoint sets to widen *this* response's cookie window. @@ -102,6 +105,7 @@ def __init__(self, app: ASGIApp, secret_key: Any, **kwargs: Any) -> None: "signature window, not the cookie window. Set the cookie window per session " "with SESSION_REMEMBER_KEY, or per response with SESSION_COOKIE_MAX_AGE_KEY." ) + kwargs.setdefault("session_cookie", SESSION_COOKIE_NAME) super().__init__(app, secret_key, max_age=SESSION_SIGNATURE_MAX_AGE, **kwargs) self._secure = bool(kwargs.get("https_only")) diff --git a/framework/hosting/simple_module_hosting/token_guard.py b/framework/hosting/simple_module_hosting/token_guard.py index 97b43f554..e4f05bd57 100644 --- a/framework/hosting/simple_module_hosting/token_guard.py +++ b/framework/hosting/simple_module_hosting/token_guard.py @@ -17,6 +17,10 @@ credentials, and a page's props are a second, unscoped read API. * :class:`TokenAllowed` on a (non-page) route admits tokens explicitly, for a route that is safe for any token without naming a permission. +* :class:`Authenticated` ("any signed-in user") is a gate for SM031 and the + OpenAPI security export, **not** a token opt-in: on its own a token is still + refused (the route has no permission a scope could name). Pair it with + :class:`TokenAllowed` when a token may call it too. * Everything else refuses a token with ``403``. On a route the public-route registry exempts, the token is not refused but **ignored**: the request runs as the anonymous visitor it could have been anyway. @@ -49,6 +53,9 @@ GATE_ALLOW = "allow" """An explicit opt-in: tokens may reach this route without a permission.""" +GATE_AUTHENTICATED = "authenticated" +"""Any signed-in user may call (:class:`Authenticated`). Does *not* admit tokens.""" + GATE_PAGE = "page" """A page (Inertia) route: tokens are refused even behind a permission.""" @@ -79,6 +86,27 @@ async def __call__(self) -> None: return None +class Authenticated: + """Dependency marker: any signed-in user may call this route (no permission). + + Answers ``401`` when there is no principal. It documents the route as + deliberately open to every signed-in user, which silences diagnostic + ``SM031`` (an ``/api`` route with no permission, allow or authenticated + gate). Tokens stay refused unless the route also carries + :class:`TokenAllowed`. + + Usage:: + + @router.get("/me", dependencies=[Depends(Authenticated())]) + """ + + __sm_token_gate__ = GATE_AUTHENTICATED + + async def __call__(self, request: HTTPConnection) -> None: + if getattr(request.state, "user", None) is None: + raise HTTPException(status_code=401, detail="Authentication required") + + async def page_route(request: HTTPConnection) -> None: """Dependency for a page route (added to every module view router). @@ -106,6 +134,13 @@ def _gate_kinds(dependant: Any, out: set[str], seen: set[int]) -> None: _gate_kinds(sub, out, seen) +def dependant_gate_kinds(dependant: Any) -> set[str]: + """Every gate kind marked anywhere in *dependant*'s tree.""" + kinds: set[str] = set() + _gate_kinds(dependant, kinds, set()) + return kinds + + def route_admits_tokens(route: Any) -> bool: """Whether *route* (the matched ``APIRoute``) may serve a token principal. @@ -204,10 +239,13 @@ async def _refuse_token(request: HTTPConnection) -> None: __all__ = [ "GATE_ALLOW", + "GATE_AUTHENTICATED", "GATE_PAGE", "GATE_PERMISSION", "TOKEN_GATE_ATTR", + "Authenticated", "TokenAllowed", + "dependant_gate_kinds", "effective_route", "is_token_principal", "mark_token_gate", diff --git a/framework/hosting/tests/test_csp_module_sources.py b/framework/hosting/tests/test_csp_module_sources.py index d805ba3aa..4e6c0b441 100644 --- a/framework/hosting/tests/test_csp_module_sources.py +++ b/framework/hosting/tests/test_csp_module_sources.py @@ -12,7 +12,7 @@ from simple_module_core import ModuleBase, ModuleMeta from simple_module_core.csp import CspSourceRegistry from simple_module_hosting._phase_helpers import build_csp -from simple_module_hosting._registrations import run_module_registrations +from simple_module_hosting._registrations import RequestRegistries, run_module_registrations from simple_module_hosting.settings import Settings @@ -40,6 +40,7 @@ def _filled_registry() -> CspSourceRegistry: design_pack_registry=MagicMock(), audit_link_registry=MagicMock(), csp_registry=reg, + request_registries=RequestRegistries(), ) return reg diff --git a/framework/hosting/tests/test_forwarded_host.py b/framework/hosting/tests/test_forwarded_host.py new file mode 100644 index 000000000..ad8155d3a --- /dev/null +++ b/framework/hosting/tests/test_forwarded_host.py @@ -0,0 +1,158 @@ +"""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_rightmost_value_from_the_trusted_proxy_is_used(self) -> None: + # An appending proxy keeps the client's value first; only the last one + # was written by the trusted peer. + body = await _get(_mw([_PUBLIC_HOST]), f"evil.example.net, {_PUBLIC_HOST}") + assert body["host"] == _PUBLIC_HOST + + async def test_client_supplied_allowlisted_value_before_a_bad_one_is_ignored(self) -> None: + body = await _get(_mw([_PUBLIC_HOST]), f"{_PUBLIC_HOST}, evil.example.net") + 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) + + +async def test_default_port_follows_forwarded_proto() -> None: + seen: dict = {} + + async def inner(scope, receive, send): + seen["server"] = scope["server"] + + mw = ForwardedHostMiddleware(inner, trusted_hosts=_PROXY_IP, allowlist=[_PUBLIC_HOST]) + + async def call(proto: str | None) -> tuple: + headers = [(b"x-forwarded-host", _PUBLIC_HOST.encode())] + if proto: + headers.append((b"x-forwarded-proto", proto.encode())) + scope = { + "type": "http", + "scheme": "http", + "client": (_PROXY_IP, 1), + "headers": headers, + "server": ("internal", 8000), + } + await mw(scope, None, None) + return seen["server"] + + assert await call("https") == (_PUBLIC_HOST, 443) + assert await call("http, https") == (_PUBLIC_HOST, 443) + assert await call("https, http") == (_PUBLIC_HOST, 80) + assert await call(None) == (_PUBLIC_HOST, 80) 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_hosting_openapi_security.py b/framework/hosting/tests/test_hosting_openapi_security.py new file mode 100644 index 000000000..30c24341d --- /dev/null +++ b/framework/hosting/tests/test_hosting_openapi_security.py @@ -0,0 +1,114 @@ +"""OpenAPI security export and SM031 (#534).""" + +from __future__ import annotations + +from fastapi import APIRouter, Depends, FastAPI +from simple_module_core.diagnostics import DiagnosticLevel +from simple_module_core.public_routes import PublicRouteRegistry +from simple_module_hosting._openapi import ( + BEARER_SCHEME, + SESSION_SCHEME, + install_openapi_filter, +) +from simple_module_hosting._route_access import UNGUARDED_ROUTE_CODE, check_unguarded_api_routes +from simple_module_hosting.permissions import RequiresPermission +from simple_module_hosting.session import SESSION_COOKIE_NAME +from simple_module_hosting.token_guard import ( + GATE_AUTHENTICATED, + Authenticated, + TokenAllowed, + dependant_gate_kinds, + route_admits_tokens, +) + +_BOTH = [{BEARER_SCHEME: []}, {SESSION_SCHEME: []}] + + +def _op(schema: dict, method: str, path: str) -> dict: + return schema["paths"][path][method] + + +def _mini_app() -> FastAPI: + app = FastAPI() + app.state.public_routes = PublicRouteRegistry() + app.state.public_routes.add_regex(r"/api/m/open/[^/]+$", methods={"GET"}) + router = APIRouter(prefix="/api/m") + router.add_api_route("/guarded", lambda: None, dependencies=[Depends(RequiresPermission("x"))]) + router.add_api_route("/bare", lambda: None) + router.add_api_route("/signed-in", lambda: None, dependencies=[Depends(Authenticated())]) + router.add_api_route("/open/{slug}", lambda slug: None) + router.add_api_route("/open/{slug}", lambda slug: None, methods=["DELETE"]) + app.include_router(router) + install_openapi_filter(app) + return app + + +def test_both_schemes_are_declared(app) -> None: + schemes = app.openapi()["components"]["securitySchemes"] + assert schemes[BEARER_SCHEME] == {"type": "http", "scheme": "bearer"} + assert schemes[SESSION_SCHEME] == { + "type": "apiKey", + "in": "cookie", + "name": SESSION_COOKIE_NAME, + } + + +def test_protected_operation_lists_both_schemes(app) -> None: + assert _op(app.openapi(), "get", "/api/audit_log/")["security"] == _BOTH + + +def test_file_storage_public_operations_are_anonymous(app) -> None: + schema = app.openapi() + for path in ( + "/api/file-storage/public/{file_id}", + "/api/file-storage/public/{file_id}/{filename}", + "/api/file-storage/public/{file_id}/thumbnail", + ): + assert _op(schema, "get", path)["security"] == [] + assert _op(schema, "get", "/api/file-storage/files")["security"] == _BOTH + + +def test_every_operation_carries_security(app) -> None: + for item in app.openapi()["paths"].values(): + for method, op in item.items(): + if isinstance(op, dict) and method != "parameters": + assert "security" in op, op["operationId"] + + +def test_public_match_is_method_aware_on_a_templated_path() -> None: + schema = _mini_app().openapi() + assert _op(schema, "get", "/api/m/open/{slug}")["security"] == [] + assert _op(schema, "delete", "/api/m/open/{slug}")["security"] == _BOTH + assert _op(schema, "get", "/api/m/bare")["security"] == _BOTH + + +def test_sm031_flags_unguarded_route_only() -> None: + found = check_unguarded_api_routes(_mini_app()) + assert {d.code for d in found} == {UNGUARDED_ROUTE_CODE} + assert all(d.level == DiagnosticLevel.WARNING for d in found) + messages = " ".join(d.message for d in found) + assert "GET /api/m/bare" in messages + assert "DELETE /api/m/open/{slug}" in messages + assert "/api/m/guarded" not in messages + assert "GET /api/m/open" not in messages + + +def test_authenticated_silences_sm031() -> None: + messages = " ".join(d.message for d in check_unguarded_api_routes(_mini_app())) + assert "/api/m/signed-in" not in messages + + +def test_authenticated_is_a_gate_but_does_not_admit_tokens() -> None: + router = APIRouter() + router.add_api_route("/a", lambda: None, dependencies=[Depends(Authenticated())]) + router.add_api_route( + "/b", lambda: None, dependencies=[Depends(Authenticated()), Depends(TokenAllowed())] + ) + routes = {r.path: r for r in router.routes} + assert GATE_AUTHENTICATED in dependant_gate_kinds(routes["/a"].dependant) + assert route_admits_tokens(routes["/a"]) is False + assert route_admits_tokens(routes["/b"]) is True + + +def test_real_app_has_no_unguarded_api_routes(app) -> None: + assert check_unguarded_api_routes(app) == [] diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index e66eb60e3..747f46b01 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -2,7 +2,8 @@ CLAUDE.md spells out the pipeline: - CorrelationId → RequestLogging → BodyLimit → GZip → Security → Session → + (ForwardedHost → ProxyHeaders, opt-in; pinned in test_forwarded_host.py) + CorrelationId → RequestLogging → CORS → BodyLimit → GZip → Security → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → RateLimit → Setup → Maintenance → CommitBeforeResponse → app @@ -80,6 +81,7 @@ _EXPECTED_MULTI_TENANT = ( "CorrelationIdMiddleware", "RequestLoggingMiddleware", + "CorsMiddleware", "BodyLimitMiddleware", "GZipMiddleware", "SecurityHeadersMiddleware", @@ -101,6 +103,7 @@ _EXPECTED_SINGLE_TENANT = ( "CorrelationIdMiddleware", "RequestLoggingMiddleware", + "CorsMiddleware", "BodyLimitMiddleware", "GZipMiddleware", "SecurityHeadersMiddleware", 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..628e69821 --- /dev/null +++ b/framework/hosting/tests/test_rate_limit_rules_registry.py @@ -0,0 +1,191 @@ +"""Per-route rate-limit rules and the ``RateLimit`` dependency (#529).""" + +from __future__ import annotations + +from types import SimpleNamespace + +import httpx +from fastapi import APIRouter, Depends, FastAPI +from simple_module_core.public_routes import PublicRouteRegistry +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_anonymous_tenant_traffic_is_counted_per_ip_not_per_tenant() -> None: + # One anonymous client must not be able to drain a tenant's shared budget. + reg = _registry(RateLimitRule("1/minute", key="tenant")) + async with _client(_app(reg)) as c: + h = {_TENANT_HEADER.decode(): "t1"} + assert (await c.post("/api/things", headers=h)).status_code == 200 + assert (await c.post("/api/things", headers=h)).status_code == 429 + member = {**h, _USER_HEADER.decode(): "a"} + assert (await c.post("/api/things", headers=member)).status_code == 200 + + +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 + + +async def test_public_rate_and_registry_rule_on_one_path_count_separately() -> None: + # Same kind/pattern/methods: each request must bump each counter once. + reg = RateLimitRegistry() + reg.add_exact("/api/things", RateLimitRule("2/minute", key="ip")) + app = _app(reg) + public = PublicRouteRegistry() + public.add_exact("/api/things", rate="2/minute") + app.state.public_routes = public + async with _client(app) as c: + assert await _burst(c, None, 3, "GET") == [200, 200, 429] + + +async def test_dependency_budget_is_per_include_prefix() -> None: + app = FastAPI() + app.state.sm = SimpleNamespace(rate_limit_store=InProcessWindowStore()) + router = APIRouter() + + @router.post("/dep", dependencies=[Depends(RateLimit("1/minute", key="ip"))]) + async def dep() -> dict[str, bool]: + return {"ok": True} + + app.include_router(router, prefix="/a") + app.include_router(router, prefix="/b") + async with _client(app) as c: + assert (await c.post("/a/dep")).status_code == 200 + assert (await c.post("/b/dep")).status_code == 200 + assert (await c.post("/a/dep")).status_code == 429 diff --git a/framework/hosting/tests/test_rate_limit_store_shared.py b/framework/hosting/tests/test_rate_limit_store_shared.py index 36c3848c1..e163b6739 100644 --- a/framework/hosting/tests/test_rate_limit_store_shared.py +++ b/framework/hosting/tests/test_rate_limit_store_shared.py @@ -4,7 +4,8 @@ from simple_module_core.rate_limit import InProcessWindowStore, RateSpec from simple_module_core.services import Services -from simple_module_hosting._rate_limit import RateLimitMiddleware, RedisWindowStore +from simple_module_hosting._rate_limit import RateLimitMiddleware +from simple_module_hosting._rate_limit_store import RedisWindowStore class _Clock: @@ -143,6 +144,6 @@ async def eval(self, script: str, numkeys: int, key: str, *args): def test_script_slides_on_flag() -> None: - from simple_module_hosting._rate_limit import _LUA + from simple_module_hosting._rate_limit_store import _LUA assert "ARGV[2] == '1'" in _LUA diff --git a/framework/hosting/tests/test_request_guard_rate_limit.py b/framework/hosting/tests/test_request_guard_rate_limit.py index 1b5b0b996..6be27cc36 100644 --- a/framework/hosting/tests/test_request_guard_rate_limit.py +++ b/framework/hosting/tests/test_request_guard_rate_limit.py @@ -11,7 +11,8 @@ RateSpec, parse_rate, ) -from simple_module_hosting._rate_limit import RateLimitMiddleware, RedisWindowStore +from simple_module_hosting._rate_limit import RateLimitMiddleware +from simple_module_hosting._rate_limit_store import RedisWindowStore from starlette.applications import Starlette from starlette.responses import JSONResponse from starlette.routing import Route @@ -266,7 +267,7 @@ async def eval(self, *a, **k): assert redis.calls == 3 def test_script_heals_a_counter_without_ttl(self) -> None: - from simple_module_hosting._rate_limit import _LUA + from simple_module_hosting._rate_limit_store import _LUA # A key that lost its TTL would block that IP forever; the script re-arms it. assert "t < 0" in _LUA and "PEXPIRE" in _LUA diff --git a/modules/background_tasks/background_tasks/db_settings.py b/modules/background_tasks/background_tasks/db_settings.py new file mode 100644 index 000000000..e9c9f661d --- /dev/null +++ b/modules/background_tasks/background_tasks/db_settings.py @@ -0,0 +1,34 @@ +"""Restart-required worker engine settings.""" + +from typing import Any + +from pydantic import Field +from pydantic_settings import BaseSettings + +_RESTART = {"requires_restart": True, "group": "Database"} + + +class WorkerDatabaseSettings(BaseSettings): + """Inherited by BackgroundTasksSettings with its SM_BG_TASKS_ env prefix.""" + + db_pool_size: int = Field(default=2, ge=1, json_schema_extra=_RESTART) + db_max_overflow: int = Field(default=3, ge=0, json_schema_extra=_RESTART) + db_recycle: int = Field(default=1800, ge=0, json_schema_extra=_RESTART) + db_statement_timeout_ms: int | None = Field(default=None, ge=0, json_schema_extra=_RESTART) + + +def worker_database_options(host: Any, worker: WorkerDatabaseSettings) -> dict[str, int]: + """Use host timeouts unless the worker has an explicit statement limit.""" + return { + "pool_size": worker.db_pool_size, + "max_overflow": worker.db_max_overflow, + "pool_recycle": worker.db_recycle, + "pool_timeout": host.db_pool_timeout, + "connect_timeout": host.db_connect_timeout, + "statement_timeout_ms": ( + host.db_statement_timeout_ms + if worker.db_statement_timeout_ms is None + else worker.db_statement_timeout_ms + ), + "idle_in_tx_timeout_ms": host.db_idle_in_tx_timeout_ms, + } diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index c29069aca..9760dc464 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -146,8 +146,9 @@ async def on_startup(self, app: FastAPI) -> None: import asyncio from background_tasks.celery_app import build_celery + from background_tasks.db_settings import worker_database_options from background_tasks.signals import bind_event_bus - from background_tasks.sync_db import set_database_url + from background_tasks.sync_db import configure_database, set_database_url from background_tasks.tenant_context import set_default_tenant services = app.state.background_tasks @@ -159,6 +160,7 @@ async def on_startup(self, app: FastAPI) -> None: multi = bool(getattr(host, "multi_tenant", False)) default_tenant = None if multi else getattr(host, "default_tenant", "") or None set_database_url(host.database_url, tenant_strict=multi, default_tenant=default_tenant) + configure_database(worker_database_options(host, services.settings)) set_default_tenant(default_tenant) # build_celery imports `signals` for side effects and runs # `autodiscover_tasks` across every installed module. diff --git a/modules/background_tasks/background_tasks/outbox_worker.py b/modules/background_tasks/background_tasks/outbox_worker.py index bd8c26a01..49e5a6a7b 100644 --- a/modules/background_tasks/background_tasks/outbox_worker.py +++ b/modules/background_tasks/background_tasks/outbox_worker.py @@ -6,7 +6,7 @@ event loop (:mod:`.outbox_loop`), and the engine — built once per loop from the URL :mod:`.sync_db` was pinned to — keeps a small connection pool on that loop. Under the ``threads`` pool every Celery thread therefore has its own loop and -engine, and the database sees up to three connections per worker thread. +engine (a pool of one, plus two overflow). Its session class gets the framework's entity listeners and the host's tenancy policy, like the sync engine's. @@ -56,16 +56,26 @@ def worker_session_factory() -> async_sessionmaker[Any]: """ url = sync_db.async_database_url() policy = sync_db.engine_tenancy() - key = (url, policy.tenant_strict, policy.default_tenant_id) + options = sync_db.database_options() + key = (url, policy.tenant_strict, policy.default_tenant_id, tuple(sorted(options.items()))) cache = worker_loop_cache() cached: tuple[Any, async_sessionmaker[Any], AsyncEngine] | None = cache.get(_ENGINE_KEY) if cached is not None and cached[0] == key: return cached[1] if cached is not None: _dispose_soon(cached[2]) - # Pooled, and small: one thread delivers one row at a time — the claim, - # the handler's session, and a handler's occasional second session. - state = init_db(url, pool_size=1, max_overflow=2) + # Small pool of its own: one thread delivers one row at a time. Timeouts and + # recycle are shared with the sync workers. + state = init_db( + url, + pool_size=1, + max_overflow=2, + pool_recycle=options.get("pool_recycle", 1800), + pool_timeout=options.get("pool_timeout", 30), + connect_timeout=options.get("connect_timeout", 10), + statement_timeout_ms=options.get("statement_timeout_ms", 0), + idle_in_tx_timeout_ms=options.get("idle_in_tx_timeout_ms", 0), + ) attach_session_listeners(state.sync_session_class) bind_engine_policy(state.engine.sync_engine, policy) cache[_ENGINE_KEY] = (key, state.session_factory, state.engine) diff --git a/modules/background_tasks/background_tasks/runner.py b/modules/background_tasks/background_tasks/runner.py index 41c145418..8fac35c00 100644 --- a/modules/background_tasks/background_tasks/runner.py +++ b/modules/background_tasks/background_tasks/runner.py @@ -53,7 +53,9 @@ def build_runner_celery(fallback_env_file: Path | None = None) -> Celery: from simple_module_hosting import merge_host_settings from background_tasks.celery_app import build_celery - from background_tasks.sync_db import set_database_url + from background_tasks.db_settings import worker_database_options + from background_tasks.settings import BackgroundTasksSettings + from background_tasks.sync_db import configure_database, set_database_url from background_tasks.tenant_context import set_default_tenant from background_tasks.worker_settings import load_runner_settings @@ -65,7 +67,10 @@ def build_runner_celery(fallback_env_file: Path | None = None) -> Celery: set_database_url( host.database_url, tenant_strict=host.multi_tenant, default_tenant=default_tenant ) + configure_database(worker_database_options(host, BackgroundTasksSettings())) set_default_tenant(default_tenant) # Hydrated from the DB like the web process, so an admin-edited setting # reaches the worker at boot, not only the web app. - return build_celery(load_runner_settings()) + settings = load_runner_settings() + configure_database(worker_database_options(host, settings)) + return build_celery(settings) diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index 94cba1027..178a1ad16 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -42,7 +42,7 @@ from pydantic import Field, ValidationInfo, field_validator, model_validator from pydantic_core import PydanticCustomError -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict from simple_module_core.settings_hydrate import RELATED_FIELDS_CTX from background_tasks.config_guards import parse_task_routes @@ -63,6 +63,7 @@ ENV_REDIS_URL, max_outbox_handler_timeout, ) +from background_tasks.db_settings import WorkerDatabaseSettings from background_tasks.settings_env_checks import ( forbid_localhost_broker_in_production, warn_on_legacy_redis_vars, @@ -79,12 +80,11 @@ def _redis_from_env(default: str) -> str: ``env_prefix`` means pydantic's env source has already answered ``SM_BG_TASKS_BROKER_URL`` / ``SM_BG_TASKS_RESULT_BACKEND`` by the time a default is needed, so this factory is only the ``SM_REDIS_URL`` half — the - legacy names keep working, and keep winning, through that source instead. - """ + legacy names keep working, and keep winning, through that source instead.""" return os.environ.get(ENV_REDIS_URL) or default -class BackgroundTasksSettings(BaseSettings): +class BackgroundTasksSettings(WorkerDatabaseSettings): """Configuration for the Celery + Redis task runner.""" # Without ``env_prefix`` pydantic-settings resolves each field from its diff --git a/modules/background_tasks/background_tasks/sync_db.py b/modules/background_tasks/background_tasks/sync_db.py index 05865445e..b620c6efb 100644 --- a/modules/background_tasks/background_tasks/sync_db.py +++ b/modules/background_tasks/background_tasks/sync_db.py @@ -21,7 +21,9 @@ from celery.signals import worker_process_init from simple_module_db import DEFAULT_TENANT_ID from simple_module_db.audit_sinks import discover_audit_callback +from simple_module_db.engine_options import connect_args from simple_module_db.listeners import attach_session_listeners +from simple_module_db.provider import DatabaseProvider, detect_provider from simple_module_db.query_filter import EngineTenancy, bind_engine_policy from sqlalchemy import create_engine from sqlalchemy.engine import Engine @@ -36,6 +38,7 @@ _engine_pid: int | None = None _session_factory: sessionmaker[Session] | None = None _url_override: str | None = None +_database_options: dict[str, int] = {} _tenant_strict: bool = False _default_tenant: str | None = None @@ -136,10 +139,34 @@ def engine_tenancy() -> EngineTenancy: ) +def configure_database(options: dict[str, int]) -> None: + """Pin resolved settings before building either worker engine.""" + global _database_options + if options != _database_options: + _database_options = dict(options) + _drop_engine() + + +def database_options() -> dict[str, int]: + """A copy for the outbox engine and its cache key.""" + return dict(_database_options) + + def _build_engine() -> Engine: sync_url = _sync_url(_resolve_url()) # Small pool — signals fire sequentially per worker process. - engine = create_engine(sync_url, pool_pre_ping=True, pool_size=2, max_overflow=3) + options = database_options() + args = connect_args( + sync_url, + connect_timeout=options.pop("connect_timeout", 10), + statement_timeout_ms=options.pop("statement_timeout_ms", 0), + idle_in_tx_timeout_ms=options.pop("idle_in_tx_timeout_ms", 0), + ) + if detect_provider(sync_url) == DatabaseProvider.SQLITE: + options = {} + else: + options = {"pool_size": 2, "max_overflow": 3, "pool_recycle": 1800} | options + engine = create_engine(sync_url, pool_pre_ping=True, connect_args=args, **options) # A worker never runs create_app, so without this its sessions would have # no tenant filter at all: the tenant restored around a task body would # scope nothing (#371). @@ -170,6 +197,7 @@ def dispose_sync_engine() -> None: global _url_override, _tenant_strict, _default_tenant _drop_engine() _url_override = None + configure_database({}) _tenant_strict = False _default_tenant = None diff --git a/modules/background_tasks/tests/test_bg_db_timeouts.py b/modules/background_tasks/tests/test_bg_db_timeouts.py new file mode 100644 index 000000000..ad5bf5919 --- /dev/null +++ b/modules/background_tasks/tests/test_bg_db_timeouts.py @@ -0,0 +1,95 @@ +"""Worker pool and startup timeouts (#531).""" + +from types import SimpleNamespace + +import pytest +from background_tasks import outbox_worker, sync_db +from background_tasks.db_settings import worker_database_options +from background_tasks.settings import BackgroundTasksSettings +from simple_module_test.database import database_url_for_tests +from sqlalchemy import event, text +from sqlalchemy.exc import DBAPIError + + +def options(**changes): + host = SimpleNamespace( + db_pool_timeout=17, + db_connect_timeout=4, + db_statement_timeout_ms=1000, + db_idle_in_tx_timeout_ms=2000, + ) + return worker_database_options(host, BackgroundTasksSettings(**changes)) + + +@pytest.fixture(autouse=True) +def cleanup(): + yield + outbox_worker.reset_worker_state() + sync_db.dispose_sync_engine() + + +def test_worker_env(monkeypatch): + monkeypatch.setenv("SM_BG_TASKS_DB_POOL_SIZE", "4") + monkeypatch.setenv("SM_BG_TASKS_DB_MAX_OVERFLOW", "5") + monkeypatch.setenv("SM_BG_TASKS_DB_RECYCLE", "90") + monkeypatch.setenv("SM_BG_TASKS_DB_STATEMENT_TIMEOUT_MS", "0") + result = options() + assert result == { + "pool_size": 4, + "max_overflow": 5, + "pool_recycle": 90, + "pool_timeout": 17, + "connect_timeout": 4, + "statement_timeout_ms": 0, + "idle_in_tx_timeout_ms": 2000, + } + + +def test_sync_options(): + sync_db.set_database_url("postgresql+asyncpg://localhost/db") + sync_db.configure_database(options()) + engine = sync_db.get_sync_session_factory().kw["bind"] + assert engine.pool.timeout() == 17 + assert engine.pool.size() == 2 + captured = {} + + @event.listens_for(engine, "do_connect") + def capture(dialect, record, args, kwargs): + captured.update(kwargs) + raise RuntimeError("captured") + + with pytest.raises(RuntimeError, match="captured"): + engine.connect() + assert captured["connect_timeout"] == 4 + assert captured["options"] == ( + "-c statement_timeout=1000 -c idle_in_transaction_session_timeout=2000" + ) + + +async def test_outbox_options(monkeypatch): + sync_db.set_database_url("sqlite+aiosqlite://") + sync_db.configure_database(options()) + captured = {} + cache = {} + monkeypatch.setattr(outbox_worker, "worker_loop_cache", lambda: cache) + original = outbox_worker.init_db + + def capture(url, **kwargs): + captured.update(kwargs) + return original(url, **kwargs) + + monkeypatch.setattr(outbox_worker, "init_db", capture) + outbox_worker.worker_session_factory() + assert captured == options() | {"pool_size": 1, "max_overflow": 2} + await outbox_worker._dispose_cached(cache) + + +@pytest.mark.postgres +def test_worker_statement_timeout(): + sync_db.set_database_url(database_url_for_tests()) + sync_db.configure_database(options()) + with ( + sync_db.get_sync_session_factory()() as session, + pytest.raises(DBAPIError, match="statement timeout"), + ): + session.execute(text("SELECT pg_sleep(2)")) diff --git a/modules/background_tasks/tests/test_bg_outbox_thread_loops.py b/modules/background_tasks/tests/test_bg_outbox_thread_loops.py index 9ff2a4d18..f9cc65f4b 100644 --- a/modules/background_tasks/tests/test_bg_outbox_thread_loops.py +++ b/modules/background_tasks/tests/test_bg_outbox_thread_loops.py @@ -73,3 +73,21 @@ def test_each_thread_gets_its_own_engine(sync_sqlite): assert sync_db.async_database_url() finally: outbox_worker.reset_worker_state() + + +def test_outbox_engine_keeps_its_own_small_pool(sync_sqlite, monkeypatch): + seen: dict = {} + real = outbox_worker.init_db + + def spy(url, **kw): + seen.update(kw) + return real(url, **kw) + + monkeypatch.setattr(outbox_worker, "init_db", spy) + monkeypatch.setattr(sync_db, "database_options", lambda: {"pool_size": 9, "max_overflow": 9}) + outbox_worker.reset_worker_state() + try: + outbox_worker.worker_session_factory() + finally: + outbox_worker.reset_worker_state() + assert (seen["pool_size"], seen["max_overflow"]) == (1, 2) 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 +#