diff --git a/CLAUDE.md b/CLAUDE.md index 26893989..090a68e7 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_setup_steps` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` / `register_admin_routes(admin_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_admin_routes` is only for modules that serve **both** public and admin pages: a module gets exactly one router per `view_prefix`, which `users` cannot express (sign-in at `/users/login`, management at `/admin/users`). Setting `ModuleMeta.admin_view_prefix` mounts a second view router there. A module whose views are *all* administrative just points `view_prefix` at `/admin/` and keeps using `register_routes`. The prefix is a URL convention, not a permission — guard these routes exactly as you would any other. `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). `register_setup_steps(registry)` lets a module declare what a usable install still needs; while any required step is incomplete `SetupMiddleware` serves the first-run wizard at `/setup` instead of the app. A module that registers nothing never gates — that is how `keycloak` opts out, since its local users table is legitimately empty forever and a host-level superuser count would lock those installs out permanently. **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 → InertiaCache → Setup → Maintenance → 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. `Maintenance` serves a 503 page to everyone but admins while `maintenance_mode` is set on `HostSettings`; it sits inside `InertiaCache` because its 503 is an Inertia payload produced by short-circuiting, and outside the cache guard that payload would ship storable. `Setup` runs just before it, for the same cache reason and because an install that was never set up has nothing meaningful to put into maintenance. +`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → Setup → Maintenance → 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). Inertia does not depend on it: the page url is rewritten to the root-relative form the protocol specifies (`_inertia_url.py`), so no scheme travels in the payload to disagree with the document's — the cross-scheme `pushState` `SecurityError` of GH #223 cannot recur on an install that never set the variable. 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. `Maintenance` serves a 503 page to everyone but admins while `maintenance_mode` is set on `HostSettings`; it sits inside `InertiaCache` because its 503 is an Inertia payload produced by short-circuiting, and outside the cache guard that payload would ship storable. `Setup` runs just before it, for the same cache reason and because an install that was never set up has nothing meaningful to put into maintenance. **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/README.md b/README.md index de5a904d..b5ae64ef 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ bundle rather than a Vite dev server. Useful overrides: | `SM_SECRET_KEY` | generated per start | Persist sessions across restarts | | `SM_DATABASE_URL` | `sqlite+aiosqlite:////app/data/app.db` | Point at Postgres | | `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` | `admin@example.com` / `changeme` | Seed a first admin nobody else can guess | -| `SM_TRUSTED_PROXY` | unset | Set to `*` behind a TLS-terminating reverse proxy | +| `SM_TRUSTED_PROXY` | unset | Set to `*` behind a reverse proxy, so logs record the visitor's IP and not the proxy's | **No background tasks.** The image skips installing the Celery module (`uv sync … --no-install-package simple-module-background-tasks`), so nothing in diff --git a/docker-compose.yml b/docker-compose.yml index 8b0af536..e9dfae97 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -17,6 +17,13 @@ services: # password and prints it — see `docker compose logs app`. - SM_USERS_BOOTSTRAP_EMAIL - SM_USERS_BOOTSTRAP_PASSWORD + # Set to `*` when this sits behind a reverse proxy that terminates TLS, + # so `X-Forwarded-For`/`-Proto` are honoured and the logs record the + # visitor rather than the proxy. Nothing in the UI depends on + # it — the Inertia page url is relative — so leaving it unset is safe, + # and it stays opt-in because a directly-exposed container would be + # trusting a header any client can set. + - SM_TRUSTED_PROXY ports: # SM_APP_PORT frees you from a host-port clash with `make dev`. - "${SM_APP_PORT:-8000}:8000" diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index 918a2ec3..fdf8dfc5 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -83,8 +83,11 @@ InertiaLayoutDataMiddleware `ProxyHeaders` is installed only when `SM_TRUSTED_PROXY` is set (uvicorn's `ProxyHeadersMiddleware`). It sits outermost so the scheme/client corrected from `X-Forwarded-*` is visible to every downstream layer — request logs get -the real client IP, and `request.url.scheme` reflects the proxy-terminated -scheme so Inertia's absolute page url is `https`, not `http`. +the real client IP rather than the proxy's, and `request.url.scheme` reflects +the proxy-terminated scheme. + +Inertia does not depend on it. The page url it ships is root-relative, as the +protocol specifies, so it carries no scheme to disagree with the document's. ### Module-registered middleware ordering diff --git a/docs/framework/middleware.md b/docs/framework/middleware.md index bd1de379..a58b9ddc 100644 --- a/docs/framework/middleware.md +++ b/docs/framework/middleware.md @@ -78,7 +78,11 @@ The last three are ordered relative to each other for reasons worth stating, bec uvicorn's own, installed **only** when `SM_TRUSTED_PROXY` is set — forwarded headers are never trusted by default. It sits outermost so the `X-Forwarded-*`-corrected scheme and client IP reach everything downstream: request logs record the real client rather than the proxy, and `request.url.scheme` reflects `X-Forwarded-Proto`. -Behind a TLS-terminating proxy this is **required**, not a nicety. Without it the app believes it is serving `http` while the browser is on `https`, Inertia's `pushState` sees a cross-scheme URL, throws a `SecurityError`, and login breaks. +Behind a TLS-terminating proxy this is **recommended**: without it the app believes it is serving `http` while the browser is on `https`, and every request log entry is attributed to the proxy's own address rather than the visitor's. + +It used to be load-bearing for the UI too, because Inertia's page url was absolute and its cross-scheme `pushState` threw a `SecurityError` on every page (GH #223). The page url is root-relative now, so that failure mode is gone whether or not this is set. + +It is still load-bearing for anything else that reads `request.url.scheme` or calls `request.url_for(...)` to build an absolute url — OAuth/OIDC's `callback_url` (`modules/users/users/oauth/api.py`, `modules/keycloak/keycloak/endpoints/api.py`) and the locale cookie's `secure` flag (`host/routes_i18n.py`) among them. Left unset behind a TLS-terminating proxy, an OAuth redirect_uri is built as `http://…` and most providers reject it as a mismatch against the `https://…` one registered in their console. 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. diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index 41a17c05..51543838 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -19,7 +19,7 @@ This is the full reference. See [Configuration](/guide/configuration) for a narr | `SM_VITE_PORT` | `5050` | Dev only — port this repo's own host `vite.config.ts` binds to. If you change it, set `SM_VITE_DEV_URL` to match so the backend points the HMR client at the right origin. New scaffolds don't need it — they read `SM_VITE_DEV_URL` directly. | | `SM_PROJECT_ROOT` | unset | Overrides project-root discovery: where the `.env` is looked up and what relative sqlite paths resolve against. Normally unnecessary — settings walk up from the cwd (stopping at repo boundaries and `$HOME`) to find the `.env` on their own. | | `SM_AUTH_PUBLIC_PATHS` | `[]` | JSON array of host-level anonymous-access path prefixes. Escape hatch for exposing a route without a session when no module owns it; modules should prefer the method-aware `register_public_routes` hook. | -| `SM_TRUSTED_PROXY` | unset | Comma-separated proxy IPs / CIDRs whose `X-Forwarded-*` headers are trusted, or `*` to trust any peer (correct when the container is only reachable through one proxy). Setting it installs uvicorn's `ProxyHeadersMiddleware` outermost, so request logs record the real client IP and `request.url.scheme` reflects `X-Forwarded-Proto`. **Required behind a TLS-terminating proxy** — without it Inertia's `pushState` sees a cross-scheme URL, throws a `SecurityError`, and login breaks. Forwarded headers are never trusted by default. | +| `SM_TRUSTED_PROXY` | unset | Comma-separated proxy IPs / CIDRs whose `X-Forwarded-*` headers are trusted, or `*` to trust any peer (correct when the container is only reachable through one proxy). Setting it installs uvicorn's `ProxyHeadersMiddleware` outermost, so request logs record the real client IP and `request.url.scheme` reflects `X-Forwarded-Proto`. **Recommended behind a TLS-terminating proxy**, where otherwise every request in the logs is attributed to the proxy's own address rather than the visitor's. It is no longer needed for Inertia: the page url is root-relative, so `pushState` can't see a cross-scheme url whatever the proxy sends (it used to throw a `SecurityError` on every page — GH #223). It is still needed for anything that builds an absolute url from `request.url.scheme`/`request.url_for(...)` — notably OAuth/OIDC's callback url and the locale cookie's `secure` flag — left unset behind such a proxy, an OAuth `redirect_uri` ships as `http://…` and most providers reject it. Forwarded headers are never trusted by default. | ## DB connection pool diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index 625cb9cb..00417505 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -21,6 +21,7 @@ from starlette.responses import Response from simple_module_hosting._inertia_shared import _INERTIA_HEADER +from simple_module_hosting._inertia_url import patch_relative_page_url from simple_module_hosting.permissions import PERMISSION_DENIED_PREFIX logger = logging.getLogger(__name__) @@ -172,11 +173,25 @@ async def render_error_page( # thing that is missing when the app is half-built, and an error page # that raises while reporting an error leaves the caller with nothing. config: InertiaConfig = request.app.state.sm.inertia_config - inertia = Inertia(request, config) - # This builds its own Inertia instead of going through get_inertia, so - # the share step has to be repeated here. Without it the error page - # renders raw translation keys (host.error.not_found_title) and loses - # auth/menus, so a signed-in user's 404 has no layout. + # Prefer the app's configured dependency over constructing Inertia + # directly: it carries the framework's wraps, and an error page built + # around them is an error page rendered differently from every other + # page. That is how the 404 kept throwing the cross-scheme `pushState` + # SecurityError after the page url was made relative everywhere else. + # The raw construction stays as the fallback for a half-built app — + # the case this whole handler exists to survive — but still goes + # through the same url-relativizing patch directly: a fallback that + # skipped it would reintroduce the exact bug this module exists to + # fix, just for the one request that hit it before setup finished. + inertia_dep = getattr(request.app.state, "inertia_dependency", None) + if inertia_dep is not None: + inertia = inertia_dep(request, None) + else: + inertia = patch_relative_page_url(Inertia(request, config)) + # This does not go through get_inertia, so the share step has to be + # repeated here. Without it the error page renders raw translation keys + # (host.error.not_found_title) and loses auth/menus, so a signed-in + # user's 404 has no layout. shared = getattr(request.state, "inertia_shared", None) if shared: inertia.share(**shared) diff --git a/framework/hosting/simple_module_hosting/_favicon.py b/framework/hosting/simple_module_hosting/_favicon.py new file mode 100644 index 00000000..09717b9d --- /dev/null +++ b/framework/hosting/simple_module_hosting/_favicon.py @@ -0,0 +1,94 @@ +"""The favicon an install has before anyone uploads one. + +Without a default, ``index.html`` emitted no ```` at all, so +every browser fell back to its implicit ``/favicon.ico`` request — which this +app does not route. Anonymous visitors got a 302 to the login page (an HTML +document offered as an image), authenticated ones a 404, and either way a +console error on every full page load, on a brand-new install that has done +nothing wrong. + +Rendered as an SVG ``data:`` URI rather than a file so there is nothing to +mount, nothing to exempt from auth, and nothing to ship in the image: the +static mount is a build artifact (``host/static/dist`` is generated), and a +route would need its own public-route rule. ``img-src`` already allows +``data:``, so the strict CSP is untouched. + +The mark mirrors ``BrandingMark``'s fallback badge — the app's initial on the +brand gradient — so the tab icon and the in-app logo are the same thing, for +whatever the app is named. An uploaded favicon still wins; this is only the +floor. +""" + +from __future__ import annotations + +from functools import lru_cache +from urllib.parse import quote +from xml.sax.saxutils import escape as _xml_escape + +#: sRGB for `--color-primary-600` / `--color-primary-800` +#: (`oklch(0.59 0.14 158)` / `oklch(0.42 0.09 175)`), the two stops of +#: `BRAND_ACCENT`. Hard-coded because a data URI cannot read a CSS custom +#: property — kept beside the tokens they mirror in `ui/styles/globals.css`. +_ACCENT_FROM = "#00955c" +_ACCENT_TO = "#005c4a" + +_SVG = ( + '' + '' + '' + '' + "" + '' + '{initial}' + "" +) + + +@lru_cache(maxsize=64) +def default_favicon_data_uri(app_name: str, accent: str = "") -> str: + """An SVG data URI showing ``app_name``'s initial on the brand gradient. + + ``accent`` is branding's configured primary colour: when set it replaces + both gradient stops with the flat brand colour, so a customised install's + tab icon matches the rest of its chrome. Already validated as a hex colour + by ``BrandingSettings``; anything else is ignored rather than trusted into + the markup. + + Pure function of its two arguments, cached: ``branding_head`` calls it on + every full page render, and with no favicon uploaded — the state every + install starts in — that means every single page load rebuilds and + re-quotes the same SVG. The cache is keyed on ``(app_name, accent)``, both + of which only change when an admin edits branding settings. + """ + initial = _initial(app_name) + from_, to = (accent, accent) if _is_hex_colour(accent) else (_ACCENT_FROM, _ACCENT_TO) + svg = _SVG.format(from_=from_, to=to, initial=initial) + # Everything structural is encoded — `#` above all, or the browser reads + # the gradient reference as the URI's fragment and the shape loses its + # fill, but also `"`, `<`, `>` and spaces so the result stays a single + # valid attribute value without relying on the template's escaping. The + # safe set is only characters that are already legal unencoded in a URI. + return "data:image/svg+xml," + quote(svg, safe="/:=,;()-_.'") + + +def _initial(app_name: str) -> str: + """First character of the app name, XML-escaped, defaulting to ``S``. + + Matches ``BrandingMark``'s ``appName.trim().charAt(0).toUpperCase() || 'S'`` + so the tab and the sidebar badge never disagree. + """ + raw = (app_name or "").strip()[:1].upper() or "S" + return _xml_escape(raw) + + +def _is_hex_colour(value: str) -> bool: + """Whether ``value`` is a ``#rgb``/``#rrggbb`` literal safe to interpolate.""" + if not value.startswith("#"): + return False + body = value[1:] + return len(body) in (3, 6) and all(c in "0123456789abcdefABCDEF" for c in body) + + +__all__ = ["default_favicon_data_uri"] diff --git a/framework/hosting/simple_module_hosting/_inertia_setup.py b/framework/hosting/simple_module_hosting/_inertia_setup.py index 75746d7a..cc6080b5 100644 --- a/framework/hosting/simple_module_hosting/_inertia_setup.py +++ b/framework/hosting/simple_module_hosting/_inertia_setup.py @@ -12,7 +12,9 @@ from inertia import InertiaConfig, inertia_dependency_factory from starlette.requests import Request +from simple_module_hosting._favicon import default_favicon_data_uri from simple_module_hosting._inertia_json import json_safe_inertia_dependency +from simple_module_hosting._inertia_url import relative_page_url_dependency from simple_module_hosting.settings import Settings logger = logging.getLogger(__name__) @@ -40,16 +42,22 @@ def branding_head(request: Request) -> dict: The favicon URL is read from the module rather than assembled here: branding owns its route shape, and framework code must not reach into a plugin (SM009). ``BrandingHead`` still applies it client-side too, so a favicon - changed at runtime updates without a reload. + changed at runtime updates without a reload. When nothing has been uploaded + — the state every install starts in — it falls back to a generated mark + rather than ``None``, because omitting the tag sends the browser to + ``/favicon.ico``, which this app does not serve. """ services = getattr(request.app.state, "branding", None) settings = getattr(services, "settings", None) - if settings is None: - return {"app_name": _DEFAULT_APP_NAME, "theme_color": None, "favicon_url": None} + app_name = getattr(settings, "app_name", "") or _DEFAULT_APP_NAME + accent = getattr(settings, "primary_color", "") or "" + favicon_url = getattr(services, "favicon_url", None) or default_favicon_data_uri( + app_name, accent + ) return { - "app_name": getattr(settings, "app_name", "") or _DEFAULT_APP_NAME, - "theme_color": getattr(settings, "primary_color", "") or None, - "favicon_url": getattr(services, "favicon_url", None), + "app_name": app_name, + "theme_color": accent or None, + "favicon_url": favicon_url, } @@ -185,9 +193,15 @@ def setup_inertia( use_flash_errors=True, ) - # 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)) + # Two wraps over the stock dependency, each closing a gap in upstream: + # * JSON branch builds a Starlette JSONResponse directly, so the encoder + # configured above only ever applies to full page loads — wrap it so a + # client-side visit encodes the same props the same way. + # * The page url is absolute, which the browser rejects for pushState + # behind a TLS-terminating proxy — wrap it back to the root-relative + # path the Inertia protocol specifies. + inertia_dep = relative_page_url_dependency( + 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/_inertia_url.py b/framework/hosting/simple_module_hosting/_inertia_url.py new file mode 100644 index 00000000..6b71425f --- /dev/null +++ b/framework/hosting/simple_module_hosting/_inertia_url.py @@ -0,0 +1,131 @@ +"""Make Inertia's page ``url`` root-relative, as the protocol specifies. + +``fastapi-inertia`` builds the page object with the *absolute* request url:: + + page_data = { + "component": self._component, + "props": await self._build_props(), + "url": str(self._request.url), # http://host:8000/dashboard/ + "version": self._config.version, + } + +The Inertia protocol says otherwise — every official adapter emits a +root-relative path (Laravel's ``$request->getRequestUri()``, Rails' +``request.original_fullpath``), and the documented page object is +``{"component": "Event", "props": {...}, "url": "/events/80", ...}``. The +client hands that value straight to ``history.pushState``/``replaceState``, +which resolves a relative url against the current document and so can never +disagree with it. + +An absolute url can, and behind a TLS-terminating reverse proxy it always +does. The proxy speaks https to the browser and http to the container, so +``request.url`` is ``http://…`` while the document origin is ``https://…``, +and the browser rejects the state write:: + + SecurityError: Failed to execute 'pushState' on 'History': A history state + object with URL 'http://example.com/' cannot be created in a document with + origin 'https://example.com' and URL 'https://example.com/' + +That fires on *every* page — the initial ``replaceState`` and every subsequent +visit — so history state is never written: back/forward navigation and scroll +restoration silently stop working, and the console fills with the error. The +throw escapes as an unhandled rejection, because the client's +``isHistoryThrottleError`` guard matches the string ``"history.pushState"`` +while the browser's message reads ``"Failed to execute 'pushState'"``. + +``SM_TRUSTED_PROXY`` fixes the scheme by trusting ``X-Forwarded-Proto``, and is +still worth setting — it is what makes request logs record the real client IP. +But an install should not need it merely to have working history, and trusting +forwarded headers is a security decision that must stay opt-in: the default +image is documented as runnable standalone, where a spoofed ``X-Forwarded-For`` +would poison the audit log. Emitting the relative url the protocol asks for +removes the dependency entirely, whatever the deployment looks like. + +Patched at ``_get_page_data`` because it is the single choke point: upstream's +SSR, JSON and full-page-load branches all build their payload from it. The +replacement binds to the instance, so the library's class is untouched. +""" + +from __future__ import annotations + +import logging +from typing import Any +from urllib.parse import urlsplit + +logger = logging.getLogger(__name__) + + +def patch_relative_page_url(inertia: Any) -> Any: + """Patch a single Inertia instance in place so its page ``url`` is + root-relative, and return it. + + Shared by :func:`relative_page_url_dependency` (the normal per-request + path) and by callers that already hold a bare instance — the error + handler's half-built-app fallback — so there is exactly one place that + knows how to apply the patch rather than a second copy of this hasattr + check. + + An instance that doesn't expose the hook is handed back untouched: the + failure mode is the absolute url that already ships, and refusing to boot + because an upstream attribute moved would be a worse trade. + """ + if hasattr(inertia, "_get_page_data"): + inertia._get_page_data = _page_data_for(inertia) + else: # pragma: no cover - upstream layout changed + logger.warning( + "Inertia instance has no _get_page_data; the page url stays " + "absolute and history writes will fail behind a TLS proxy" + ) + return inertia + + +def relative_page_url_dependency(inertia_dep: Any) -> Any: + """Wrap an Inertia dependency so its page ``url`` is root-relative. + + Keeps the ``(request, client)`` shape ``inertia_dependency_factory`` + returns, and composes with the other wraps in either order — each patches a + different instance attribute, and the JSON wrap looks ``_get_page_data`` up + on the instance at call time. + """ + + def dependency(request: Any, client: Any = None) -> Any: + return patch_relative_page_url(inertia_dep(request, client)) + + return dependency + + +def _page_data_for(inertia: Any) -> Any: + """Build the instance's replacement ``_get_page_data``.""" + stock = inertia._get_page_data + + async def _get_page_data() -> dict: + page_data = await stock() + url = page_data.get("url") + if isinstance(url, str): + page_data["url"] = to_relative_url(url) + return page_data + + return _get_page_data + + +def to_relative_url(url: str) -> str: + """Reduce an absolute url to the path-and-query the protocol wants. + + A url that is already relative is returned unchanged, so this is safe to + apply twice and safe to apply to a payload upstream may one day fix. + Anything unparseable is passed through rather than mangled — a wrong url is + still better than a crash on the render path. + """ + try: + parts = urlsplit(url) + except ValueError: # pragma: no cover - urlsplit is near-total + return url + if not parts.scheme and not parts.netloc: + return url + relative = parts.path or "/" + if parts.query: + relative = f"{relative}?{parts.query}" + return relative + + +__all__ = ["patch_relative_page_url", "relative_page_url_dependency", "to_relative_url"] diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 7130885d..120ad747 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -167,8 +167,10 @@ def install_middleware( app.add_middleware(RequestLoggingMiddleware) app.add_middleware(CorrelationIdMiddleware) # Outermost: rewrite scheme/client from X-Forwarded-* before anything else - # reads them, so request logs see the real client IP and Inertia's absolute - # page url carries the proxy-terminated scheme (GH #223). Gated on an + # reads them, so request logs see the real client IP rather than the + # proxy's. Inertia no longer needs this: its page url is + # root-relative, so pushState can't see a cross-scheme url either way (it + # used to throw a SecurityError on every page — GH #223). Gated on an # explicit trust setting — never trust forwarded headers by default. if settings.trusted_proxy: app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=settings.trusted_proxy) diff --git a/framework/hosting/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index 54e82cf8..24d6dfab 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/framework/hosting/simple_module_hosting/host_settings.py @@ -58,8 +58,15 @@ class HostSettings(BaseSettings): # the container is only reachable through a single proxy), or a comma- # separated list of proxy IPs / CIDRs. Drives uvicorn's # ProxyHeadersMiddleware so request.url.scheme reflects X-Forwarded-Proto - # behind a TLS-terminating proxy — without it Inertia's pushState throws a - # cross-scheme SecurityError and login breaks (GH #223). + # and request logs record the real client IP rather than the proxy's. + # Recommended behind a TLS-terminating proxy, not required for + # Inertia: the page url is root-relative, so pushState can't see a + # cross-scheme url regardless (it used to throw a SecurityError on every + # page — GH #223). Still required for anything else that reads + # request.url.scheme or calls request.url_for(...) to build an absolute + # url, e.g. OAuth's callback_url and the locale cookie's `secure` flag — + # left unset behind such a proxy, an OAuth redirect_uri ships as http:// + # and most providers reject it. trusted_proxy: str | None = None auth_provider: str = DEFAULT_AUTH_PROVIDER diff --git a/framework/hosting/tests/test_branding_head.py b/framework/hosting/tests/test_branding_head.py index fe3790b8..03b4c2a2 100644 --- a/framework/hosting/tests/test_branding_head.py +++ b/framework/hosting/tests/test_branding_head.py @@ -4,6 +4,7 @@ from types import SimpleNamespace +from simple_module_hosting._favicon import default_favicon_data_uri from simple_module_hosting._inertia_setup import branding_head @@ -16,7 +17,11 @@ def _request(branding: object | None) -> SimpleNamespace: def test_defaults_when_branding_not_installed() -> None: meta = branding_head(_request(None)) - assert meta == {"app_name": "SimpleModule", "theme_color": None, "favicon_url": None} + assert meta == { + "app_name": "SimpleModule", + "theme_color": None, + "favicon_url": default_favicon_data_uri("SimpleModule"), + } def test_reads_app_name_and_theme_color() -> None: @@ -29,7 +34,11 @@ def test_reads_app_name_and_theme_color() -> None: def test_blank_values_fall_back() -> None: settings = SimpleNamespace(app_name="", primary_color="") meta = branding_head(_request(SimpleNamespace(settings=settings))) - assert meta == {"app_name": "SimpleModule", "theme_color": None, "favicon_url": None} + assert meta == { + "app_name": "SimpleModule", + "theme_color": None, + "favicon_url": default_favicon_data_uri("SimpleModule"), + } def test_favicon_url_comes_from_the_module_not_from_here() -> None: @@ -42,8 +51,25 @@ def test_favicon_url_comes_from_the_module_not_from_here() -> None: assert branding_head(_request(services))["favicon_url"] == "/api/branding/favicon?v=abc" -def test_favicon_url_is_none_on_a_host_without_that_attribute() -> None: - # An older branding release exposes no favicon_url; the shell just omits - # the link rather than erroring, and BrandingHead still sets it client-side. +def test_an_uploaded_favicon_always_wins_over_the_default() -> None: + services = SimpleNamespace( + settings=SimpleNamespace(app_name="Acme", primary_color="#1a7dd1"), + favicon_url="/api/branding/favicon?v=abc", + ) + assert branding_head(_request(services))["favicon_url"] == "/api/branding/favicon?v=abc" + + +def test_a_host_without_that_attribute_still_gets_a_favicon() -> None: + # An older branding release exposes no favicon_url. Omitting the link tag + # sends the browser to /favicon.ico, which this app does not route — a 404 + # (or a 302 to the login page) in the console on every full page load — so + # the generated mark stands in. services = SimpleNamespace(settings=SimpleNamespace(app_name="Acme", primary_color="")) - assert branding_head(_request(services))["favicon_url"] is None + assert branding_head(_request(services))["favicon_url"] == default_favicon_data_uri("Acme") + + +def test_the_default_follows_the_configured_brand_colour() -> None: + services = SimpleNamespace(settings=SimpleNamespace(app_name="Acme", primary_color="#1a7dd1")) + favicon = branding_head(_request(services))["favicon_url"] + assert favicon == default_favicon_data_uri("Acme", "#1a7dd1") + assert favicon != default_favicon_data_uri("Acme") diff --git a/framework/hosting/tests/test_error_page_shared_props.py b/framework/hosting/tests/test_error_page_shared_props.py index 60c945a3..6f827254 100644 --- a/framework/hosting/tests/test_error_page_shared_props.py +++ b/framework/hosting/tests/test_error_page_shared_props.py @@ -181,3 +181,36 @@ async def test_unmatched_url_sends_no_message( "the status phrase reached the page as a message, so it renders in " f"place of the catalog description: {props['message']!r}" ) + + +class TestErrorPageUrl: + """The error page's own url must be relative, like every other page's. + + ``render_error_page`` used to construct ``Inertia`` directly, so it missed + the wrap that rewrites the absolute url upstream emits. Every real page + was fixed and the 404 alone kept throwing the cross-scheme ``pushState`` + ``SecurityError`` behind a TLS-terminating proxy — the one page a lost + visitor is most likely to be looking at. + """ + + async def test_the_error_page_url_is_relative( + self, authenticated_client: httpx.AsyncClient + ) -> None: + resp = await authenticated_client.get(_MISSING_PATH) + url = _inertia_page(resp.text)["url"] + assert url == _MISSING_PATH, f"error page url is not root-relative: {url!r}" + + async def test_the_error_page_url_carries_no_origin( + self, authenticated_client: httpx.AsyncClient + ) -> None: + url = _inertia_page((await authenticated_client.get(_MISSING_PATH)).text)["url"] + assert not url.startswith("http://") + assert not url.startswith("https://") + + async def test_a_forbidden_page_url_is_relative_too( + self, guarded_client: httpx.AsyncClient + ) -> None: + """403 renders through the same handler, so it must not regress alone.""" + resp = await guarded_client.get(_GUARDED_PATH) + assert resp.status_code == _FORBIDDEN + assert _inertia_page(resp.text)["url"] == _GUARDED_PATH diff --git a/framework/hosting/tests/test_favicon_default.py b/framework/hosting/tests/test_favicon_default.py new file mode 100644 index 00000000..b04bb8da --- /dev/null +++ b/framework/hosting/tests/test_favicon_default.py @@ -0,0 +1,72 @@ +"""The generated favicon an install has before anyone uploads one. + +Emitted as a data URI straight into a ``href="…"`` attribute, so the encoding +is the whole risk: a surviving ``#`` truncates the URI at the gradient +reference and the mark loses its fill, and a surviving ``"`` ends the attribute. +""" + +from __future__ import annotations + +from urllib.parse import unquote + +import pytest +from simple_module_hosting._favicon import default_favicon_data_uri + +_PREFIX = "data:image/svg+xml," + + +def _svg(uri: str) -> str: + return unquote(uri.removeprefix(_PREFIX)) + + +class TestEncoding: + def test_it_is_an_svg_data_uri(self) -> None: + assert default_favicon_data_uri("SimpleModule").startswith(_PREFIX) + + @pytest.mark.parametrize("char", ["#", '"', "<", ">", " "]) + def test_no_uri_breaking_character_survives_encoding(self, char: str) -> None: + payload = default_favicon_data_uri("SimpleModule").removeprefix(_PREFIX) + assert char not in payload + + def test_the_payload_decodes_to_well_formed_svg(self) -> None: + svg = _svg(default_favicon_data_uri("SimpleModule")) + assert svg.startswith("") + # The gradient is referenced by the id it defines; a mismatch here is + # an unfilled square that still "renders". + assert 'id="a"' in svg and "url(#a)" in svg + + +class TestTheMark: + @pytest.mark.parametrize( + ("app_name", "initial"), + [("SimpleModule", "S"), ("Acme Corp", "A"), (" spaced", "S"), ("zephyr", "Z")], + ) + def test_it_shows_the_app_initial(self, app_name: str, initial: str) -> None: + """Mirrors BrandingMark's badge, so tab and sidebar never disagree.""" + assert f">{initial}" in _svg(default_favicon_data_uri(app_name)) + + @pytest.mark.parametrize("app_name", ["", " "]) + def test_a_nameless_app_still_gets_a_mark(self, app_name: str) -> None: + assert ">S" in _svg(default_favicon_data_uri(app_name)) + + def test_an_app_name_cannot_inject_markup(self) -> None: + svg = _svg(default_favicon_data_uri("