diff --git a/docs/guide/installation.md b/docs/guide/installation.md index a4b3bf5f..9fc7f608 100644 --- a/docs/guide/installation.md +++ b/docs/guide/installation.md @@ -140,6 +140,22 @@ smpy package-update Pass `--dry-run` first to preview the diff. +Each constraint keeps the operator you wrote — `==0.0.32` becomes `==0.0.33`, +`>=0.0.32` becomes `>=0.0.33` — so bumping versions never changes your pinning +policy. That matters because the published module wheels pin their framework +deps exactly: a host loosened to `>=` hands the effective version to whichever +wheel pins hardest, rather than deciding it itself. + +Two consequences worth knowing: + +- A dependency whose upper bound excludes the latest release (`>=0.1,<1.0` + when 2.0.0 is out) is reported and left alone, rather than having its ceiling + silently dropped. +- A dependency with no constraint at all has no pin style to preserve, so it + gets `>=`. + +Pass `--loosen` to rewrite every constraint to `>=` instead. + ## Troubleshooting **`smpy: command not found`** after `uv tool install`. diff --git a/docs/modules/branding.md b/docs/modules/branding.md index 496ce712..7f863fa2 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -2,7 +2,7 @@ White-labels the application. An administrator sets the **app name**, **logo** (plus an optional dark-background variant), **favicon**, **primary colour**, **[design pack](/framework-conventions#design-packs-site-wide-look)**, and a site-wide **announcement banner** — from an admin page, with no code change or redeploy. -Values persist in the shared [settings](/modules/settings) store (there is no branding table) and reach **every** Inertia page — authenticated *and* guest — through a registered shared-props provider, so the frontend can render the name, swap the logo/favicon, apply the brand colour, and show the banner everywhere. Footer content is owned by the framework site layouts, not branding. +Values persist in the shared [settings](/modules/settings) store (there is no branding table) and reach **every** Inertia page — authenticated *and* guest — through a registered shared-props provider, so the frontend can render the name, swap the logo/favicon, apply the brand colour, show the banner and set the footer links everywhere. ## ModuleMeta @@ -104,6 +104,7 @@ DB-backed via `register_module_settings`; pydantic defaults seed at boot. Edited | `favicon_file_id` | `""` | UUID of the favicon; `""` ⇒ no custom favicon. | | `banner_message` | `""` | Site-wide announcement text (≤ 500 chars); `""` ⇒ no banner. | | `banner_severity` | `"info"` | One of `info`, `warning`, `danger`. Unknown values normalise to `info`. | +| `footer_links` | `[]` | Up to 6 `{label, href}` links shown in the site footer; `[]` ⇒ show the framework's own links. | `app_name` rejects control characters, not just blanks: the name is used in HTML titles and — critically — email `Subject` headers, where an embedded CR/LF would survive a bare `strip()` and then raise, breaking every transactional email. @@ -147,6 +148,18 @@ A message plus a severity, rendered above every shell — app, public and auth Severity colours are semantic, not brand-tinted: a warning wearing the deployment's accent colour stops reading as a warning. +## Footer links + +The footer renders on every page — the app shell and the public site — so its links are outward-facing attribution on a deployed product. Setting `footer_links` replaces the framework's own *Docs / Changelog / GitHub* row, which points at `antosubash/simple_module_python`. + +Leaving the list empty keeps those framework links, so a deployment that never touches this looks exactly as it did. Clearing the list back to empty is how you return to them. + +Each `href` must be `http://`, `https://`, `mailto:` or a site-relative path beginning with a single `/`. That is an allow-list rather than tidiness: the value is rendered straight into an `` on every page, signed-in or not, so `javascript:` and `data:` would make the branding screen a stored-XSS sink for anyone holding `branding.manage`. Scheme-relative `//host` is rejected too — it reads as a relative path but navigates off-site. + +Labels are trimmed, non-blank, ≤ 40 characters and reject control characters, on the same reasoning as `app_name`. + +On the frontend, `BrandingFooter` takes an optional `links` prop and falls back to `BRAND_FOOTER_LINKS` when it is absent, `null` or empty; `SidebarLayout` and `PublicLayout` pass the shared prop through, so a host gets the override without forking either layout. + ## Dark-background logo The sidebar and mobile bar sit on a near-black surface in every theme, while the sign-in card and public page are light — so a single logo cannot read on both. Uploading a *Logo (dark backgrounds)* variant swaps it in on those surfaces only. @@ -166,12 +179,13 @@ On startup the module registers a shared-props provider (`register_inertia_share "logoUrl": "/api/branding/logo?v=", "logoDarkUrl": "/api/branding/logo-dark?v=", "faviconUrl": "/api/branding/favicon?v=", - "banner": { "message": "Maintenance at 22:00 UTC", "severity": "warning" } + "banner": { "message": "Maintenance at 22:00 UTC", "severity": "warning" }, + "footerLinks": [{ "label": "Handbook", "href": "https://acme.example.org/handbook" }] } } ``` -`primaryColor` and `designPack` are `null` when unset; the three image URLs are `null` when no file is configured. `banner` is `null` when no message is set, so the frontend renders nothing at all rather than an empty bar. +`primaryColor` and `designPack` are `null` when unset; the three image URLs are `null` when no file is configured. `banner` is `null` when no message is set, so the frontend renders nothing at all rather than an empty bar. `footerLinks` is `null` when none are configured, which is what tells the frontend to keep the framework's own links rather than render an empty row. The provider is defensive — it returns `{}` if branding state isn't mounted yet, so a half-booted app never errors a render. Because changes go through the settings store, a save hot-reloads `app.state.branding.settings`; the next render reflects the new values without a restart. diff --git a/docs/reference/make-commands.md b/docs/reference/make-commands.md index 5f148c58..9e3cb5f5 100644 --- a/docs/reference/make-commands.md +++ b/docs/reference/make-commands.md @@ -49,7 +49,7 @@ Both cache the scratch host under `.smpy/verify-host`; pass `--fresh` to rebuild | Command | What | |---|---| -| `smpy package-update` | Bump every `simple_module_*` dependency in `pyproject.toml` to the latest PyPI version. `--dry-run` previews the diff. | +| `smpy package-update` | Bump every `simple_module_*` dependency in `pyproject.toml` to the latest PyPI version, keeping each constraint's operator (`==` stays `==`). `--dry-run` previews the diff; `--loosen` rewrites everything to `>=`. | | `smpy skills add\|list\|update` | Install or update the bundled agent skills under `.claude/skills/` for use with Claude Code. | ### Module-contributed plugins diff --git a/framework/cli/simple_module_cli/package_update.py b/framework/cli/simple_module_cli/package_update.py index 9b63d58e..e553803c 100644 --- a/framework/cli/simple_module_cli/package_update.py +++ b/framework/cli/simple_module_cli/package_update.py @@ -2,8 +2,14 @@ Walks the project's ``pyproject.toml`` (and any ``[tool.uv.workspace]`` members), finds every dependency whose distribution name starts with ``simple_module_`` / -``simple-module-``, queries PyPI for the latest non-yanked release, and rewrites -the constraint to ``name>=``. +``simple-module-``, queries PyPI for the latest non-yanked release, and points +the constraint at it. + +The rewrite preserves the pin style you wrote: ``==0.0.32`` becomes +``==0.0.33``, ``>=0.0.32`` becomes ``>=0.0.33``, and an upper bound that +excludes the latest release skips the dependency instead of being dropped. +``--loosen`` restores the older behaviour of rewriting every constraint to +``name>=``. Dependencies whose ``[tool.uv.sources]`` entry points at a workspace member, a local path, a git ref, or a URL are left untouched — those aren't installed @@ -12,11 +18,7 @@ from __future__ import annotations -import json import re -import urllib.error -import urllib.request -from collections.abc import Callable from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass from pathlib import Path @@ -26,17 +28,12 @@ import typer from tomlkit.items import Array, Table -__all__ = ["package_update", "run_update"] +from simple_module_cli.pypi import Fetcher, default_fetcher, fetch_latest +from simple_module_cli.requirements import parse_requirement, rewrite_requirement -Fetcher = Callable[[str], dict[str, Any]] +__all__ = ["package_update", "run_update"] -_PYPI_URL = "https://pypi.org/pypi/{name}/json" _SM_PREFIX_RE = re.compile(r"^simple[_-]module[_-]", re.IGNORECASE) -# PEP 440 release segments contain only digits + dots; any letter signals -# a pre/post/dev release (a, b, rc, post, dev). Coarser than packaging.version -# but `packaging` isn't a CLI dep (see test_no_framework_deps.py). -_PRE_RELEASE_RE = re.compile(r"[a-zA-Z]") -_REQ_OPS = ("===", "==", ">=", "<=", "!=", "~=", ">", "<") @dataclass(frozen=True) @@ -60,14 +57,8 @@ def _is_sm_package(name: str) -> bool: def _dep_name(spec: str) -> str | None: """Extract the distribution name from a PEP 508 requirement string.""" - base = spec.split(";", 1)[0].strip() - base = base.split("[", 1)[0] - for op in _REQ_OPS: - if op in base: - base = base.split(op, 1)[0] - break - name = base.strip() - return name or None + parsed = parse_requirement(spec) + return parsed.name if parsed else None def _is_local_source(source: Any) -> bool: @@ -89,40 +80,6 @@ def _get_uv_section(doc: tomlkit.TOMLDocument, key: str) -> dict[str, Any] | Non return section if isinstance(section, dict) else None -def _fetch_latest(name: str, *, include_pre: bool, fetcher: Fetcher) -> str | None: - try: - data = fetcher(_PYPI_URL.format(name=name)) - except (urllib.error.HTTPError, urllib.error.URLError): - return None - releases = data.get("releases") or {} - candidates: list[str] = [] - for version, files in releases.items(): - if not files: - continue - if any(f.get("yanked") for f in files): - continue - if not include_pre and _PRE_RELEASE_RE.search(version): - continue - candidates.append(version) - if candidates: - return max(candidates, key=_version_key) - info = data.get("info") or {} - return info.get("version") - - -def _version_key(v: str) -> tuple[int, ...]: - parts: list[int] = [] - for part in v.split("."): - digits = re.match(r"\d+", part) - parts.append(int(digits.group()) if digits else 0) - return tuple(parts) - - -def _default_fetcher(url: str) -> dict[str, Any]: - with urllib.request.urlopen(url, timeout=10) as resp: - return json.loads(resp.read().decode("utf-8")) - - def _workspace_member_dirs(root_pyproject: Path, doc: tomlkit.TOMLDocument) -> list[Path]: workspace = _get_uv_section(doc, "workspace") if workspace is None: @@ -166,6 +123,7 @@ def _process_file( doc: tomlkit.TOMLDocument, *, cache: dict[str, str | None], + loosen: bool = False, ) -> tuple[list[Change], list[Skip], tomlkit.TOMLDocument | None]: project = doc.get("project") if not isinstance(project, (dict, Table)): @@ -190,11 +148,13 @@ def _process_file( if latest is None: skips.append(Skip(path, name, "not found on PyPI")) continue - new_dep = f"{name}>={latest}" - if new_dep == dep_str.strip(): + result = rewrite_requirement(dep_str, latest, loosen=loosen) + if result.spec is None: + if result.reason: + skips.append(Skip(path, name, result.reason)) continue - deps[idx] = new_dep - changes.append(Change(path, name, dep_str.strip(), new_dep)) + deps[idx] = result.spec + changes.append(Change(path, name, dep_str.strip(), result.spec)) return changes, skips, doc if changes else None @@ -226,6 +186,7 @@ def run_update( *, dry_run: bool = False, include_pre: bool = False, + loosen: bool = False, fetcher: Fetcher | None = None, ) -> None: """Programmatic entry point — separated from the Typer command for testing.""" @@ -234,7 +195,7 @@ def run_update( typer.echo(f"ERROR: {root} not found.", err=True) raise typer.Exit(code=1) - fetch = fetcher or _default_fetcher + fetch = fetcher or default_fetcher root_doc = tomlkit.parse(root.read_text(encoding="utf-8")) files: list[tuple[Path, tomlkit.TOMLDocument]] = [(root, root_doc)] for member in _workspace_member_dirs(root, root_doc): @@ -246,7 +207,7 @@ def run_update( if unique_names: with ThreadPoolExecutor(max_workers=min(8, len(unique_names))) as pool: results = pool.map( - lambda n: (n, _fetch_latest(n, include_pre=include_pre, fetcher=fetch)), + lambda n: (n, fetch_latest(n, include_pre=include_pre, fetcher=fetch)), unique_names, ) cache = dict(results) @@ -256,7 +217,7 @@ def run_update( pending: list[tuple[Path, tomlkit.TOMLDocument]] = [] for file, doc in files: - changes, skips, new_doc = _process_file(file, doc, cache=cache) + changes, skips, new_doc = _process_file(file, doc, cache=cache, loosen=loosen) all_changes.extend(changes) all_skips.extend(skips) if new_doc is not None: @@ -282,6 +243,20 @@ def package_update( bool, typer.Option("--include-pre", help="Include pre-release versions."), ] = False, + loosen: Annotated[ + bool, + typer.Option( + "--loosen", + help="Rewrite every constraint to `>=` instead of keeping its operator.", + ), + ] = False, ) -> None: - """Update all simple_module_* dependencies to the latest PyPI versions.""" - run_update(path, dry_run=dry_run, include_pre=include_pre) + """Update all simple_module_* dependencies to the latest PyPI versions. + + Each constraint keeps the operator it already has — `==0.0.32` becomes + `==0.0.33`, `>=0.0.32` becomes `>=0.0.33` — so bumping versions never + silently changes a project's pinning policy. A dependency whose upper + bound excludes the latest release is reported and left alone. Pass + `--loosen` to rewrite everything to `>=` instead. + """ + run_update(path, dry_run=dry_run, include_pre=include_pre, loosen=loosen) diff --git a/framework/cli/simple_module_cli/pypi.py b/framework/cli/simple_module_cli/pypi.py new file mode 100644 index 00000000..c119ee6a --- /dev/null +++ b/framework/cli/simple_module_cli/pypi.py @@ -0,0 +1,55 @@ +"""PyPI release lookup for ``smpy package-update``. + +Split out of ``package_update`` so the network half is separately testable and +neither file approaches the 300-line cap. The ``Fetcher`` indirection is what +lets the CLI tests run without touching the network. +""" + +from __future__ import annotations + +import json +import re +import urllib.error +import urllib.request +from collections.abc import Callable +from typing import Any + +from simple_module_cli.requirements import version_key + +__all__ = ["PYPI_URL", "Fetcher", "default_fetcher", "fetch_latest"] + +Fetcher = Callable[[str], dict[str, Any]] + +PYPI_URL = "https://pypi.org/pypi/{name}/json" + +# PEP 440 release segments contain only digits + dots; any letter signals +# a pre/post/dev release (a, b, rc, post, dev). Coarser than packaging.version +# but `packaging` isn't a CLI dep (see test_no_framework_deps.py). +_PRE_RELEASE_RE = re.compile(r"[a-zA-Z]") + + +def fetch_latest(name: str, *, include_pre: bool, fetcher: Fetcher) -> str | None: + """Latest non-yanked release of ``name``, or ``None`` if PyPI doesn't have it.""" + try: + data = fetcher(PYPI_URL.format(name=name)) + except (urllib.error.HTTPError, urllib.error.URLError): + return None + releases = data.get("releases") or {} + candidates: list[str] = [] + for version, files in releases.items(): + if not files: + continue + if any(f.get("yanked") for f in files): + continue + if not include_pre and _PRE_RELEASE_RE.search(version): + continue + candidates.append(version) + if candidates: + return max(candidates, key=version_key) + info = data.get("info") or {} + return info.get("version") + + +def default_fetcher(url: str) -> dict[str, Any]: + with urllib.request.urlopen(url, timeout=10) as resp: + return json.loads(resp.read().decode("utf-8")) diff --git a/framework/cli/simple_module_cli/requirements.py b/framework/cli/simple_module_cli/requirements.py new file mode 100644 index 00000000..c490dcf0 --- /dev/null +++ b/framework/cli/simple_module_cli/requirements.py @@ -0,0 +1,218 @@ +"""PEP 508 requirement rewriting for ``smpy package-update``. + +Split out of ``package_update`` so the version-bump rules live in one place +with their own tests, and so neither file approaches the 300-line cap. + +The rule that matters: bumping a dependency changes its *version*, not its +*pin style*. ``simple_module_core==0.0.32`` becomes ``==0.0.33``, not +``>=0.0.33`` — a host that pins exactly has made a deliberate choice, and the +published module wheels pin exactly too, so loosening the host leaves the +effective version decided by whichever wheel pins hardest. ``--loosen`` +restores the old blanket ``>=`` rewrite for anyone who wants it. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass + +__all__ = ["ParsedRequirement", "RewriteResult", "parse_requirement", "rewrite_requirement"] + +# Longest-first: ``===`` must win over ``==``, and ``>=`` over ``>``. +_OPS = ("===", "==", "~=", "!=", ">=", "<=", ">", "<") +_CLAUSE_RE = re.compile(rf"\s*({'|'.join(re.escape(op) for op in _OPS)})\s*([^,]+)") + +#: Operators naming a plain floor the bump should raise. ``>`` is deliberately +#: absent: ``>0.0.31`` is already satisfied by anything newer, and rewriting it +#: to ``>0.0.33`` would exclude the very version being installed. ``~=`` is +#: absent too — it carries an implicit ceiling, handled separately below. +_FLOOR_OPS = frozenset({"==", "===", ">="}) + +#: Suffix marking a PEP 440 wildcard (``==1.4.*``, ``!=1.0.*``). These match on +#: a *prefix* of the release segments, so they must never go through the +#: numeric comparison — ``version_key`` maps ``*`` to 0, which would read +#: ``1.0.*`` as ``1.0.0`` and quietly answer the wrong question. +_WILDCARD = ".*" + + +@dataclass(frozen=True) +class ParsedRequirement: + """A PEP 508 requirement split into the parts the rewrite cares about.""" + + name: str + #: Extras bracket including the brackets (``"[redis]"``), or ``""``. + extras: str + #: ``(operator, version)`` pairs in source order. + clauses: tuple[tuple[str, str], ...] + #: Environment marker including the leading ``;``, or ``""``. + marker: str + + +@dataclass(frozen=True) +class RewriteResult: + """Outcome of a rewrite: a new spec, or a reason there isn't one.""" + + #: The rewritten requirement, or ``None`` when nothing should change. + spec: str | None + #: Human-readable reason when ``spec`` is ``None`` and the user should be + #: told (an excluded latest); ``None`` when the no-op is unremarkable. + reason: str | None = None + + +def version_key(version: str) -> tuple[int, ...]: + """Coarse PEP 440 release-segment ordering. + + ``packaging`` isn't a CLI dependency (see ``test_no_framework_deps.py``), + so this compares the numeric release segments only. Good enough to answer + "does the latest release trip this upper bound", which is all it's for. + """ + parts: list[int] = [] + for part in version.split("."): + digits = re.match(r"\d+", part) + parts.append(int(digits.group()) if digits else 0) + return tuple(parts) + + +def _compare(left: str, right: str) -> int: + """Three-way compare two versions on their release segments, zero-padded.""" + a, b = version_key(left), version_key(right) + width = max(len(a), len(b)) + a += (0,) * (width - len(a)) + b += (0,) * (width - len(b)) + return (a > b) - (a < b) + + +def parse_requirement(spec: str) -> ParsedRequirement | None: + """Split a PEP 508 requirement string, or ``None`` if the name is unusable.""" + base, sep, marker = spec.partition(";") + base = base.strip() + marker = f";{marker}" if sep else "" + + head, bracket, rest = base.partition("[") + if bracket: + extras_body, closed, tail = rest.partition("]") + if not closed: + return None + extras = f"[{extras_body}]" + remainder = tail + else: + extras = "" + # No extras: the name runs until the first operator character. + match = re.match(r"[^<>=!~]*", head) + name_end = match.end() if match else 0 + head, remainder = head[:name_end], head[name_end:] + + name = head.strip() + if not name: + return None + + clauses = tuple((op, version.strip()) for op, version in _CLAUSE_RE.findall(remainder)) + return ParsedRequirement(name=name, extras=extras, clauses=clauses, marker=marker) + + +def _render(parsed: ParsedRequirement, clauses: tuple[tuple[str, str], ...]) -> str: + body = ",".join(f"{op}{version}" for op, version in clauses) + return f"{parsed.name}{parsed.extras}{body}{parsed.marker}" + + +def _matches_wildcard(version: str, pattern: str) -> bool: + """Does ``version`` fall under a PEP 440 wildcard like ``1.4.*``?""" + prefix = version_key(pattern[: -len(_WILDCARD)]) + release = version_key(version) + return len(release) >= len(prefix) and release[: len(prefix)] == prefix + + +def _compatible_band(version: str) -> str | None: + """The wildcard a ``~=`` clause implies, or ``None`` if it has none. + + ``~=1.4`` means ``>=1.4, ==1.*``; ``~=1.4.2`` means ``>=1.4.2, ==1.4.*``. + A single-segment ``~=1`` is invalid PEP 440 and has no band to derive. + """ + parts = version.split(".") + if len(parts) < 2: + return None + return ".".join(parts[:-1]) + _WILDCARD + + +def _allows(op: str, version: str, latest: str) -> bool: + """Would this clause still be satisfied by ``latest``? + + Floors (``==``/``===``/``>=``) are always "allowed": the rewrite moves them + *to* ``latest``, so they can't exclude it. Everything else either carries a + ceiling of its own or is left verbatim, and has to be checked. + """ + if op in _FLOOR_OPS and not version.endswith(_WILDCARD): + return True + if version.endswith(_WILDCARD): + # ``==1.0.*`` allows anything under the prefix; ``!=1.0.*`` allows + # anything outside it. + inside = _matches_wildcard(latest, version) + return inside if op in ("==", "===") else not inside + if op == "~=": + band = _compatible_band(version) + return band is not None and _matches_wildcard(latest, band) + cmp = _compare(latest, version) + if op == "<": + return cmp < 0 + if op == "<=": + return cmp <= 0 + if op == "!=": + return cmp != 0 + # ``>`` — already satisfied by anything newer than the pin. + return True + + +def _excluded_by(parsed: ParsedRequirement, latest: str) -> str | None: + """Return the clause that rules ``latest`` out, or ``None`` if it's allowed.""" + for op, version in parsed.clauses: + if not _allows(op, version, latest): + return f"{op}{version}" + return None + + +def _bump(op: str, version: str, latest: str) -> str: + """The version this clause should carry after the update. + + Only a plain floor moves. A wildcard band (``==1.0.*``, ``!=1.0.*``) that + already allows ``latest`` is left exactly as written — narrowing it to a + single release would be a policy change, not a version bump. ``~=`` does + move, but only because ``_allows`` has already established that ``latest`` + is inside its compatible band. + """ + if version.endswith(_WILDCARD): + return version + if op in _FLOOR_OPS or op == "~=": + return latest + return version + + +def rewrite_requirement(spec: str, latest: str, *, loosen: bool = False) -> RewriteResult: + """Point ``spec`` at ``latest``, preserving its pin style unless ``loosen``. + + Returns a ``RewriteResult`` whose ``spec`` is ``None`` when the requirement + already asks for ``latest``, when it carries no floor to raise, or when an + upper bound excludes ``latest`` (the last carries a ``reason``). + """ + parsed = parse_requirement(spec) + if parsed is None: + return RewriteResult(None) + + if loosen or not parsed.clauses: + # Nothing to preserve — an unconstrained dependency has no pin style, + # so it gets the tool's default floor. Extras and markers survive. + new = f"{parsed.name}{parsed.extras}>={latest}{parsed.marker}" + return RewriteResult(None) if new == spec.strip() else RewriteResult(new) + + blocker = _excluded_by(parsed, latest) + if blocker is not None: + return RewriteResult(None, reason=f"{latest} excluded by {blocker}") + + bumped = tuple((op, _bump(op, version, latest)) for op, version in parsed.clauses) + if bumped == parsed.clauses: + # Nothing here names a floor to raise — a bare ``>``/``<``/``!=``, or a + # wildcard band that already covers ``latest``. Rewriting either would + # invent a constraint the author didn't ask for. + return RewriteResult(None) + + new = _render(parsed, bumped) + return RewriteResult(None) if new == spec.strip() else RewriteResult(new) diff --git a/framework/cli/tests/conftest.py b/framework/cli/tests/conftest.py index 68b9e8b6..71c0baee 100644 --- a/framework/cli/tests/conftest.py +++ b/framework/cli/tests/conftest.py @@ -1,4 +1,4 @@ -"""Fixtures shared by the module-asset tests in this directory. +"""Fixtures shared by the tests in this directory. These test files have no ``__init__.py`` (test basenames are globally unique instead), so a plain helper import across files is not available — a fixture is @@ -10,10 +10,47 @@ import subprocess import sys from pathlib import Path +from urllib.error import HTTPError import pytest +@pytest.fixture +def fake_pypi(): + """Return a factory building a `package-update` fetcher stub. + + Takes `{dist_name: latest_version}` and answers only for those names; + anything else raises the 404 the real fetcher would see. Keeps the CLI + tests off the network. + """ + + def factory(versions: dict[str, str]): + def fetcher(url: str) -> dict: + name = url.rsplit("/json", 1)[0].rsplit("/", 1)[1] + if name not in versions: + raise HTTPError(url, 404, "not found", {}, None) + latest = versions[name] + return {"info": {"version": latest}, "releases": {latest: [{"yanked": False}]}} + + return fetcher + + return factory + + +@pytest.fixture +def write_pyproject(): + """Return a factory writing a minimal pyproject with the given deps.""" + + def factory(path: Path, *deps: str) -> None: + body = "".join(f' "{d}",\n' for d in deps) + path.write_text( + f'[project]\nname = "x"\nversion = "0"\ndependencies = [\n{body}]\n', + encoding="utf-8", + ) + + return factory + + @pytest.fixture def make_importable_module(tmp_path: Path): """Return a factory creating a real importable package bound to a ModuleBase. diff --git a/framework/cli/tests/test_cli_package_update.py b/framework/cli/tests/test_cli_package_update.py index d64e830d..e3ef29cd 100644 --- a/framework/cli/tests/test_cli_package_update.py +++ b/framework/cli/tests/test_cli_package_update.py @@ -4,7 +4,6 @@ import importlib from pathlib import Path -from urllib.error import HTTPError import click import pytest @@ -13,29 +12,13 @@ from typer.testing import CliRunner -def _fake_pypi(versions: dict[str, str]): - """Build a fetcher stub that returns canned responses keyed by package name.""" - - def fetcher(url: str) -> dict: - name = url.rsplit("/json", 1)[0].rsplit("/", 1)[1] - if name not in versions: - raise HTTPError(url, 404, "not found", {}, None) - latest = versions[name] - return { - "info": {"version": latest}, - "releases": {latest: [{"yanked": False}]}, - } - - return fetcher - - -def test_updates_simple_module_deps(tmp_path: Path) -> None: +def test_updates_simple_module_deps(tmp_path: Path, fake_pypi) -> None: pyproject = tmp_path / "pyproject.toml" pyproject.write_text( '[project]\nname = "x"\nversion = "0.1.0"\n' "dependencies = [\n" ' "simple_module_core>=0.1",\n' - ' "simple_module_db>=0.1,<1.0",\n' + ' "simple_module_db>=0.1,<3.0",\n' ' "fastapi>=0.110",\n' "]\n", encoding="utf-8", @@ -45,16 +28,16 @@ def test_updates_simple_module_deps(tmp_path: Path) -> None: path=pyproject, dry_run=False, include_pre=False, - fetcher=_fake_pypi({"simple_module_core": "1.2.3", "simple_module_db": "2.0.0"}), + fetcher=fake_pypi({"simple_module_core": "1.2.3", "simple_module_db": "2.0.0"}), ) out = pyproject.read_text(encoding="utf-8") assert "simple_module_core>=1.2.3" in out - assert "simple_module_db>=2.0.0" in out + assert "simple_module_db>=2.0.0,<3.0" in out # upper bound preserved assert "fastapi>=0.110" in out # untouched -def test_walks_workspace_members(tmp_path: Path) -> None: +def test_walks_workspace_members(tmp_path: Path, fake_pypi) -> None: root = tmp_path / "pyproject.toml" root.write_text( '[project]\nname = "root"\nversion = "0"\ndependencies = ["simple_module_core>=0.1"]\n' @@ -72,14 +55,14 @@ def test_walks_workspace_members(tmp_path: Path) -> None: path=tmp_path, dry_run=False, include_pre=False, - fetcher=_fake_pypi({"simple_module_core": "1.0.0", "simple_module_db": "2.0.0"}), + fetcher=fake_pypi({"simple_module_core": "1.0.0", "simple_module_db": "2.0.0"}), ) assert "simple_module_core>=1.0.0" in root.read_text(encoding="utf-8") assert "simple_module_db>=2.0.0" in (member / "pyproject.toml").read_text(encoding="utf-8") -def test_skips_workspace_source_deps(tmp_path: Path) -> None: +def test_skips_workspace_source_deps(tmp_path: Path, fake_pypi) -> None: pyproject = tmp_path / "pyproject.toml" pyproject.write_text( '[project]\nname = "x"\nversion = "0"\n' @@ -93,7 +76,7 @@ def test_skips_workspace_source_deps(tmp_path: Path) -> None: path=pyproject, dry_run=False, include_pre=False, - fetcher=_fake_pypi({"simple_module_core": "9.9.9", "simple_module_db": "2.0.0"}), + fetcher=fake_pypi({"simple_module_core": "9.9.9", "simple_module_db": "2.0.0"}), ) out = pyproject.read_text(encoding="utf-8") @@ -102,7 +85,7 @@ def test_skips_workspace_source_deps(tmp_path: Path) -> None: assert "simple_module_db>=2.0.0" in out -def test_dry_run_does_not_write(tmp_path: Path) -> None: +def test_dry_run_does_not_write(tmp_path: Path, fake_pypi) -> None: pyproject = tmp_path / "pyproject.toml" original = '[project]\nname = "x"\nversion = "0"\ndependencies = ["simple_module_core>=0.1"]\n' pyproject.write_text(original, encoding="utf-8") @@ -111,13 +94,13 @@ def test_dry_run_does_not_write(tmp_path: Path) -> None: path=pyproject, dry_run=True, include_pre=False, - fetcher=_fake_pypi({"simple_module_core": "1.0.0"}), + fetcher=fake_pypi({"simple_module_core": "1.0.0"}), ) assert pyproject.read_text(encoding="utf-8") == original -def test_skips_unknown_pypi_package(tmp_path: Path) -> None: +def test_skips_unknown_pypi_package(tmp_path: Path, fake_pypi) -> None: pyproject = tmp_path / "pyproject.toml" pyproject.write_text( '[project]\nname = "x"\nversion = "0"\ndependencies = ["simple_module_unknown>=0.1"]\n', @@ -128,7 +111,7 @@ def test_skips_unknown_pypi_package(tmp_path: Path) -> None: path=pyproject, dry_run=False, include_pre=False, - fetcher=_fake_pypi({}), + fetcher=fake_pypi({}), ) assert "simple_module_unknown>=0.1" in pyproject.read_text(encoding="utf-8") @@ -179,13 +162,13 @@ def _exit_exception_types() -> tuple[type[BaseException], ...]: return tuple(found) -def test_missing_pyproject_exits_nonzero(tmp_path: Path) -> None: +def test_missing_pyproject_exits_nonzero(tmp_path: Path, fake_pypi) -> None: with pytest.raises(_exit_exception_types()) as exc: pu.run_update( path=tmp_path, dry_run=False, include_pre=False, - fetcher=_fake_pypi({}), + fetcher=fake_pypi({}), ) assert getattr(exc.value, "exit_code", getattr(exc.value, "code", None)) == 1 diff --git a/framework/cli/tests/test_cli_package_update_pins.py b/framework/cli/tests/test_cli_package_update_pins.py new file mode 100644 index 00000000..c76be546 --- /dev/null +++ b/framework/cli/tests/test_cli_package_update_pins.py @@ -0,0 +1,217 @@ +"""Constraint rewriting for `smpy package-update` — GH #284. + +Split from `test_cli_package_update.py` (which covers file walking, workspace +members and the PyPI lookup) to keep both under the 300-line cap. + +The rule under test throughout: bumping a dependency changes its *version*, +not its *pin style*, and never silently drops a bound that excludes the +release being installed. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from simple_module_cli import package_update as pu + + +def test_exact_pins_stay_exact(tmp_path: Path, fake_pypi, write_pyproject) -> None: + """The #284 regression: `package-update` bumped versions *and* pin style.""" + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core==0.0.32", "simple_module_db===0.0.32") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "0.0.33", "simple_module_db": "0.0.33"}), + ) + + out = pyproject.read_text(encoding="utf-8") + assert "simple_module_core==0.0.33" in out + assert "simple_module_db===0.0.33" in out + assert ">=" not in out + + +def test_compatible_release_pin_stays_compatible( + tmp_path: Path, fake_pypi, write_pyproject +) -> None: + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core~=0.0.32") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "0.0.33"}), + ) + + assert "simple_module_core~=0.0.33" in pyproject.read_text(encoding="utf-8") + + +def test_loosen_flag_restores_the_old_rewrite(tmp_path: Path, fake_pypi, write_pyproject) -> None: + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core==0.0.32") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + loosen=True, + fetcher=fake_pypi({"simple_module_core": "0.0.33"}), + ) + + assert "simple_module_core>=0.0.33" in pyproject.read_text(encoding="utf-8") + + +def test_unconstrained_dep_gets_a_lower_bound(tmp_path: Path, fake_pypi, write_pyproject) -> None: + """Nothing to preserve, so the tool's default style applies.""" + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "0.0.33"}), + ) + + assert "simple_module_core>=0.0.33" in pyproject.read_text(encoding="utf-8") + + +def test_upper_bound_excluding_latest_skips_rather_than_drops( + tmp_path: Path, capsys: pytest.CaptureFixture[str], fake_pypi, write_pyproject +) -> None: + """The old rewrite silently deleted the ceiling; now it's reported.""" + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core>=0.1,<1.0") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "2.0.0"}), + ) + + assert "simple_module_core>=0.1,<1.0" in pyproject.read_text(encoding="utf-8") + assert "excluded by <1.0" in capsys.readouterr().out + + +def test_extras_and_markers_survive_the_rewrite(tmp_path: Path, fake_pypi, write_pyproject) -> None: + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core[redis]==0.0.32; python_version >= '3.12'") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "0.0.33"}), + ) + + out = pyproject.read_text(encoding="utf-8") + assert "simple_module_core[redis]==0.0.33" in out + assert "python_version >= '3.12'" in out + + +class TestWildcardsAndImplicitCeilings: + """Wildcards and `~=` carry ceilings `version_key` can't see. + + `version_key` maps `*` to 0, so `!=1.0.*` compared numerically reads as + `!=1.0.0` and never fires. That produced `>=1.0.5,!=1.0.*` — a specifier + nothing can satisfy, which fails the `uv sync` the tool tells you to run. + """ + + def test_wildcard_exclusion_is_honoured( + self, tmp_path: Path, fake_pypi, write_pyproject + ) -> None: + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core>=1.0,!=1.0.*") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "1.0.5"}), + ) + + assert "simple_module_core>=1.0,!=1.0.*" in pyproject.read_text(encoding="utf-8") + + def test_wildcard_that_does_not_cover_latest_still_bumps( + self, tmp_path: Path, fake_pypi, write_pyproject + ) -> None: + """`!=1.1.*` has nothing to say about 1.0.5, so the floor moves.""" + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core>=1.0,!=1.1.*") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "1.0.5"}), + ) + + assert "simple_module_core>=1.0.5,!=1.1.*" in pyproject.read_text(encoding="utf-8") + + def test_compatible_release_outside_its_band_is_reported( + self, tmp_path: Path, fake_pypi, write_pyproject + ) -> None: + """`~=1.4` means `>=1.4, ==1.*`, so 2.0.0 is out of range.""" + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core~=1.4") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "2.0.0"}), + ) + + assert "simple_module_core~=1.4" in pyproject.read_text(encoding="utf-8") + assert "~=2.0.0" not in pyproject.read_text(encoding="utf-8") + + def test_compatible_release_inside_its_band_moves( + self, tmp_path: Path, fake_pypi, write_pyproject + ) -> None: + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core~=1.4.2") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "1.4.9"}), + ) + + assert "simple_module_core~=1.4.9" in pyproject.read_text(encoding="utf-8") + + def test_wildcard_pin_covering_latest_is_left_alone( + self, tmp_path: Path, fake_pypi, write_pyproject + ) -> None: + """`==1.0.*` already allows 1.0.5; narrowing it would change policy.""" + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core==1.0.*") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "1.0.5"}), + ) + + assert "simple_module_core==1.0.*" in pyproject.read_text(encoding="utf-8") + + def test_wildcard_pin_not_covering_latest_is_reported( + self, tmp_path: Path, fake_pypi, write_pyproject + ) -> None: + pyproject = tmp_path / "pyproject.toml" + write_pyproject(pyproject, "simple_module_core==1.0.*") + + pu.run_update( + path=pyproject, + dry_run=False, + include_pre=False, + fetcher=fake_pypi({"simple_module_core": "1.1.0"}), + ) + + assert "simple_module_core==1.0.*" in pyproject.read_text(encoding="utf-8") diff --git a/framework/core/simple_module_core/settings_base.py b/framework/core/simple_module_core/settings_base.py new file mode 100644 index 00000000..646b54f4 --- /dev/null +++ b/framework/core/simple_module_core/settings_base.py @@ -0,0 +1,47 @@ +"""Base class for DB-backed settings that must not read the environment. + +Module and host settings are hydrated from the settings store at boot and +hot-swapped through ``settings.reload.apply_changes_and_reload``. The DB is +meant to be the only source of truth for them. + +Subclassing ``BaseSettings`` and simply omitting ``env_prefix`` does **not** +achieve that. pydantic-settings still installs its env source; without a +prefix it resolves each field from the bare, case-insensitive *field name*. +So a class with a ``password`` field reads ``$password``, one with a +``backend`` field reads ``$backend``, and one with a ``base_url`` field reads +``$base_url`` — names generic enough that an unrelated component setting them +silently reconfigures the app, and the DB is the source of truth only when +nobody happens to have them in the environment. That was GH #283, reported +against ``background_tasks`` but true of every bundled settings class. + +``DbBackedSettings`` drops the env, dotenv and secrets sources, leaving only +values passed to the constructor. Settings that genuinely must be readable +before any DB row exists — ``BackgroundTasksSettings``, whose broker URL a +worker process needs and which the production validator rejects at the +localhost default — subclass ``BaseSettings`` directly *and* declare an +explicit ``env_prefix``, so the names they read are namespaced and documented. +""" + +from __future__ import annotations + +from pydantic_settings import BaseSettings, PydanticBaseSettingsSource, SettingsConfigDict + +__all__ = ["DbBackedSettings"] + + +class DbBackedSettings(BaseSettings): + """A ``BaseSettings`` whose values come from the constructor and the DB only.""" + + model_config = SettingsConfigDict(extra="ignore") + + @classmethod + def settings_customise_sources( + cls, + settings_cls: type[BaseSettings], + init_settings: PydanticBaseSettingsSource, + env_settings: PydanticBaseSettingsSource, + dotenv_settings: PydanticBaseSettingsSource, + file_secret_settings: PydanticBaseSettingsSource, + ) -> tuple[PydanticBaseSettingsSource, ...]: + """Init args only — no env, no ``.env``, no Docker secrets files.""" + return (init_settings,) diff --git a/framework/core/tests/test_settings_base.py b/framework/core/tests/test_settings_base.py new file mode 100644 index 00000000..77a5aaeb --- /dev/null +++ b/framework/core/tests/test_settings_base.py @@ -0,0 +1,75 @@ +"""``DbBackedSettings`` must not read the environment at all — GH #283. + +The bug this guards was reported against ``background_tasks``, but the cause +was shared by every bundled settings class: subclassing ``BaseSettings`` +without an ``env_prefix`` doesn't disable env reads, it un-namespaces them. +Fields then resolve from bare names — ``password``, ``backend``, ``base_url`` +— which are common enough in a container that an unrelated component setting +one silently reconfigures the app. +""" + +from __future__ import annotations + +import pytest +from pydantic_settings import BaseSettings +from simple_module_core.settings_base import DbBackedSettings + + +class _Example(DbBackedSettings): + password: str = "" + backend: str = "filesystem" + base_url: str = "http://localhost:8000" + enabled: bool = False + retries: int = 3 + + +class _LeakyExample(BaseSettings): + """The old shape, kept as the contrast the test is asserting against.""" + + password: str = "" + + +def test_bare_field_names_are_not_read_from_env(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("password", "hunter2") + monkeypatch.setenv("backend", "s3") + monkeypatch.setenv("base_url", "http://evil.example.com") + monkeypatch.setenv("enabled", "true") + monkeypatch.setenv("retries", "99") + + settings = _Example() + + assert settings.password == "" + assert settings.backend == "filesystem" + assert settings.base_url == "http://localhost:8000" + assert settings.enabled is False + assert settings.retries == 3 + + +def test_plain_base_settings_would_have_read_them(monkeypatch: pytest.MonkeyPatch) -> None: + """Pin the behaviour being defended against, so the test can't quietly pass.""" + monkeypatch.setenv("password", "hunter2") + + assert _LeakyExample().password == "hunter2" + + +def test_uppercase_names_are_ignored_too(monkeypatch: pytest.MonkeyPatch) -> None: + """pydantic-settings matches env case-insensitively by default.""" + monkeypatch.setenv("PASSWORD", "hunter2") + + assert _Example().password == "" + + +def test_constructor_values_still_win() -> None: + """Hydration from the DB goes through the constructor, so it must work.""" + settings = _Example(password="from-db", retries=7) + + assert settings.password == "from-db" + assert settings.retries == 7 + + +def test_dotenv_file_is_not_read(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: + """The host loads `.env` into `os.environ`; these classes still ignore it.""" + monkeypatch.chdir(tmp_path) + (tmp_path / ".env").write_text("password=from-dotenv\n", encoding="utf-8") + + assert _Example().password == "" diff --git a/framework/hosting/tests/test_auth_provider_setting.py b/framework/hosting/tests/test_auth_provider_setting.py index 348623ff..800f7dc2 100644 --- a/framework/hosting/tests/test_auth_provider_setting.py +++ b/framework/hosting/tests/test_auth_provider_setting.py @@ -60,3 +60,28 @@ def test_agrees_with_resolve_auth_provider(self, raw: str, monkeypatch): """Host and out-of-process readers must land on the same name.""" monkeypatch.setenv("SM_AUTH_PROVIDER", raw) assert _settings().auth_provider == resolve_auth_provider() + + +class TestShimKeepsItsEnvironment: + """``Settings`` mixes ``HostSettings`` with ``BootstrapSettings``. + + Both halves namespace their env reads under ``SM_``, and the bootstrap + half's must keep working: ``SM_DATABASE_URL`` and friends are read by + design, before any database exists to read them from. The bare, + unprefixed names must not resolve on either half (GH #283). + """ + + def test_bootstrap_fields_still_read_sm_prefixed_env(self, monkeypatch) -> None: + monkeypatch.setenv("SM_AUTH_PROVIDER", "keycloak") + monkeypatch.setenv("SM_DATABASE_URL", "sqlite+aiosqlite:///:memory:") + + settings = Settings(environment="testing", secret_key="test-secret-key") + + assert settings.auth_provider == "keycloak" + assert settings.database_url == "sqlite+aiosqlite:///:memory:" + + def test_host_fields_stay_namespaced_on_the_shim(self, monkeypatch) -> None: + """The bare name never works, even where the shim restores env reads.""" + monkeypatch.setenv("maintenance_mode", "true") + + assert _settings().maintenance_mode is False diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index cb7314a2..69c0bcf1 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -15,7 +15,9 @@ ``SM_BG_TASKS_BROKER_URL`` and ``SM_BG_TASKS_RESULT_BACKEND`` still work and take precedence over ``SM_REDIS_URL`` — a deployment that set them meant them — -but log a deprecation warning. +but log a deprecation warning. They are answered by pydantic's own env source +now that ``env_prefix`` is set (GH #283), which is also what stops every other +field being readable from its bare, unprefixed name. The other env read is ``SM_ENVIRONMENT``, consulted by the ``@model_validator`` to refuse a localhost broker in production — that's a @@ -35,7 +37,6 @@ from pydantic import Field, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict -from simple_module_core.dotenv import env_bool from simple_module_core.environments import NON_PROD_ENVIRONMENTS from background_tasks.constants import ( @@ -56,48 +57,41 @@ logger = logging.getLogger(__name__) -def _redis_from_env(legacy_var: str, default: str) -> str: - """Resolve a Celery URL from env: legacy var → ``SM_REDIS_URL`` → default. +def _redis_from_env(default: str) -> str: + """``SM_REDIS_URL`` → the module default. - The legacy var is checked first because a deployment that set both meant - the specific one. It warns rather than failing: ``smpy_gis``, ``smpy_saas``, - ``laco_wiki_python`` and the ``nodes-k8s`` manifests all set these, and - breaking them on upgrade buys nothing. + ``env_prefix`` means pydantic's env source has already answered + ``SM_BG_TASKS_BROKER_URL`` / ``SM_BG_TASKS_RESULT_BACKEND`` by the time a + default is needed, so this factory is only the ``SM_REDIS_URL`` half — the + legacy names keep working, and keep winning, through that source instead. """ - legacy = os.environ.get(legacy_var) - if legacy: - logger.warning( - "%s is deprecated; use %s instead (one URL seeds both the broker " - "and the result backend).", - legacy_var, - ENV_REDIS_URL, - ) - return legacy return os.environ.get(ENV_REDIS_URL) or default class BackgroundTasksSettings(BaseSettings): """Configuration for the Celery + Redis task runner.""" - model_config = SettingsConfigDict(extra="ignore") + # Without ``env_prefix`` pydantic-settings resolves each field from its + # bare, case-insensitive name — so a container setting ``broker_url`` or + # ``retention_days`` for any other purpose silently reconfigured Celery, + # and the documented ``SM_BG_TASKS_*`` names did nothing (GH #283). + model_config = SettingsConfigDict(extra="ignore", env_prefix=ENV_PREFIX) broker_url: str = Field( - default_factory=lambda: _redis_from_env(f"{ENV_PREFIX}BROKER_URL", DEFAULT_BROKER_URL), + default_factory=lambda: _redis_from_env(DEFAULT_BROKER_URL), json_schema_extra=_CELERY_RESTART, ) result_backend: str = Field( - default_factory=lambda: _redis_from_env( - f"{ENV_PREFIX}RESULT_BACKEND", DEFAULT_RESULT_BACKEND - ), + default_factory=lambda: _redis_from_env(DEFAULT_RESULT_BACKEND), json_schema_extra=_CELERY_RESTART, ) task_default_queue: str = Field(default=DEFAULT_QUEUE, json_schema_extra=_CELERY_RESTART) - # Run tasks synchronously inside the calling process. Read at - # module-import time so tests can flip it on via ``SM_BG_TASKS_*`` - # without going through DB-backed hydration (which never fires for - # suites that don't use the FastAPI lifespan). - task_always_eager: bool = env_bool("SM_BG_TASKS_TASK_ALWAYS_EAGER") + # Run tasks synchronously inside the calling process. Tests flip it on via + # ``SM_BG_TASKS_TASK_ALWAYS_EAGER`` or by passing it explicitly, either of + # which works without DB-backed hydration (which never fires for suites + # that don't use the FastAPI lifespan). + task_always_eager: bool = False task_eager_propagates: bool = True # A task that has been ``running`` longer than this without a heartbeat is @@ -121,6 +115,30 @@ class BackgroundTasksSettings(BaseSettings): description="Configured retry ceiling; individual tasks define their own policies (0-100).", ) + @model_validator(mode="after") + def _warn_on_legacy_redis_vars(self) -> BackgroundTasksSettings: + """Nudge deployments off the per-field names onto ``SM_REDIS_URL``. + + Warns rather than failing: ``smpy_gis``, ``smpy_saas``, + ``laco_wiki_python`` and the ``nodes-k8s`` manifests all set these, and + breaking them on upgrade buys nothing. It lives here rather than in the + default factory because ``env_prefix`` means the env source answers + these names before any default is consulted, so the factory never sees + them. + """ + for field, legacy_var in ( + ("broker_url", f"{ENV_PREFIX}BROKER_URL"), + ("result_backend", f"{ENV_PREFIX}RESULT_BACKEND"), + ): + if os.environ.get(legacy_var) == getattr(self, field): + logger.warning( + "%s is deprecated; use %s instead (one URL seeds both the " + "broker and the result backend).", + legacy_var, + ENV_REDIS_URL, + ) + return self + @model_validator(mode="after") def _forbid_localhost_broker_in_production(self) -> BackgroundTasksSettings: """Fail boot if production is still pointed at the dev default broker. @@ -139,8 +157,12 @@ def _forbid_localhost_broker_in_production(self) -> BackgroundTasksSettings: bad.append("result_backend") if bad: names = ", ".join(bad) + env_names = ", ".join(f"{ENV_PREFIX}{name.upper()}" for name in bad) raise ValueError( f"{names} must not point at localhost when SM_ENVIRONMENT={env!r}. " - "Set these to the Redis service host (e.g. redis://redis:6379/0)." + f"Set {env_names} on the container to the Redis service host " + "(e.g. redis://redis:6379/0) — these are read before the " + "DB-backed settings exist, so an environment variable is the " + "only thing that can satisfy this at boot." ) return self diff --git a/modules/background_tasks/tests/test_bg_settings_env.py b/modules/background_tasks/tests/test_bg_settings_env.py index 702c3095..309041f7 100644 --- a/modules/background_tasks/tests/test_bg_settings_env.py +++ b/modules/background_tasks/tests/test_bg_settings_env.py @@ -1,9 +1,10 @@ -"""Broker/result-backend env plumbing for BackgroundTasksSettings. +"""Env plumbing for BackgroundTasksSettings. -These two fields are read from the environment at construction because they -have to be right *before* the DB-backed settings exist: a container boots in -production, where the localhost defaults are rejected, and a Celery worker -process never sees ``app.state`` at all. Everything else is DB-backed. +Every field reads from a ``SM_BG_TASKS_``-prefixed variable at construction. +That matters most for the broker and result backend, which have to be right +*before* the DB-backed settings exist: a container boots in production, where +the localhost defaults are rejected, and a Celery worker process never sees +``app.state`` at all. A DB value still wins once hydration runs. """ from __future__ import annotations @@ -62,3 +63,56 @@ def test_localhost_still_rejected_in_production(monkeypatch: pytest.MonkeyPatch) with pytest.raises(ValueError, match="must not point at localhost"): BackgroundTasksSettings() + + +def test_validator_names_the_env_vars_that_fix_it(monkeypatch: pytest.MonkeyPatch) -> None: + """The error is most people's first encounter with this — GH #283.""" + monkeypatch.setenv("SM_ENVIRONMENT", "production") + monkeypatch.delenv("SM_BG_TASKS_BROKER_URL", raising=False) + monkeypatch.delenv("SM_BG_TASKS_RESULT_BACKEND", raising=False) + + with pytest.raises(ValueError, match="SM_BG_TASKS_BROKER_URL"): + BackgroundTasksSettings() + + +def test_unprefixed_env_vars_are_ignored(monkeypatch: pytest.MonkeyPatch) -> None: + """GH #283: without ``env_prefix`` these bare names silently won. + + ``broker_url`` and ``result_backend`` are generic enough that another + component setting them for its own purposes would have reconfigured + Celery, and the DB was only the source of truth when nobody happened to + have those names in the environment. + """ + monkeypatch.delenv("SM_BG_TASKS_BROKER_URL", raising=False) + monkeypatch.delenv("SM_BG_TASKS_RESULT_BACKEND", raising=False) + monkeypatch.setenv("broker_url", "redis://someone-elses-service:6379/0") + monkeypatch.setenv("result_backend", "redis://someone-elses-service:6379/1") + monkeypatch.setenv("retention_days", "999") + + settings = BackgroundTasksSettings() + + assert settings.broker_url == DEFAULT_BROKER_URL + assert settings.result_backend == DEFAULT_RESULT_BACKEND + assert settings.retention_days != 999 + + +def test_every_field_reads_its_prefixed_name(monkeypatch: pytest.MonkeyPatch) -> None: + """Not just the two broker URLs — the class has a single rule now.""" + monkeypatch.setenv("SM_BG_TASKS_TASK_DEFAULT_QUEUE", "reports") + monkeypatch.setenv("SM_BG_TASKS_RETENTION_DAYS", "45") + monkeypatch.setenv("SM_BG_TASKS_MAX_RETRIES", "7") + monkeypatch.setenv("SM_BG_TASKS_TASK_ALWAYS_EAGER", "true") + + settings = BackgroundTasksSettings() + + assert settings.task_default_queue == "reports" + assert settings.retention_days == 45 + assert settings.max_retries == 7 + assert settings.task_always_eager is True + + +def test_explicit_kwargs_beat_the_environment(monkeypatch: pytest.MonkeyPatch) -> None: + """The test plugin and run_worker recipes pass fields directly.""" + monkeypatch.setenv("SM_BG_TASKS_TASK_ALWAYS_EAGER", "false") + + assert BackgroundTasksSettings(task_always_eager=True).task_always_eager is True diff --git a/modules/branding/branding/components/FooterLinksField.tsx b/modules/branding/branding/components/FooterLinksField.tsx new file mode 100644 index 00000000..dd1e423c --- /dev/null +++ b/modules/branding/branding/components/FooterLinksField.tsx @@ -0,0 +1,103 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { Label } from '@simple-module-py/ui/components/ui/label'; + +/** Mirrors `MAX_FOOTER_LINKS` in `branding/constants.py`. */ +export const MAX_FOOTER_LINKS = 6; +/** Mirrors `MAX_FOOTER_LINK_LABEL_LEN`. */ +export const MAX_FOOTER_LABEL = 40; +/** Mirrors `MAX_FOOTER_LINK_HREF_LEN`. */ +export const MAX_FOOTER_HREF = 500; + +export interface FooterLink { + label: string; + href: string; +} + +interface FooterLinksFieldProps { + links: FooterLink[]; + onChange: (next: FooterLink[]) => void; + disabled: boolean; +} + +/** + * Editor for the links in the site footer. + * + * An empty list means "show the framework's own links", which is why removing + * the last row is allowed and reads as a reset rather than an empty footer. + * The server enforces the same caps and an href allow-list; the `maxLength` + * attributes here just stop the round trip. + */ +export function FooterLinksField({ links, onChange, disabled }: FooterLinksFieldProps) { + const { t } = useT(); + + const update = (index: number, patch: Partial) => + onChange(links.map((link, i) => (i === index ? { ...link, ...patch } : link))); + + const remove = (index: number) => onChange(links.filter((_, i) => i !== index)); + + const add = () => onChange([...links, { label: '', href: '' }]); + + return ( +
+ + + {links.length === 0 ? ( +

+ {t(keys.branding.manage.footer_links_empty)} +

+ ) : ( +
    + {links.map((link, index) => ( + // Index-keyed on purpose: rows have no id, and label/href are the + // very fields being edited, so a value-derived key would remount + // the input on every keystroke and lose focus. + // biome-ignore lint/suspicious/noArrayIndexKey: see above +
  • + update(index, { label: e.target.value })} + className="w-40" + /> + update(index, { href: e.target.value })} + className="min-w-60 flex-1 font-mono text-xs" + /> + +
    • + ))} +
+ )} + + + +

{t(keys.branding.manage.footer_links_help)}

+
+ ); +} diff --git a/modules/branding/branding/constants.py b/modules/branding/branding/constants.py index 3ee10a3b..25322ed0 100644 --- a/modules/branding/branding/constants.py +++ b/modules/branding/branding/constants.py @@ -115,3 +115,67 @@ def clean_app_name(value: str) -> str: if any(ord(ch) < 0x20 for ch in cleaned): raise ValueError("app_name must not contain control characters") return cleaned + + +# ── Footer links ─────────────────────────────────────────────────────── +# An empty list means "use the framework's own links" (`BRAND_FOOTER_LINKS` +# in @simple-module-py/ui), so an existing deployment that never sets these +# keeps the footer it has today. +MAX_FOOTER_LINKS: Final = 6 +MAX_FOOTER_LINK_LABEL_LEN: Final = 40 +MAX_FOOTER_LINK_HREF_LEN: Final = 500 + +#: Schemes an admin-supplied footer href may use, plus site-relative paths. +#: +#: This is a real allow-list, not tidiness: the href is rendered straight into +#: an ```` on every page of the site, signed-in or not, so +#: ``javascript:`` (and ``data:``) would turn the branding screen into a stored +#: XSS sink for anyone holding ``branding.manage``. +FOOTER_LINK_SCHEMES: Final = ("http://", "https://", "mailto:") +FOOTER_LINK_HREF_ERROR: Final = ( + "footer link href must start with " + ", ".join(FOOTER_LINK_SCHEMES) + " or /" +) +#: Backslashes are rejected outright. Browsers normalise ``\`` to ``/`` in the +#: authority position of a special-scheme URL, so ``/\evil.example.com`` reads +#: as a site-relative path but navigates to ``https://evil.example.com`` — the +#: exact bypass the ``//host`` rule below exists to close. No legitimate footer +#: target needs a raw backslash; ``%5C`` still works for one in a path. +FOOTER_LINK_BACKSLASH_ERROR: Final = "footer link href must not contain a backslash" + + +def clean_footer_label(value: str) -> str: + """Normalise + validate a footer link's visible text.""" + cleaned = value.strip() + if not cleaned: + raise ValueError("footer link label must not be blank") + if len(cleaned) > MAX_FOOTER_LINK_LABEL_LEN: + raise ValueError( + f"footer link label must be at most {MAX_FOOTER_LINK_LABEL_LEN} characters" + ) + if any(ord(ch) < 0x20 for ch in cleaned): + raise ValueError("footer link label must not contain control characters") + return cleaned + + +def clean_footer_href(value: str) -> str: + """Normalise + validate a footer link's target. + + Scheme-relative ``//host`` is rejected along with everything else outside + the allow-list: it reads as a relative path but navigates off-site, so + allowing it would make the ``/`` rule mean something other than "this site". + Backslashes go with it — a browser reads ``/\\host`` the same way. + """ + cleaned = value.strip() + if not cleaned: + raise ValueError("footer link href must not be blank") + if len(cleaned) > MAX_FOOTER_LINK_HREF_LEN: + raise ValueError(f"footer link href must be at most {MAX_FOOTER_LINK_HREF_LEN} characters") + if any(ord(ch) < 0x20 for ch in cleaned): + raise ValueError("footer link href must not contain control characters") + if "\\" in cleaned: + raise ValueError(FOOTER_LINK_BACKSLASH_ERROR) + lowered = cleaned.lower() + relative = cleaned.startswith("/") and not cleaned.startswith("//") + if not relative and not lowered.startswith(FOOTER_LINK_SCHEMES): + raise ValueError(FOOTER_LINK_HREF_ERROR) + return cleaned diff --git a/modules/branding/branding/contracts/schemas.py b/modules/branding/branding/contracts/schemas.py index b8047c80..415f28c3 100644 --- a/modules/branding/branding/contracts/schemas.py +++ b/modules/branding/branding/contracts/schemas.py @@ -13,11 +13,43 @@ HEX_COLOR_RE, MAX_APP_NAME_LEN, MAX_BANNER_MESSAGE_LEN, + MAX_FOOTER_LINKS, clean_app_name, clean_banner_message, + clean_footer_href, + clean_footer_label, ) +class FooterLink(SQLModel): + """One link in the site footer. + + Deliberately just a label and a target: the surface removed in #273 grew + columns, social icons and a tagline, and the thing hosts actually lost was + the ability to stop advertising the framework's repository. + """ + + label: str + href: str + + @field_validator("label") + @classmethod + def _clean_label(cls, value: str) -> str: + return clean_footer_label(value) + + @field_validator("href") + @classmethod + def _clean_href(cls, value: str) -> str: + return clean_footer_href(value) + + +def bounded_footer_links(links: list[FooterLink]) -> list[FooterLink]: + """Cap the list length (shared by the settings and update DTO).""" + if len(links) > MAX_FOOTER_LINKS: + raise ValueError(f"at most {MAX_FOOTER_LINKS} footer links may be set") + return links + + class BrandingOut(SQLModel): """Current branding, with logo/favicon resolved to download URLs.""" @@ -30,6 +62,8 @@ class BrandingOut(SQLModel): favicon_url: str | None = None banner_message: str = "" banner_severity: str = "" + #: Empty means the framework's own links are shown. + footer_links: list[FooterLink] = Field(default_factory=list) class BrandingUpdate(SQLModel): @@ -40,6 +74,13 @@ class BrandingUpdate(SQLModel): design_pack: str | None = Field(default=None) banner_message: str | None = Field(default=None, max_length=MAX_BANNER_MESSAGE_LEN) banner_severity: str | None = Field(default=None) + #: Send ``[]`` to fall back to the framework's own links. + footer_links: list[FooterLink] | None = Field(default=None) + + @field_validator("footer_links") + @classmethod + def _bounded_links(cls, value: list[FooterLink] | None) -> list[FooterLink] | None: + return None if value is None else bounded_footer_links(value) @field_validator("banner_message") @classmethod diff --git a/modules/branding/branding/locales/en.json b/modules/branding/branding/locales/en.json index 0d9ee10f..17a20d50 100644 --- a/modules/branding/branding/locales/en.json +++ b/modules/branding/branding/locales/en.json @@ -35,7 +35,15 @@ "upload_error_toast": "Could not upload image", "preview_no_banner": "No banner set", "preview_button": "Action", - "banner_severity_label": "Banner severity" + "banner_severity_label": "Banner severity", + "footer_links_label": "Footer links", + "footer_links_help": "Shown in the footer of every page, signed-in or not. Remove them all to fall back to the framework's own links.", + "footer_links_empty": "No links set — the framework's own Docs, Changelog and GitHub links are shown.", + "footer_links_add": "Add link", + "footer_link_label_label": "Link text", + "footer_link_label_placeholder": "Docs", + "footer_link_href_label": "Link target", + "footer_link_href_placeholder": "https://example.org/docs or /about" }, "nav": { "branding": "Branding" diff --git a/modules/branding/branding/pages/Manage.tsx b/modules/branding/branding/pages/Manage.tsx index 82530043..a0216e4b 100644 --- a/modules/branding/branding/pages/Manage.tsx +++ b/modules/branding/branding/pages/Manage.tsx @@ -12,6 +12,7 @@ import { toast } from 'sonner'; import { BannerField, type BannerSeverity } from '../components/BannerField'; import { BrandingPreview } from '../components/BrandingPreview'; import { DesignPackField, type DesignPackOption } from '../components/DesignPackField'; +import { type FooterLink, FooterLinksField } from '../components/FooterLinksField'; import { ImageField } from '../components/ImageField'; import { PresetField, type PresetOption } from '../components/PresetField'; @@ -49,6 +50,7 @@ function Manage() { const [bannerSeverity, setBannerSeverity] = useState( (branding?.banner?.severity as BannerSeverity) ?? 'info', ); + const [footerLinks, setFooterLinks] = useState(branding?.footerLinks ?? []); const [busy, setBusy] = useState(false); // Applying a preset changes branding on the *server*; `router.reload()` brings @@ -61,6 +63,10 @@ function Manage() { const propDesignPack = branding?.designPack ?? ''; const propBannerMessage = branding?.banner?.message ?? ''; const propBannerSeverity = (branding?.banner?.severity as BannerSeverity) ?? 'info'; + // Serialised for the same reason the others are primitives: the array's + // identity changes on every `router.reload()`, so depending on it directly + // would re-seed the rows mid-edit and discard in-progress typing. + const propFooterLinks = JSON.stringify(branding?.footerLinks ?? []); useEffect(() => { setAppName(propAppName); @@ -68,7 +74,15 @@ function Manage() { setDesignPack(propDesignPack); setBannerMessage(propBannerMessage); setBannerSeverity(propBannerSeverity); - }, [propAppName, propColor, propDesignPack, propBannerMessage, propBannerSeverity]); + setFooterLinks(JSON.parse(propFooterLinks) as FooterLink[]); + }, [ + propAppName, + propColor, + propDesignPack, + propBannerMessage, + propBannerSeverity, + propFooterLinks, + ]); async function run(work: () => Promise, errorMsg: string) { setBusy(true); @@ -96,6 +110,9 @@ function Manage() { design_pack: designPack, banner_message: bannerMessage, banner_severity: bannerSeverity, + // Blank rows are the natural state of a row you just added and + // haven't filled in; dropping them here beats a 422 on save. + footer_links: footerLinks.filter((l) => l.label.trim() && l.href.trim()), }), }), t(keys.branding.manage.error_toast), @@ -199,6 +216,12 @@ function Manage() { disabled={!canManage || busy} /> + + BrandingOut: favicon_url=asset_url(FAVICON_URL, settings.favicon_file_id), banner_message=settings.banner_message, banner_severity=settings.banner_severity, + footer_links=list(settings.footer_links), ) async def apply(self, changes: dict[str, Any]) -> BrandingOut: diff --git a/modules/branding/branding/settings.py b/modules/branding/branding/settings.py index 92eeb53f..4f721a06 100644 --- a/modules/branding/branding/settings.py +++ b/modules/branding/branding/settings.py @@ -11,8 +11,9 @@ from __future__ import annotations -from pydantic import field_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic import Field, field_validator +from pydantic_settings import SettingsConfigDict +from simple_module_core.settings_base import DbBackedSettings from branding.constants import ( BANNER_SEVERITY_INFO, @@ -23,13 +24,17 @@ clean_banner_message, normalize_banner_severity, ) +from branding.contracts.schemas import FooterLink, bounded_footer_links DEFAULT_APP_NAME = "SimpleModule" -class BrandingSettings(BaseSettings): +class BrandingSettings(DbBackedSettings): """Customisable application identity.""" + # ``DbBackedSettings`` (not ``BaseSettings``) so the DB is genuinely the + # only source: omitting ``env_prefix`` would leave pydantic-settings + # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") app_name: str = DEFAULT_APP_NAME @@ -43,6 +48,14 @@ class BrandingSettings(BaseSettings): design_pack: str = "" # "" = base tokens only; otherwise a registered slug banner_message: str = "" # "" = no site-wide banner banner_severity: str = BANNER_SEVERITY_INFO + # [] = show the framework's own links, so a deployment that never touches + # this keeps the footer it has today. + footer_links: list[FooterLink] = Field(default_factory=list) + + @field_validator("footer_links") + @classmethod + def _bounded_links(cls, value: list[FooterLink]) -> list[FooterLink]: + return bounded_footer_links(value) @field_validator("app_name") @classmethod diff --git a/modules/branding/branding/shared_props.py b/modules/branding/branding/shared_props.py index bafd0393..ff54aa27 100644 --- a/modules/branding/branding/shared_props.py +++ b/modules/branding/branding/shared_props.py @@ -44,6 +44,11 @@ def branding_payload(settings: BrandingSettings) -> dict: # deployment with a single logo keeps its current appearance. "logoDarkUrl": asset_url(LOGO_DARK_URL, settings.logo_dark_file_id), "faviconUrl": asset_url(FAVICON_URL, settings.favicon_file_id), + # None when the admin hasn't set any, so the frontend falls back to the + # framework's own BRAND_FOOTER_LINKS rather than rendering an empty row. + "footerLinks": ( + [{"label": link.label, "href": link.href} for link in settings.footer_links] or None + ), # None when no message is set, so the frontend renders nothing at all # rather than an empty bar. "banner": ( diff --git a/modules/branding/tests/test_branding.py b/modules/branding/tests/test_branding.py index 0a81abec..0d7b2eae 100644 --- a/modules/branding/tests/test_branding.py +++ b/modules/branding/tests/test_branding.py @@ -61,6 +61,9 @@ def test_branding_payload_unset() -> None: "faviconUrl": None, # None, not an empty dict — no message means render no bar at all. "banner": None, + # None = the admin set no links, so the frontend keeps the framework's + # own BRAND_FOOTER_LINKS. + "footerLinks": None, } diff --git a/modules/branding/tests/test_footer_links.py b/modules/branding/tests/test_footer_links.py new file mode 100644 index 00000000..4fd9fab2 --- /dev/null +++ b/modules/branding/tests/test_footer_links.py @@ -0,0 +1,168 @@ +"""Admin-configurable footer links — GH #282. + +Until 0.0.32 the footer's links were a module-level constant in +``@simple-module-py/ui``, so every deployment advertised the framework +author's repository under "Docs", "Changelog" and "GitHub" on every page, +with no setting, prop or admin screen that changed it. +""" + +from __future__ import annotations + +import pytest +from branding.constants import MAX_FOOTER_LINKS +from branding.contracts.schemas import BrandingUpdate, FooterLink +from branding.settings import BrandingSettings +from branding.shared_props import branding_payload +from httpx import AsyncClient +from pydantic import ValidationError + +_LINKS = [ + {"label": "Docs", "href": "https://example.org/docs"}, + {"label": "Contact", "href": "mailto:team@example.org"}, + {"label": "About", "href": "/about"}, +] + + +class TestSettings: + def test_unset_is_empty_so_the_framework_links_stand(self) -> None: + assert BrandingSettings().footer_links == [] + + def test_links_hydrate_from_plain_dicts(self) -> None: + """The settings store round-trips this field as JSON.""" + settings = BrandingSettings(footer_links=_LINKS) + + assert [link.label for link in settings.footer_links] == ["Docs", "Contact", "About"] + assert settings.footer_links[2].href == "/about" + + def test_too_many_links_are_rejected(self) -> None: + too_many = [{"label": f"L{i}", "href": "/x"} for i in range(MAX_FOOTER_LINKS + 1)] + + with pytest.raises(ValidationError, match="at most"): + BrandingSettings(footer_links=too_many) + + def test_blank_label_is_rejected(self) -> None: + with pytest.raises(ValidationError, match="must not be blank"): + BrandingSettings(footer_links=[{"label": " ", "href": "/x"}]) + + +class TestHrefAllowList: + """The href lands in an ```` on every page, signed-in or not.""" + + @pytest.mark.parametrize( + "href", + [ + "javascript:alert(1)", + "JavaScript:alert(1)", + " javascript:alert(1)", + "data:text/html;base64,PHNjcmlwdD4=", + "vbscript:msgbox(1)", + "//evil.example.com", + ], + ) + def test_dangerous_or_offsite_schemes_are_rejected(self, href: str) -> None: + with pytest.raises(ValidationError): + FooterLink(label="Click", href=href) + + @pytest.mark.parametrize( + "href", + [ + "https://example.org", + "http://example.org/a/b?c=d#e", + "mailto:team@example.org", + "/about", + "/a/deep/path?q=1", + ], + ) + def test_allowed_targets_survive(self, href: str) -> None: + assert FooterLink(label="Link", href=href).href == href + + @pytest.mark.parametrize( + "href", + [ + "/\\evil.example.com", + "/\\\\evil.example.com", + "https://ok.example.org/a\\b", + ], + ) + def test_backslashes_are_rejected(self, href: str) -> None: + """A browser reads `/\\host` as `//host` — the bypass the `//` rule closes.""" + with pytest.raises(ValidationError, match="backslash"): + FooterLink(label="Click", href=href) + + def test_percent_encoded_backslash_is_still_a_path(self) -> None: + """`%5C` is decoded after the authority is parsed, so it stays relative.""" + assert FooterLink(label="Link", href="/%5Cfile").href == "/%5Cfile" + + def test_control_characters_are_rejected(self) -> None: + with pytest.raises(ValidationError, match="control characters"): + FooterLink(label="Link", href="https://example.org\nX") + + +class TestSharedProp: + def test_unset_sends_none_so_the_frontend_falls_back(self) -> None: + assert branding_payload(BrandingSettings())["footerLinks"] is None + + def test_set_links_reach_the_frontend_in_order(self) -> None: + payload = branding_payload(BrandingSettings(footer_links=_LINKS)) + + assert payload["footerLinks"] == _LINKS + + +class TestUpdateDto: + def test_omitting_the_field_leaves_links_alone(self) -> None: + """A PUT that only changes the app name must not wipe the footer.""" + data = BrandingUpdate(app_name="Acme") + + assert "footer_links" not in data.model_dump(exclude_unset=True) + + def test_dumps_to_plain_dicts_for_the_settings_store(self) -> None: + """``apply_changes_and_reload`` json.dumps() whatever it's handed.""" + data = BrandingUpdate(footer_links=_LINKS) + + assert data.model_dump(exclude_unset=True)["footer_links"] == _LINKS + + def test_empty_list_is_a_real_value_not_an_omission(self) -> None: + """Clearing back to the framework defaults has to be expressible.""" + data = BrandingUpdate(footer_links=[]) + + assert data.model_dump(exclude_unset=True)["footer_links"] == [] + + +class TestApi: + async def test_put_persists_and_reaches_the_shared_props( + self, authenticated_client: AsyncClient + ) -> None: + res = await authenticated_client.put("/api/branding/", json={"footer_links": _LINKS}) + assert res.status_code == 200, res.text + assert res.json()["footer_links"] == _LINKS + + page = await authenticated_client.get("/admin/branding/", headers={"X-Inertia": "true"}) + assert page.json()["props"]["branding"]["footerLinks"] == _LINKS + + async def test_put_rejects_a_javascript_href(self, authenticated_client: AsyncClient) -> None: + res = await authenticated_client.put( + "/api/branding/", + json={"footer_links": [{"label": "Click", "href": "javascript:alert(1)"}]}, + ) + + assert res.status_code == 422 + + async def test_clearing_restores_the_framework_links( + self, authenticated_client: AsyncClient + ) -> None: + await authenticated_client.put("/api/branding/", json={"footer_links": _LINKS}) + + res = await authenticated_client.put("/api/branding/", json={"footer_links": []}) + + assert res.status_code == 200, res.text + assert res.json()["footer_links"] == [] + + async def test_anonymous_visitors_get_the_links_too( + self, authenticated_client: AsyncClient, client: AsyncClient + ) -> None: + """The footer renders on the public shell, where nobody is signed in.""" + await authenticated_client.put("/api/branding/", json={"footer_links": _LINKS}) + + page = await client.get("/", headers={"X-Inertia": "true"}) + + assert page.json()["props"]["branding"]["footerLinks"] == _LINKS diff --git a/modules/file_storage/file_storage/settings.py b/modules/file_storage/file_storage/settings.py index 82e913e1..f38a277b 100644 --- a/modules/file_storage/file_storage/settings.py +++ b/modules/file_storage/file_storage/settings.py @@ -14,12 +14,13 @@ from pathlib import Path from pydantic import Field, model_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict +from simple_module_core.settings_base import DbBackedSettings from file_storage import constants -class FileStorageSettings(BaseSettings): +class FileStorageSettings(DbBackedSettings): """Configuration for the file_storage module. The active backend is selected by ``backend`` (matches a key in the @@ -29,6 +30,9 @@ class FileStorageSettings(BaseSettings): subclassing or by reading additional fields at registration time. """ + # ``DbBackedSettings`` (not ``BaseSettings``) so the DB is genuinely the + # only source: omitting ``env_prefix`` would leave pydantic-settings + # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") backend: str = constants.DEFAULT_BACKEND diff --git a/modules/keycloak/keycloak/settings.py b/modules/keycloak/keycloak/settings.py index b9f2b286..bb8d2785 100644 --- a/modules/keycloak/keycloak/settings.py +++ b/modules/keycloak/keycloak/settings.py @@ -3,17 +3,21 @@ from __future__ import annotations from pydantic import Field, field_validator, model_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict from simple_module_core.dotenv import env_str from simple_module_core.environments import NON_PROD_ENVIRONMENTS from simple_module_core.redirect_safety import non_empty_redirect +from simple_module_core.settings_base import DbBackedSettings DEFAULT_LOGIN_REDIRECT_URL = "/dashboard/" -class KeycloakSettings(BaseSettings): +class KeycloakSettings(DbBackedSettings): """Keycloak OIDC configuration.""" + # ``DbBackedSettings`` (not ``BaseSettings``) so the DB is genuinely the + # only source: omitting ``env_prefix`` would leave pydantic-settings + # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") server_url: str = env_str("SM_KEYCLOAK_SERVER_URL", "") diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index 54524f41..a5995fe7 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -139,13 +139,16 @@ def _env_readable_var(settings: BaseSettings, name: str) -> str | None: ``env_var`` on the view is a *label* — the ``SM__`` name the ``smpy settings import-from-env`` CLI looks for, kept from before settings moved into the DB. It is not evidence that pydantic reads it: the bundled - module settings classes declare ``SettingsConfigDict(extra="ignore")`` with - no ``env_prefix``, so ``SM_FILE_STORAGE_BACKEND`` has no effect on - ``FileStorageSettings()``. Deriving env-readability from the class's own - ``env_prefix`` keeps the "From environment" badge honest, and works as-is - for the classes that do declare one — the host's ``Settings`` (``SM_``) and - every module built from the scaffold, whose template ships - ``env_prefix="SM__"``. + module settings classes subclass ``DbBackedSettings``, which drops the env + source entirely, so ``SM_FILE_STORAGE_BACKEND`` has no effect on + ``FileStorageSettings()``. (Until GH #283 those classes subclassed + ``BaseSettings`` and merely omitted ``env_prefix``, which left pydantic + reading each field from its *bare* name instead of not at all.) Deriving + env-readability from the class's own ``env_prefix`` keeps the "From + environment" badge honest, and works as-is for the classes that do declare + one — the host's ``Settings`` (``SM_``), ``BackgroundTasksSettings`` + (``SM_BG_TASKS_``), and every module built from the scaffold, whose + template ships ``env_prefix="SM__"``. """ env_prefix = str(type(settings).model_config.get("env_prefix") or "") if not env_prefix: diff --git a/modules/settings/settings/reload.py b/modules/settings/settings/reload.py index e7be49fe..e6105e04 100644 --- a/modules/settings/settings/reload.py +++ b/modules/settings/settings/reload.py @@ -41,11 +41,16 @@ async def apply_changes_and_reload( services = getattr(app.state, package) current = services.settings - diff = {k: v for k, v in changes.items() if getattr(current, k) != v} + # Compare against the *dumped* current value, not the attribute: a field + # typed as a list of models (branding's ``footer_links``) holds model + # instances, while ``changes`` carries the plain dicts a DTO dumps to. The + # attribute comparison never matched those, so an unchanged list was + # rewritten to the store on every save. + merged = current.model_dump() + diff = {k: v for k, v in changes.items() if merged.get(k) != v} if not diff: return current - merged = current.model_dump() merged.update(diff) validated = cls(**merged) diff --git a/modules/settings/settings/settings.py b/modules/settings/settings/settings.py index 99a27ed2..58ee5e47 100644 --- a/modules/settings/settings/settings.py +++ b/modules/settings/settings/settings.py @@ -2,10 +2,14 @@ from __future__ import annotations -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict +from simple_module_core.settings_base import DbBackedSettings -class SettingsSettings(BaseSettings): +class SettingsSettings(DbBackedSettings): """Configuration for the Settings module.""" + # ``DbBackedSettings`` (not ``BaseSettings``) so the DB is genuinely the + # only source: omitting ``env_prefix`` would leave pydantic-settings + # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") diff --git a/modules/site_lock/site_lock/settings.py b/modules/site_lock/site_lock/settings.py index b39fa43d..5043c847 100644 --- a/modules/site_lock/site_lock/settings.py +++ b/modules/site_lock/site_lock/settings.py @@ -9,14 +9,18 @@ from pydantic import model_validator from pydantic_core import InitErrorDetails, PydanticCustomError from pydantic_core import ValidationError as CoreValidationError -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict +from simple_module_core.settings_base import DbBackedSettings _BLANK_PASSWORD_MSG = "password must be set before enabling the site lock" -class SiteLockSettings(BaseSettings): +class SiteLockSettings(DbBackedSettings): """Site-wide shared-password gate configuration.""" + # ``DbBackedSettings`` (not ``BaseSettings``) so the DB is genuinely the + # only source: omitting ``env_prefix`` would leave pydantic-settings + # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") enabled: bool = False diff --git a/modules/users/users/settings.py b/modules/users/users/settings.py index 22ba66f9..84976853 100644 --- a/modules/users/users/settings.py +++ b/modules/users/users/settings.py @@ -13,10 +13,11 @@ import os from pydantic import Field, field_validator, model_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict from simple_module_core.dotenv import env_str from simple_module_core.environments import NON_PROD_ENVIRONMENTS from simple_module_core.redirect_safety import non_empty_redirect +from simple_module_core.settings_base import DbBackedSettings _PLACEHOLDER_RESET_SECRET = "dev-reset-token-secret-change-me" _PLACEHOLDER_VERIFY_SECRET = "dev-verify-token-secret-change-me" @@ -24,9 +25,12 @@ DEFAULT_LOGIN_REDIRECT_URL = "/dashboard/" -class UsersSettings(BaseSettings): +class UsersSettings(DbBackedSettings): """Local user management configuration.""" + # ``DbBackedSettings`` (not ``BaseSettings``) so the DB is genuinely the + # only source: omitting ``env_prefix`` would leave pydantic-settings + # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") # Self-service signup diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 0b42adcd..b7086595 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -164,6 +164,14 @@ export default { 'branding.manage.error_toast': '', 'branding.manage.favicon_help': '', 'branding.manage.favicon_label': '', + 'branding.manage.footer_link_href_label': '', + 'branding.manage.footer_link_href_placeholder': '', + 'branding.manage.footer_link_label_label': '', + 'branding.manage.footer_link_label_placeholder': '', + 'branding.manage.footer_links_add': '', + 'branding.manage.footer_links_empty': '', + 'branding.manage.footer_links_help': '', + 'branding.manage.footer_links_label': '', 'branding.manage.logo_dark_help': '', 'branding.manage.logo_dark_label': '', 'branding.manage.logo_help': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 0261bc6b..0605959e 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -215,6 +215,14 @@ export const keys = { error_toast: 'branding.manage.error_toast', favicon_help: 'branding.manage.favicon_help', favicon_label: 'branding.manage.favicon_label', + footer_link_href_label: 'branding.manage.footer_link_href_label', + footer_link_href_placeholder: 'branding.manage.footer_link_href_placeholder', + footer_link_label_label: 'branding.manage.footer_link_label_label', + footer_link_label_placeholder: 'branding.manage.footer_link_label_placeholder', + footer_links_add: 'branding.manage.footer_links_add', + footer_links_empty: 'branding.manage.footer_links_empty', + footer_links_help: 'branding.manage.footer_links_help', + footer_links_label: 'branding.manage.footer_links_label', logo_dark_help: 'branding.manage.logo_dark_help', logo_dark_label: 'branding.manage.logo_dark_label', logo_help: 'branding.manage.logo_help', diff --git a/packages/ui/src/components/BrandingFooter.test.tsx b/packages/ui/src/components/BrandingFooter.test.tsx index ceb01d12..7887aaeb 100644 --- a/packages/ui/src/components/BrandingFooter.test.tsx +++ b/packages/ui/src/components/BrandingFooter.test.tsx @@ -17,6 +17,52 @@ describe('BrandingFooter', () => { expect(screen.getByText(new RegExp(`${year}.*MIT`))).toBeInTheDocument(); }); + test('renders host-configured links instead of the framework ones', () => { + render( + , + ); + expect(screen.getByRole('link', { name: 'Handbook' })).toHaveAttribute( + 'href', + 'https://acme.example.org/handbook', + ); + expect(screen.getByRole('link', { name: 'Contact' })).toBeInTheDocument(); + // GH #282: the whole point is that the framework repo stops being advertised. + expect(screen.queryByRole('link', { name: 'GitHub' })).not.toBeInTheDocument(); + expect(screen.queryByRole('link', { name: 'Changelog' })).not.toBeInTheDocument(); + }); + + test.each([ + ['null', null], + ['undefined', undefined], + ['an empty list', []], + ])('falls back to the framework links when links is %s', (_name, links) => { + render(); + expect(screen.getByRole('link', { name: 'Docs' })).toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'GitHub' })).toBeInTheDocument(); + }); + + test('renders two links sharing a href', () => { + // Nothing server-side enforces href uniqueness, so a href-keyed list would + // collide here and reconcile unpredictably. + render( + , + ); + expect(screen.getByRole('link', { name: 'Docs' })).toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Handbook' })).toBeInTheDocument(); + }); + test('renders the uploaded logo when a logoUrl is provided', () => { render(); expect(screen.getByRole('img', { name: 'Acme' })).toHaveAttribute( diff --git a/packages/ui/src/components/BrandingFooter.tsx b/packages/ui/src/components/BrandingFooter.tsx index 703b8a12..9a86b021 100644 --- a/packages/ui/src/components/BrandingFooter.tsx +++ b/packages/ui/src/components/BrandingFooter.tsx @@ -1,4 +1,4 @@ -import { BRAND_ACCENT, BRAND_FOOTER_LINKS, BRAND_LICENSE } from '../lib/brand'; +import { BRAND_ACCENT, BRAND_FOOTER_LINKS, BRAND_LICENSE, type BrandLink } from '../lib/brand'; import { BrandingMark } from './BrandingMark'; /** Stable for the lifetime of the bundle — the year only matters at page load. */ @@ -14,6 +14,13 @@ interface BrandingFooterProps { * the full content width of the sidebar shell. */ variant?: 'app' | 'public'; + /** + * Links shown on the right. Omitted or `null` falls back to the framework's + * own `BRAND_FOOTER_LINKS`, so a host that configures nothing is unchanged. + * Layouts pass the `branding` shared prop's `footerLinks` through, which is + * what makes the footer white-labellable without forking this component. + */ + links?: BrandLink[] | null; } /** Framework-owned footer shared by the authenticated and public layouts. */ @@ -21,7 +28,12 @@ export function BrandingFooter({ appName, logoUrl, variant = 'app', + links, }: BrandingFooterProps): React.ReactElement { + // An empty array is treated as "unset" too: it is what the server sends + // for a deployment that has cleared its links, and a footer with no links + // at all reads as broken rather than deliberate. + const shown = links?.length ? links : BRAND_FOOTER_LINKS; const container = variant === 'public' ? 'mx-auto max-w-6xl px-4 py-6 sm:px-8' : 'px-4 py-6 sm:px-6 lg:px-8'; @@ -39,9 +51,13 @@ export function BrandingFooter({ />