From 885cc06c567c79026e74d792ea5f7434c4ebb8e9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 13:38:15 +0200 Subject: [PATCH 1/2] fix(hosting): stop Inertia payloads rendering as pages and 500ing on visit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in how the Inertia JSON representation is produced and served, both found on a deployed site running the framework. **The payload can be served as the page.** Every Inertia route answers one URL two ways, chosen on the `X-Inertia` request header, and neither response said so. A public-content module marking its page `Cache-Control: public` therefore made both representations share one cache entry: visit a page through the SPA, then open the same URL directly, and the browser serves the stored JSON as the document — the visitor gets `{"component":...}` where the page should be. Reproduced end to end in a browser against a live site. That is the framework's problem to fix rather than each module's, because `InertiaLayoutDataMiddleware` is what merges the signed-in user's `auth` block, permissions and menus into every payload. A route author choosing `public` for their own page content has no way to know the response carries someone's identity — which also made this a disclosure bug, not just a broken page. `InertiaCacheMiddleware` now forces `private, no-store` on any response to an `X-Inertia` request, drops its ETag so nothing can revalidate its way back to a stored copy, and adds `Vary: X-Inertia` to both representations. Documents keep whatever caching their route chose. **A rich prop 500s only on a client-side visit.** `InertiaConfig.json_encoder` exists to run props through `jsonable_encoder`, and upstream applies it only on the full page load; the JSON branch builds a Starlette `JSONResponse` and reaches plain `json.dumps`. So a `Path`, `Decimal` or `date` in props renders on a reload and raises on every navigation to the same route. Settings → Modules hit this against a module declaring `media_root: Path`: TypeError: Object of type PosixPath is not JSON serializable The dependency is now wrapped so both branches encode identically, and `settings.serialize` encodes field values at the boundary where a settings object stops being Python and becomes a prop. No module in this repo declares a non-JSON-native settings field, which is why CI never saw it — the new tests add one and keep it. Claude-Session: https://claude.ai/code/session_013z9hUQKEehrp99jWUDPV39 --- CLAUDE.md | 2 +- .../simple_module_hosting/_inertia_cache.py | 101 +++++++++++++ .../simple_module_hosting/_inertia_json.py | 86 +++++++++++ .../simple_module_hosting/_inertia_setup.py | 6 +- .../simple_module_hosting/_phase_helpers.py | 8 +- framework/hosting/tests/test_inertia_cache.py | 143 ++++++++++++++++++ .../tests/test_inertia_json_encoder.py | 83 ++++++++++ .../hosting/tests/test_middleware_order.py | 10 +- modules/settings/settings/_module_settings.py | 15 +- .../tests/test_module_settings_render.py | 90 +++++++++++ .../tests/test_module_settings_serialize.py | 92 +++++++++++ 11 files changed, 629 insertions(+), 7 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/_inertia_cache.py create mode 100644 framework/hosting/simple_module_hosting/_inertia_json.py create mode 100644 framework/hosting/tests/test_inertia_cache.py create mode 100644 framework/hosting/tests/test_inertia_json_encoder.py create mode 100644 modules/settings/tests/test_module_settings_render.py create mode 100644 modules/settings/tests/test_module_settings_serialize.py 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..9ba11808 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_inertia_cache.py @@ -0,0 +1,101 @@ +"""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" +_HEADER_INERTIA_TRUE = b"true" + +#: 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.""" + return any( + key == _HEADER_INERTIA and value.strip().lower() == _HEADER_INERTIA_TRUE + for key, value 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..d521a4d2 --- /dev/null +++ b/framework/hosting/tests/test_inertia_cache.py @@ -0,0 +1,143 @@ +"""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 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" From ea167e1f7b9b0761656365f0313f27d16a94dadd Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 21 Aug 2026 14:56:03 +0200 Subject: [PATCH 2/2] fix(hosting): detect Inertia requests the way the renderer does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review caught that `is_inertia_request` and the thing it exists to shadow disagreed. It required the `X-Inertia` header to equal `b"true"`, but `fastapi-inertia`'s own `Inertia._is_inertia_request` — the property `render()` consults to choose JSON over HTML — is presence-only: return "X-Inertia" in self._request.headers So a request carrying any other value (`X-Inertia: 1`, `X-Inertia: false`) was rendered as JSON by the library while this middleware did not recognise it as Inertia: no `private, no-store`, no ETag strip, no `Vary`. The payload — auth block, permissions and menus included — went back marked however the route had marked it, which for a public-content route means publicly cacheable. That is precisely the leak this middleware was added to close, reachable by changing one header value. Now mirrors upstream's predicate exactly, so the two cannot drift apart while the library keeps that definition. `_HEADER_INERTIA_TRUE` goes with it. The regression test deliberately does not call `is_inertia_request`: its handler reproduces upstream's presence check directly, so the test asserts agreement between two independent implementations rather than a function agreeing with itself. Claude-Session: https://claude.ai/code/session_013z9hUQKEehrp99jWUDPV39 --- .../simple_module_hosting/_inertia_cache.py | 15 ++++--- framework/hosting/tests/test_inertia_cache.py | 44 +++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/framework/hosting/simple_module_hosting/_inertia_cache.py b/framework/hosting/simple_module_hosting/_inertia_cache.py index 9ba11808..e2e73e78 100644 --- a/framework/hosting/simple_module_hosting/_inertia_cache.py +++ b/framework/hosting/simple_module_hosting/_inertia_cache.py @@ -38,7 +38,6 @@ _SCOPE_HTTP = "http" _HEADER_INERTIA = b"x-inertia" -_HEADER_INERTIA_TRUE = b"true" #: What an Inertia payload is allowed to say about its own cacheability. PAYLOAD_CACHE_CONTROL = "private, no-store" @@ -60,11 +59,15 @@ def add_vary(headers: MutableHeaders, field: str = VARY_FIELD) -> None: def is_inertia_request(scope: Scope) -> bool: - """Whether this request asked for the JSON representation.""" - return any( - key == _HEADER_INERTIA and value.strip().lower() == _HEADER_INERTIA_TRUE - for key, value in scope.get("headers", ()) - ) + """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: diff --git a/framework/hosting/tests/test_inertia_cache.py b/framework/hosting/tests/test_inertia_cache.py index d521a4d2..655d87bc 100644 --- a/framework/hosting/tests/test_inertia_cache.py +++ b/framework/hosting/tests/test_inertia_cache.py @@ -101,6 +101,50 @@ async def test_payload_varies_on_the_header_that_selected_it( 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."""