diff --git a/CLAUDE.md b/CLAUDE.md index ab872d90..7304b48a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,7 +77,7 @@ hence `SM022`/`SM023`. See `docs/module-authoring.md` § Styling. `register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_health_checks` / `register_public_routes` / `register_csp_sources` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_csp_sources(registry)` lets a module whitelist external asset origins (`registry.add("style-src", "https://rsms.me")`) — fetch directives only, validated at boot. `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). **Middleware pipeline** (Starlette `add_middleware` is LIFO — last added runs first). Execution order on a request: -`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → CommitBeforeResponse → app`. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs and Inertia's absolute page url). When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. +`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → CommitBeforeResponse → app`. `InertiaCache` answers for `InertiaLayoutData` merging per-user `auth`/`menus` into every payload: a response to an `X-Inertia` request is forced to `private, no-store` with its ETag dropped, and both representations of a URL gain `Vary: X-Inertia` — so no cache can store the JSON payload or hand it back for a page request. A module wanting its public page content cached should set `Cache-Control` and an ETag on the *document*; that path is left alone. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs and Inertia's absolute page url). When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. **Database**: per-module `Base` via `create_module_base("")`. Every module owns its own `MetaData` (so Alembic autogenerate can attribute tables to a module), but all tables live in the host's single schema. `__tablename__` must be prefixed with the module name to avoid collisions (`orders_order`). Postgres and SQLite share the same layout. diff --git a/framework/hosting/simple_module_hosting/_inertia_cache.py b/framework/hosting/simple_module_hosting/_inertia_cache.py new file mode 100644 index 00000000..e2e73e78 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_inertia_cache.py @@ -0,0 +1,104 @@ +"""Keep the Inertia payload out of caches that answer page requests. + +Every Inertia route serves one URL as two representations, chosen on the +request's ``X-Inertia`` header: an HTML document for a full page load, a JSON +payload for a client-side visit. Nothing in either response says so, which +leaves a cache free to store one and hand it back for the other. Visit a page +through the SPA, then open the same URL directly, and the browser can serve the +stored payload as the document — the visitor gets +``{"component":"...","props":{...}}`` where the page should be. + +That only bites once a route marks itself cacheable, which a public-content +module reasonably does. The framework is what makes it unsafe: +:class:`~simple_module_hosting.middleware.InertiaLayoutDataMiddleware` merges +the signed-in user's ``auth`` block, their permission list and the menus their +roles resolve to into *every* Inertia payload. A route author choosing +``Cache-Control: public`` for their page content has no way to know that, so +the guarantee belongs here rather than in each module: + +* **An Inertia payload is never stored.** ``private, no-store``, and the ETag + is dropped so no cache can revalidate its way back to a copy it should not + have kept. The cost is per-visit caching on client-side navigation, which was + never safe to take — those bytes are specific to one user. +* **Both representations declare ``Vary: X-Inertia``**, so a cache that honours + Vary keeps them in separate entries instead of inferring it from ``Accept``. + +``Vary`` is added to the document only when the response is HTML, so static +assets and JSON APIs keep the validators and cache entries they had. + +A module that wants its public page content cached should give the *document* +its own ``Cache-Control`` and an ETag that identifies the representation; this +middleware leaves that path alone and only governs the payload. +""" + +from __future__ import annotations + +from starlette.datastructures import MutableHeaders +from starlette.types import ASGIApp, Message, Receive, Scope, Send + +_SCOPE_HTTP = "http" +_HEADER_INERTIA = b"x-inertia" + +#: What an Inertia payload is allowed to say about its own cacheability. +PAYLOAD_CACHE_CONTROL = "private, no-store" + +#: The request header that selects the representation, and therefore the one +#: caches must key on. +VARY_FIELD = "X-Inertia" + + +def add_vary(headers: MutableHeaders, field: str = VARY_FIELD) -> None: + """Add a field to ``Vary``, keeping whatever is already listed.""" + existing = headers.get("vary") + if not existing: + headers["vary"] = field + return + if any(part.strip().lower() == field.lower() for part in existing.split(",")): + return + headers["vary"] = f"{existing}, {field}" + + +def is_inertia_request(scope: Scope) -> bool: + """Whether this request asked for the JSON representation. + + Mirrors ``fastapi-inertia``'s own ``Inertia._is_inertia_request`` exactly — + presence of the header, any value — rather than requiring it to equal + ``"true"``. The library renders JSON for *any* ``X-Inertia`` value, so a + stricter check here would disagree with it: a request the renderer treats + as Inertia would sail through uncached, undoing the whole fix. + """ + return any(key == _HEADER_INERTIA for key, _ in scope.get("headers", ())) + + +def _is_html(headers: MutableHeaders) -> bool: + return headers.get("content-type", "").split(";", 1)[0].strip() == "text/html" + + +class InertiaCacheMiddleware: + """Stop an Inertia payload being cached as though it were the page.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != _SCOPE_HTTP: + await self.app(scope, receive, send) + return + + inertia = is_inertia_request(scope) + + async def send_with_cache_rules(message: Message) -> None: + if message["type"] == "http.response.start": + headers = MutableHeaders(scope=message) + if inertia: + headers["cache-control"] = PAYLOAD_CACHE_CONTROL + del headers["etag"] + add_vary(headers) + elif _is_html(headers): + add_vary(headers) + await send(message) + + await self.app(scope, receive, send_with_cache_rules) + + +__all__ = ["PAYLOAD_CACHE_CONTROL", "VARY_FIELD", "InertiaCacheMiddleware", "add_vary"] diff --git a/framework/hosting/simple_module_hosting/_inertia_json.py b/framework/hosting/simple_module_hosting/_inertia_json.py new file mode 100644 index 00000000..62a18b0e --- /dev/null +++ b/framework/hosting/simple_module_hosting/_inertia_json.py @@ -0,0 +1,86 @@ +"""Make Inertia's JSON branch encode props the way its HTML branch does. + +``fastapi-inertia`` renders the same props two ways and configures only one of +them:: + + # full page load — honours InertiaConfig.json_encoder + json_string = json.dumps(page_data, cls=self._config.json_encoder) + + # client-side visit — Starlette's JSONResponse, so plain json.dumps + return JSONResponse(content=await self._get_page_data(), ...) + +``InertiaJsonEncoder`` exists precisely to run the payload through FastAPI's +``jsonable_encoder``, and the second branch never reaches it. The effect is a +route that works on a reload and 500s on every client-side visit, for any prop +the stdlib encoder cannot handle — ``Path``, ``Decimal``, ``UUID``, a dataclass, +an enum. + +Settings → Modules is where this surfaced: it reflects each installed module's +pydantic settings back to the admin, and a module whose settings carry a +``Path`` field (``PagebuilderSettings.media_root``) puts a ``PosixPath`` in the +payload:: + + TypeError: Object of type PosixPath is not JSON serializable + +Fixing it here rather than in each module is deliberate. A module putting a +rich value in props is not doing anything wrong — the HTML branch has always +accepted it — so the asymmetry is the defect, and closing it at the point of +serialisation covers every module and every prop type at once. + +Applied by wrapping the dependency published on ``app.state.inertia_dependency``, +which ``inertia_deps.get_inertia`` calls once per request. The replacement binds +to the instance, so the library's class is untouched and any other construction +path keeps the stock behaviour. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from fastapi.encoders import jsonable_encoder +from starlette.responses import JSONResponse + +logger = logging.getLogger(__name__) + +#: The headers upstream's ``_render_json`` sets, reproduced so the wrap is a +#: pure encoder change. ``InertiaCacheMiddleware`` owns the caching rules and +#: extends ``Vary`` on the way out. +_JSON_HEADERS = {"X-Inertia": "true", "Vary": "Accept"} + + +def json_safe_inertia_dependency(inertia_dep: Any) -> Any: + """Wrap an Inertia dependency so its JSON branch uses ``jsonable_encoder``. + + The wrapped callable keeps the ``(request, client)`` shape + ``inertia_dependency_factory`` returns. An instance that doesn't expose the + private hook this relies on is handed back untouched: the failure mode is + the 500 that already happens, and refusing to boot because an upstream + attribute moved would be a worse trade. + """ + + def dependency(request: Any, client: Any = None) -> Any: + inertia = inertia_dep(request, client) + if hasattr(inertia, "_get_page_data"): + inertia._render_json = _render_json_for(inertia) + else: # pragma: no cover - upstream layout changed + logger.warning( + "Inertia instance has no _get_page_data; JSON responses keep the " + "stock encoder and non-JSON-native props will fail to serialise" + ) + return inertia + + return dependency + + +def _render_json_for(inertia: Any) -> Any: + """Build the instance's replacement ``_render_json``.""" + + async def _render_json() -> JSONResponse: + page_data = await inertia._get_page_data() + return JSONResponse(content=jsonable_encoder(page_data), headers=_JSON_HEADERS) + + return _render_json + + +__all__ = ["json_safe_inertia_dependency"] diff --git a/framework/hosting/simple_module_hosting/_inertia_setup.py b/framework/hosting/simple_module_hosting/_inertia_setup.py index 8c23bc30..75746d7a 100644 --- a/framework/hosting/simple_module_hosting/_inertia_setup.py +++ b/framework/hosting/simple_module_hosting/_inertia_setup.py @@ -12,6 +12,7 @@ from inertia import InertiaConfig, inertia_dependency_factory from starlette.requests import Request +from simple_module_hosting._inertia_json import json_safe_inertia_dependency from simple_module_hosting.settings import Settings logger = logging.getLogger(__name__) @@ -184,6 +185,9 @@ def setup_inertia( use_flash_errors=True, ) - inertia_dep = inertia_dependency_factory(inertia_config) + # Upstream's JSON branch builds a Starlette JSONResponse directly, so the + # encoder configured above only ever applies to full page loads. Wrap the + # dependency so a client-side visit encodes the same props the same way. + inertia_dep = json_safe_inertia_dependency(inertia_dependency_factory(inertia_config)) app.state.inertia_dependency = inertia_dep return inertia_config diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 204377fc..4f53c15a 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -34,6 +34,7 @@ unhandled_exception_handler, ) from simple_module_hosting._host_services import _HostServices +from simple_module_hosting._inertia_cache import InertiaCacheMiddleware from simple_module_hosting.host_settings import HostSettings from simple_module_hosting.i18n_middleware import LocaleMiddleware from simple_module_hosting.middleware import ( @@ -103,12 +104,17 @@ def install_middleware( Order matters: last added = first executed. Execution order: (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging → Security → Session → [module] → (Tenant, if multi_tenant) → Locale - → Inertia → CommitBeforeResponse. + → Inertia → InertiaCache → CommitBeforeResponse. """ # Added first, so it is innermost and its send-wrapper is the first to see # the response: the request's DB work commits before any byte reaches the # client, instead of in get_db's post-response exit code (GH #257). app.add_middleware(CommitBeforeResponseMiddleware) + # Paired with InertiaLayoutDataMiddleware below, which is what puts this + # user's auth, permissions and menus into every Inertia payload: this one + # makes sure the payload that results is never stored where a page request + # can be answered with it. + app.add_middleware(InertiaCacheMiddleware) app.add_middleware( InertiaLayoutDataMiddleware, menu_registry=menu_registry, diff --git a/framework/hosting/tests/test_inertia_cache.py b/framework/hosting/tests/test_inertia_cache.py new file mode 100644 index 00000000..655d87bc --- /dev/null +++ b/framework/hosting/tests/test_inertia_cache.py @@ -0,0 +1,187 @@ +"""An Inertia payload must never be cacheable as the page it belongs to. + +The same URL answers twice — HTML for a full page load, JSON for a client-side +visit — and the only thing separating them is the ``X-Inertia`` request header. +Left unsaid, a cache stores one and serves it for the other, so opening a page +directly after visiting it through the SPA renders the raw payload. The payload +also carries this user's ``auth`` block, permissions and menus, so a shared +cache storing it is a disclosure bug as well as a broken page. +""" + +from __future__ import annotations + +import httpx +import pytest +from simple_module_hosting._inertia_cache import ( + PAYLOAD_CACHE_CONTROL, + InertiaCacheMiddleware, + is_inertia_request, +) +from starlette.applications import Starlette +from starlette.responses import HTMLResponse, JSONResponse, Response +from starlette.routing import Route + +_OK = 200 +_INERTIA = {"X-Inertia": "true"} +#: What a public-content module might reasonably put on its own page. +_PUBLIC_CACHE = "public, max-age=60, stale-while-revalidate=600" +_SHARED_ETAG = 'W/"deadbeef"' + + +def _vary_fields(response: httpx.Response) -> set[str]: + return {part.strip().lower() for part in response.headers.get("vary", "").split(",")} + + +def _build_app() -> Starlette: + """A route that markets itself as publicly cacheable, both ways. + + Mirrors what the page-builder's public viewer does: one ETag and one + ``Cache-Control`` computed from the page row, stamped on whichever + representation the request asked for. + """ + + async def page(request): + if is_inertia_request(request.scope): + response: Response = JSONResponse( + {"component": "PublicPage", "props": {"auth": {"user": "editor"}}}, + headers={"X-Inertia": "true", "Vary": "Accept"}, + ) + else: + response = HTMLResponse("Page") + response.headers["Cache-Control"] = _PUBLIC_CACHE + response.headers["ETag"] = _SHARED_ETAG + return response + + async def asset(request): + return Response(b"body{}", media_type="text/css", headers={"ETag": _SHARED_ETAG}) + + app = Starlette(routes=[Route("/p/home", page), Route("/static/app.css", asset)]) + app.add_middleware(InertiaCacheMiddleware) + return app + + +@pytest.fixture +def cache_client() -> httpx.AsyncClient: + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=_build_app()), base_url="http://test" + ) + + +class TestInertiaPayload: + async def test_payload_is_not_stored_even_when_the_route_says_public( + self, cache_client: httpx.AsyncClient + ) -> None: + """The route asked for `public`; the payload is per-user, so it loses.""" + async with cache_client as client: + resp = await client.get("/p/home", headers=_INERTIA) + + assert resp.status_code == _OK + assert resp.headers["cache-control"] == PAYLOAD_CACHE_CONTROL + + async def test_payload_carries_no_validator(self, cache_client: httpx.AsyncClient) -> None: + """A shared ETag lets a cache revalidate its way back to the payload. + + The route stamps the same ETag on both representations, so a client + holding the JSON could revalidate a *document* request into a 304 and + keep rendering it. Nothing to revalidate means nothing to restore. + """ + async with cache_client as client: + resp = await client.get("/p/home", headers=_INERTIA) + + assert "etag" not in resp.headers + + async def test_payload_varies_on_the_header_that_selected_it( + self, cache_client: httpx.AsyncClient + ) -> None: + async with cache_client as client: + resp = await client.get("/p/home", headers=_INERTIA) + + assert "x-inertia" in _vary_fields(resp) + # Whatever the route already listed stays listed. + assert "accept" in _vary_fields(resp) + + +class TestMatchesUpstreamDetection: + """``is_inertia_request`` must agree with ``fastapi-inertia``'s own check. + + Upstream's ``Inertia._is_inertia_request`` is ``"X-Inertia" in + self._request.headers`` — presence only, any value. A stricter check here + (e.g. requiring the value to equal ``"true"``) would disagree with it: a + request the renderer treats as Inertia and answers with JSON would sail + through this middleware uncached, reopening the exact leak the fix closes. + """ + + def test_any_header_value_counts_as_inertia(self) -> None: + scope = {"type": "http", "headers": [(b"x-inertia", b"false")]} + + assert is_inertia_request(scope) is True + + async def test_a_non_true_value_still_gets_the_safety_headers(self) -> None: + """A route that mirrors upstream's own (presence-only) detection. + + Unlike ``cache_client``'s app, this handler does not call + ``is_inertia_request`` itself — it reproduces upstream's check + directly, so the two are genuinely independent here. + """ + + async def page(request): + if "x-inertia" in request.headers: + response = JSONResponse({"component": "Home", "props": {}}) + else: + response = HTMLResponse("Page") + response.headers["Cache-Control"] = _PUBLIC_CACHE + response.headers["ETag"] = _SHARED_ETAG + return response + + app = Starlette(routes=[Route("/p/home", page)]) + app.add_middleware(InertiaCacheMiddleware) + transport = httpx.ASGITransport(app=app) + + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + resp = await client.get("/p/home", headers={"X-Inertia": "false"}) + + assert resp.headers["content-type"].startswith("application/json") + assert resp.headers["cache-control"] == PAYLOAD_CACHE_CONTROL + assert "etag" not in resp.headers + + +class TestDocumentResponse: + async def test_document_varies_on_x_inertia(self, cache_client: httpx.AsyncClient) -> None: + """Otherwise a cached document can be served to a client-side visit.""" + async with cache_client as client: + resp = await client.get("/p/home") + + assert "x-inertia" in _vary_fields(resp) + + async def test_document_keeps_the_caching_the_route_chose( + self, cache_client: httpx.AsyncClient + ) -> None: + """Public page content stays cacheable — that half was never the bug.""" + async with cache_client as client: + resp = await client.get("/p/home") + + assert resp.headers["cache-control"] == _PUBLIC_CACHE + assert resp.headers["etag"] == _SHARED_ETAG + + +class TestUnrelatedResponses: + async def test_static_assets_are_untouched(self, cache_client: httpx.AsyncClient) -> None: + """Adding Vary or dropping ETags here would cost every asset its 304.""" + async with cache_client as client: + resp = await client.get("/static/app.css") + + assert resp.headers["etag"] == _SHARED_ETAG + assert "x-inertia" not in _vary_fields(resp) + + +class TestAgainstTheRealApp: + async def test_a_rendered_view_never_returns_a_storable_payload( + self, authenticated_client: httpx.AsyncClient + ) -> None: + """End to end, through the whole middleware pipeline.""" + resp = await authenticated_client.get("/dashboard/", headers=_INERTIA) + + assert resp.status_code == _OK + assert resp.headers["content-type"].startswith("application/json") + assert resp.headers["cache-control"] == PAYLOAD_CACHE_CONTROL + assert "x-inertia" in _vary_fields(resp) diff --git a/framework/hosting/tests/test_inertia_json_encoder.py b/framework/hosting/tests/test_inertia_json_encoder.py new file mode 100644 index 00000000..34c26b2b --- /dev/null +++ b/framework/hosting/tests/test_inertia_json_encoder.py @@ -0,0 +1,83 @@ +"""Both Inertia render paths must encode props the same way. + +Upstream configures ``InertiaConfig.json_encoder`` — which exists to run props +through FastAPI's ``jsonable_encoder`` — and then only applies it on the full +page load. A client-side visit builds a Starlette ``JSONResponse``, reaching +plain ``json.dumps``. Any prop the stdlib cannot encode therefore renders on a +reload and 500s when reached by clicking a link, which is a miserable bug to +read from a stack trace: the page "works", right up until it is navigated to. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +from simple_module_hosting._inertia_json import json_safe_inertia_dependency +from starlette.responses import JSONResponse + +_OK = 200 + + +class _FakeInertia: + """Stands in for the library's Inertia, with the two hooks the wrap uses.""" + + def __init__(self, page_data: dict) -> None: + self._page_data = page_data + self.rendered_by_stock_encoder = False + + async def _get_page_data(self) -> dict: + return self._page_data + + async def _render_json(self) -> JSONResponse: + # What upstream does: no encoder, so a rich value raises here. + self.rendered_by_stock_encoder = True + return JSONResponse(content=self._page_data) + + +def _dependency_for(page_data: dict): + inertia = _FakeInertia(page_data) + dependency = json_safe_inertia_dependency(lambda request, client=None: inertia) + return dependency(object(), None) + + +class TestJsonSafeRendering: + async def test_a_path_prop_would_break_the_stock_encoder(self) -> None: + """Establishes the bug this wrap exists for, so the fix can't drift.""" + with pytest.raises(TypeError, match="PosixPath"): + json.dumps({"props": {"media_root": Path("var/media")}}) + + async def test_a_path_prop_renders(self) -> None: + inertia = _dependency_for({"props": {"media_root": Path("var/media")}}) + + response = await inertia._render_json() + + assert response.status_code == _OK + assert json.loads(bytes(response.body))["props"]["media_root"] == "var/media" + + async def test_the_wrap_replaces_the_stock_path(self) -> None: + """Not merely catching the error afterwards — the encoder is swapped.""" + inertia = _dependency_for({"props": {}}) + + await inertia._render_json() + + assert inertia.rendered_by_stock_encoder is False + + async def test_inertia_headers_are_preserved(self) -> None: + inertia = _dependency_for({"props": {}}) + + response = await inertia._render_json() + + assert response.headers["x-inertia"] == "true" + + async def test_an_unfamiliar_instance_is_left_alone(self) -> None: + """An upstream rename must not take the boot down with it.""" + + class _Unfamiliar: + pass + + original = _Unfamiliar() + wrapped = json_safe_inertia_dependency(lambda request, client=None: original) + + assert wrapped(object(), None) is original diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index 32a5dff6..0410ec7f 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -4,7 +4,13 @@ CorrelationId → RequestLogging → GZip → Security → Session → → Tenant (opt-in) → Locale → InertiaLayoutData - → CommitBeforeResponse → app + → InertiaCache → CommitBeforeResponse → app + +InertiaCache sits directly inside InertiaLayoutData, the middleware that puts +this user's auth block, permissions and menus into every Inertia payload. Being +inside it means its send-wrapper sees the response before anything else can +read the headers, and pairing the two keeps "what makes the payload per-user" +and "what stops the payload being cached" from drifting apart. Tenant/Locale must see ``request.state.user`` set by AuthMiddleware so DB queries get filtered correctly; CorrelationId must wrap everything so @@ -45,6 +51,7 @@ "TenantMiddleware", "LocaleMiddleware", "InertiaLayoutDataMiddleware", + "InertiaCacheMiddleware", "CommitBeforeResponseMiddleware", ) @@ -58,6 +65,7 @@ "AuthMiddleware", "LocaleMiddleware", "InertiaLayoutDataMiddleware", + "InertiaCacheMiddleware", "CommitBeforeResponseMiddleware", ) diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index 80f33427..01e936be 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -12,6 +12,7 @@ from typing import Any from fastapi import FastAPI +from fastapi.encoders import jsonable_encoder from pydantic_settings import BaseSettings from settings.env_vars import env_prefix_for @@ -251,7 +252,15 @@ async def overrides_by_package(service: SettingService) -> dict[str, frozenset[s def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: - """Convert dataclass views to plain dicts for Inertia props.""" + """Convert dataclass views to plain dicts for Inertia props. + + Field values arrive as whatever type the module declared — pydantic has + already coerced ``media_root: Path`` to a ``PosixPath``, ``timeout: + timedelta`` to a ``timedelta`` — and this screen reflects every installed + module's settings, so the set of types is open-ended by design. They are + encoded here rather than handed on as-is: this is the boundary where a + settings object stops being Python and becomes a prop. + """ return [ { "module_name": v.module_name, @@ -262,8 +271,8 @@ def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: { "name": f.name, "env_var": f.env_var, - "value": f.value, - "default": f.default, + "value": jsonable_encoder(f.value), + "default": jsonable_encoder(f.default), "description": f.description, "is_secret": f.is_secret, "type": f.type, diff --git a/modules/settings/tests/test_module_settings_render.py b/modules/settings/tests/test_module_settings_render.py new file mode 100644 index 00000000..260bfa48 --- /dev/null +++ b/modules/settings/tests/test_module_settings_render.py @@ -0,0 +1,90 @@ +"""Settings → Modules must render for a module with a rich settings type. + +This screen reflects *every* installed module's pydantic settings, so the value +types are open-ended — but none of the modules in this repo declare anything +the stdlib JSON encoder can't handle, which is why the failure only ever showed +up downstream. A wheel-installed module with ``media_root: Path`` was enough to +500 the page, and only on a client-side visit: the HTML render path has its own +encoder, so reloading the same URL worked and the report read as "sometimes". + +The demo module below exists to keep a non-JSON-native settings type in the +suite permanently. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +import httpx +import pytest +from fastapi import FastAPI +from pydantic_settings import BaseSettings +from simple_module_core.module import ModuleMeta + +_OK = 200 +_SERVER_ERROR = 500 +_INERTIA = {"X-Inertia": "true", "X-Inertia-Version": "1.0"} + + +class _PathCfg(BaseSettings): + """Mirrors the shape that broke it: a Path with a relative default.""" + + media_root: Path = Path("var/demo/media") + workers: int = 2 + + +class _PathModule: + meta = ModuleMeta(name="PathDemo") + + +_PathModule.__module__ = "pathdemo" + + +@dataclass +class _PathServices: + settings: _PathCfg + + +@pytest.fixture +def app_with_path_setting(app: FastAPI) -> FastAPI: + """Register the demo module against the live app the client talks to.""" + app.state.settings.module_registry.register("pathdemo", _PathCfg) + app.state.pathdemo = _PathServices(settings=_PathCfg()) + return app + + +class TestModulesScreenRenders: + async def test_client_side_visit_does_not_500( + self, + app_with_path_setting: FastAPI, + authenticated_client: httpx.AsyncClient, + ) -> None: + """The reported bug: reaching the page by clicking the sidebar link.""" + resp = await authenticated_client.get("/settings/", headers=_INERTIA) + + assert resp.status_code != _SERVER_ERROR + assert resp.status_code == _OK + + async def test_the_path_survives_as_a_string( + self, + app_with_path_setting: FastAPI, + authenticated_client: httpx.AsyncClient, + ) -> None: + resp = await authenticated_client.get("/settings/", headers=_INERTIA) + modules = resp.json()["props"]["modules"] + + demo = next(m for m in modules if m["package"] == "pathdemo") + media_root = next(f for f in demo["fields"] if f["name"] == "media_root") + assert media_root["value"] == "var/demo/media" + + async def test_full_page_load_still_works( + self, + app_with_path_setting: FastAPI, + authenticated_client: httpx.AsyncClient, + ) -> None: + """The path that always worked must keep working.""" + resp = await authenticated_client.get("/settings/") + + assert resp.status_code == _OK + assert resp.headers["content-type"].startswith("text/html") diff --git a/modules/settings/tests/test_module_settings_serialize.py b/modules/settings/tests/test_module_settings_serialize.py new file mode 100644 index 00000000..cd3cf69e --- /dev/null +++ b/modules/settings/tests/test_module_settings_serialize.py @@ -0,0 +1,92 @@ +"""Serialized module settings must be props, not live Python objects. + +Settings → Modules reflects every installed module's pydantic settings, so the +value types are whatever those modules declared — pydantic has already turned +``media_root: Path`` into a ``PosixPath`` before this screen sees it. Passing +one on untouched puts a value in the Inertia payload that only the HTML render +path can encode, so the page renders on a reload and 500s when an admin clicks +"Settings" in the sidebar. +""" + +from __future__ import annotations + +import json +from datetime import date +from decimal import Decimal +from pathlib import Path +from typing import Any + +import pytest +from settings._module_settings import ( + ModuleSettingField, + ModuleSettingsView, + serialize, +) + + +def _view_with(value: Any, default: Any = "") -> ModuleSettingsView: + return ModuleSettingsView( + module_name="Demo", + package="demo", + env_prefix="SM_DEMO_", + class_name="DemoCfg", + fields=[ + ModuleSettingField( + name="media_root", + env_var="SM_DEMO_MEDIA_ROOT", + value=value, + default=default, + description="", + is_secret=False, + type="Path", + requires_restart=False, + group=None, + ) + ], + ) + + +def _only_field(views: list[dict]) -> dict: + return views[0]["fields"][0] + + +class TestSerializeProducesJsonSafeProps: + def test_a_path_value_becomes_a_string(self) -> None: + result = serialize([_view_with(Path("var/pagebuilder/media"))]) + + assert _only_field(result)["value"] == "var/pagebuilder/media" + + def test_a_path_default_becomes_a_string(self) -> None: + """Defaults reach the payload too — the screen shows both.""" + result = serialize([_view_with("", default=Path("var/media"))]) + + assert _only_field(result)["default"] == "var/media" + + @pytest.mark.parametrize( + "value", + [Path("var/media"), Decimal("1.5"), date(2026, 8, 21)], + ids=["path", "decimal", "date"], + ) + def test_the_payload_survives_a_plain_json_dump(self, value: Any) -> None: + """The check that matters: Starlette's JSONResponse has no encoder. + + Asserting on ``json.dumps`` rather than on each converted type keeps + this honest for value types no one has thought of yet. + """ + result = serialize([_view_with(value)]) + + assert json.dumps(result) + + def test_ordinary_values_are_unchanged(self) -> None: + result = serialize([_view_with(8000, default=25)]) + field = _only_field(result) + + assert field["value"] == 8000 + assert field["default"] == 25 + + def test_the_rest_of_the_view_is_intact(self) -> None: + result = serialize([_view_with(Path("x"))]) + + assert result[0]["package"] == "demo" + assert _only_field(result)["env_var"] == "SM_DEMO_MEDIA_ROOT" + assert _only_field(result)["source"] == "default"