From f2000bcb1e1393f85467f897e7afc30f027571c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:40:42 +0000 Subject: [PATCH 1/5] fix(cli): package-update keeps each dependency's pin style (#284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `smpy package-update` rewrote every constraint to `name>=`, so bumping versions silently changed a project's pinning policy. A host that pinned `==0.0.32` came back loosened to `>=0.0.32` while the module wheels it depends on still pin exactly — leaving the effective version decided by whichever wheel pins hardest rather than by the host. The bump now changes the version and nothing else: - `==`, `===`, `~=`, `>=` have their version replaced in place. - `>` is left alone; anything newer already satisfies it, and `>latest` would exclude the release being installed. - `<`, `<=`, `!=` are preserved verbatim, and a dependency whose upper bound excludes the latest release (`>=0.1,<1.0` when 2.0.0 is out) is reported and skipped instead of having its ceiling dropped. - A dependency with no constraint has no pin style to preserve, so it still gets `>=`. `--loosen` restores the previous blanket `>=` rewrite. Two incidental fixes fall out of parsing requirements properly rather than splitting on the first operator: extras (`pkg[redis]`) and environment markers (`; python_version >= '3.12'`) now survive the rewrite, where before they were dropped. Requirement parsing moves to `requirements.py` and the PyPI lookup to `pypi.py`, keeping every file under the 300-line cap. --- docs/guide/installation.md | 16 ++ docs/reference/make-commands.md | 2 +- .../cli/simple_module_cli/package_update.py | 105 +++++------- framework/cli/simple_module_cli/pypi.py | 55 ++++++ .../cli/simple_module_cli/requirements.py | 156 ++++++++++++++++++ .../cli/tests/test_cli_package_update.py | 108 +++++++++++- 6 files changed, 374 insertions(+), 68 deletions(-) create mode 100644 framework/cli/simple_module_cli/pypi.py create mode 100644 framework/cli/simple_module_cli/requirements.py 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/reference/make-commands.md b/docs/reference/make-commands.md index fec4f6a0..aaf2e6ed 100644 --- a/docs/reference/make-commands.md +++ b/docs/reference/make-commands.md @@ -25,7 +25,7 @@ smpy --help | 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..b6871d6a --- /dev/null +++ b/framework/cli/simple_module_cli/requirements.py @@ -0,0 +1,156 @@ +"""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 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. +_FLOOR_OPS = frozenset({"==", "===", "~=", ">="}) + +#: Operators that can exclude the latest release. Kept verbatim; if the latest +#: version trips one, the dependency is skipped rather than silently loosened. +_CEILING_OPS = frozenset({"<", "<=", "!="}) + + +@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 _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 op not in _CEILING_OPS: + continue + cmp = _compare(latest, version) + if (op == "<" and cmp >= 0) or (op == "<=" and cmp > 0) or (op == "!=" and cmp == 0): + return f"{op}{version}" + return None + + +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, latest) if op in _FLOOR_OPS else (op, version) for op, version in parsed.clauses + ) + if bumped == parsed.clauses: + # Only ``>``/``<``/``!=`` clauses: already satisfied by anything newer, + # so raising a floor here would be inventing a constraint. + return RewriteResult(None) + + new = _render(parsed, bumped) + return RewriteResult(None) if new == spec.strip() else RewriteResult(new) diff --git a/framework/cli/tests/test_cli_package_update.py b/framework/cli/tests/test_cli_package_update.py index ae944542..a4aaac81 100644 --- a/framework/cli/tests/test_cli_package_update.py +++ b/framework/cli/tests/test_cli_package_update.py @@ -34,7 +34,7 @@ def test_updates_simple_module_deps(tmp_path: Path) -> None: '[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", @@ -49,7 +49,7 @@ def test_updates_simple_module_deps(tmp_path: Path) -> None: 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 @@ -180,3 +180,107 @@ def test_cli_command_registered() -> None: result = CliRunner().invoke(app, ["package-update", "--help"]) assert result.exit_code == 0 assert "package-update" in result.output or "Update all simple_module" in result.output + + +def _write(pyproject: Path, *deps: str) -> None: + body = "".join(f' "{d}",\n' for d in deps) + pyproject.write_text( + f'[project]\nname = "x"\nversion = "0"\ndependencies = [\n{body}]\n', + encoding="utf-8", + ) + + +def test_exact_pins_stay_exact(tmp_path: Path) -> None: + """The #284 regression: `package-update` bumped versions *and* pin style.""" + pyproject = tmp_path / "pyproject.toml" + _write(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) -> None: + pyproject = tmp_path / "pyproject.toml" + _write(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) -> None: + pyproject = tmp_path / "pyproject.toml" + _write(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) -> None: + """Nothing to preserve, so the tool's default style applies.""" + pyproject = tmp_path / "pyproject.toml" + _write(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] +) -> None: + """The old rewrite silently deleted the ceiling; now it's reported.""" + pyproject = tmp_path / "pyproject.toml" + _write(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) -> None: + pyproject = tmp_path / "pyproject.toml" + _write(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 From f6aa585a5948f4ac7e9e7953a0d77bb0b27bc997 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:43:33 +0000 Subject: [PATCH 2/5] fix(background_tasks): namespace settings env vars under SM_BG_TASKS_ (#283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BackgroundTasksSettings` subclassed `BaseSettings` with no `env_prefix`, so pydantic-settings resolved every field from its bare, case-insensitive name. The documented `SM_BG_TASKS_*` variables did nothing, while `broker_url`, `result_backend`, `task_default_queue`, `retention_days` and `max_retries` were live environment reads under names generic enough that another component setting them for its own purposes would silently reconfigure Celery. It also made the DB the source of truth only when nobody happened to have those names in the environment. Setting `env_prefix=ENV_PREFIX` gives the class a single rule and aligns it with the docstring, `settings.env_vars`, the `smpy` docker-compose recipe, `seed_dev_settings.py`, `tasks.py` and the worker's `_assert_broker_isolated` — all of which already assume the prefix. The one-off `default_factory` reads on `broker_url`/`result_backend` and the `env_bool` call on `task_always_eager` are now redundant and gone. The localhost validator also names the mechanism it expects. It was most people's first encounter with this and said only "set these to the Redis service host", so the natural guess was the prefixed name that had no effect; it now names `SM_BG_TASKS_BROKER_URL` / `SM_BG_TASKS_RESULT_BACKEND` and says why an env var is the only thing that can satisfy it before hydration. Deployments relying on the accidental bare names must rename them. --- .../background_tasks/settings.py | 58 +++++++++-------- .../tests/test_bg_settings_env.py | 64 +++++++++++++++++-- 2 files changed, 90 insertions(+), 32 deletions(-) diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index ef3d56f0..f986b890 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -4,14 +4,19 @@ hosting lifespan before module ``on_startup`` runs. Runtime changes go through ``settings.reload.apply_changes_and_reload``. -Two of those defaults are deployment plumbing rather than module config, so -they stay env-readable: ``SM_BG_TASKS_BROKER_URL`` and -``SM_BG_TASKS_RESULT_BACKEND`` name the Redis a container can actually reach. -They have to work *before* any DB row exists — the production validator below -rejects the localhost defaults, so without them a containerised app can't -boot far enough to hydrate settings, and a worker process (which never sees -``app.state``) has no other source at all. A DB value still wins once -hydration runs. +Every field is also readable from a ``SM_BG_TASKS_``-prefixed environment +variable, which is what ``env_prefix`` on ``model_config`` buys: the names +match the docs, ``settings.env_vars``, the ``smpy`` docker-compose recipe and +the worker's ``_assert_broker_isolated``. A DB value still wins once hydration +runs, so env is the pre-hydration floor rather than an override. + +That floor is load-bearing for two fields in particular. +``SM_BG_TASKS_BROKER_URL`` and ``SM_BG_TASKS_RESULT_BACKEND`` name the Redis a +container can actually reach, and they have to work *before* any DB row +exists: the production validator below rejects the localhost defaults, so +without them a containerised app can't boot far enough to hydrate settings, +and a worker process (which never sees ``app.state``) has no other source at +all. The other env read is ``SM_ENVIRONMENT``, consulted by the ``@model_validator`` to refuse a localhost broker in production — that's a @@ -30,7 +35,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 ( @@ -51,25 +55,21 @@ class BackgroundTasksSettings(BaseSettings): """Configuration for the Celery + Redis task runner.""" - model_config = SettingsConfigDict(extra="ignore") - - broker_url: str = Field( - default_factory=lambda: os.environ.get(f"{ENV_PREFIX}BROKER_URL", DEFAULT_BROKER_URL), - json_schema_extra=_CELERY_RESTART, - ) - result_backend: str = Field( - default_factory=lambda: os.environ.get( - f"{ENV_PREFIX}RESULT_BACKEND", DEFAULT_RESULT_BACKEND - ), - json_schema_extra=_CELERY_RESTART, - ) + # 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=DEFAULT_BROKER_URL, json_schema_extra=_CELERY_RESTART) + result_backend: str = Field(default=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 @@ -101,8 +101,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 1337f063..62ae6692 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 @@ -56,3 +57,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 From d5920abc87f29f176e847763ddef65009a8f8dd5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 06:02:54 +0000 Subject: [PATCH 3/5] fix(settings): stop DB-backed settings classes reading bare env names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalises the root cause of GH #283. Every bundled settings class subclassed `BaseSettings` and simply omitted `env_prefix`, which does not disable environment reads — it un-namespaces them. pydantic-settings still installs its env source and, with no prefix, resolves each field from the bare, case-insensitive field name. Verified before this change, with the named variables set in the environment: SiteLockSettings() -> enabled=True password='hunter2' UsersSettings() -> smtp_host='evil.example.com' base_url='http://evil.example.com' FileStorageSettings() -> backend='s3' s3_bucket='attacker-bucket' `enabled`, `password`, `backend`, `base_url`, `client_secret` and `maintenance_mode` are common enough in a container that an unrelated component setting one silently reconfigures the app — and site_lock's pair is the site gate, while users' `base_url` is the origin of password-reset links. It also made the DB the source of truth only when nobody happened to have those names in the environment. `DbBackedSettings` (new, in `simple_module_core.settings_base`) keeps only the init source, so values come from the constructor — which is how DB hydration already sets them — and from nothing else. site_lock, users, file_storage, settings, branding, keycloak and HostSettings now subclass it. This is what `_module_settings.py` already told the admin UI was true, and what the 2026-04-21 DB-backed-settings plan intended by dropping `env_prefix`. `background_tasks` deliberately keeps `BaseSettings` + an explicit `SM_BG_TASKS_` prefix: its broker URL must be readable before any DB row exists. The `Settings` shim needed an explicit override. It combines `HostSettings` with `BootstrapSettings`, and `HostSettings` now comes first in the MRO carrying the source override with it — which would have stripped the environment from the bootstrap half, where `SM_DATABASE_URL`, `SM_SECRET_KEY` and `SM_AUTH_PROVIDER` are read by design. It restores pydantic's default ordering, so the shim behaves exactly as before. `i18n_supported_locales` moves to `default_factory` on the way past: ruff's RUF012 pydantic exemption keys off the literal `BaseSettings` base, so the mutable default became visible once the base changed. --- .../core/simple_module_core/settings_base.py | 47 ++++++++++++ framework/core/tests/test_settings_base.py | 75 +++++++++++++++++++ .../simple_module_hosting/host_settings.py | 12 ++- .../hosting/simple_module_hosting/settings.py | 26 +++++++ .../tests/test_auth_provider_setting.py | 25 +++++++ modules/branding/branding/settings.py | 8 +- modules/file_storage/file_storage/settings.py | 8 +- modules/keycloak/keycloak/settings.py | 8 +- modules/settings/settings/_module_settings.py | 17 +++-- modules/settings/settings/settings.py | 8 +- modules/site_lock/site_lock/settings.py | 8 +- modules/users/users/settings.py | 8 +- 12 files changed, 227 insertions(+), 23 deletions(-) create mode 100644 framework/core/simple_module_core/settings_base.py create mode 100644 framework/core/tests/test_settings_base.py 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/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index b6d9989c..ec655634 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/framework/hosting/simple_module_hosting/host_settings.py @@ -7,13 +7,17 @@ from __future__ import annotations -from pydantic import model_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic import Field, model_validator +from pydantic_settings import SettingsConfigDict +from simple_module_core.settings_base import DbBackedSettings -class HostSettings(BaseSettings): +class HostSettings(DbBackedSettings): """DB-backed host configuration — defaults live here, overrides in DB.""" + # ``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") multi_tenant: bool = False @@ -30,7 +34,7 @@ class HostSettings(BaseSettings): generic translated copy.""" i18n_default_locale: str = "en" - i18n_supported_locales: list[str] = ["en"] + i18n_supported_locales: list[str] = Field(default_factory=lambda: ["en"]) i18n_cookie_name: str = "locale" @model_validator(mode="after") diff --git a/framework/hosting/simple_module_hosting/settings.py b/framework/hosting/simple_module_hosting/settings.py index 33bd8fbe..2e679a4a 100644 --- a/framework/hosting/simple_module_hosting/settings.py +++ b/framework/hosting/simple_module_hosting/settings.py @@ -2,9 +2,35 @@ from __future__ import annotations +from pydantic_settings import BaseSettings, PydanticBaseSettingsSource + from simple_module_hosting.bootstrap_settings import BootstrapSettings from simple_module_hosting.host_settings import HostSettings class Settings(HostSettings, BootstrapSettings): """Combined bootstrap + host settings for legacy import sites.""" + + @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, ...]: + """Restore the default sources that ``HostSettings`` removes. + + ``HostSettings`` is a ``DbBackedSettings``, which drops the env source + so its fields can't be set by bare names (GH #283). It comes first in + this shim's MRO, so without this override the *bootstrap* half loses + its environment too — and ``SM_DATABASE_URL``, ``SM_SECRET_KEY`` and + ``SM_AUTH_PROVIDER`` are read from the environment by design, before + any database exists to read them from. + + Restoring pydantic's default ordering keeps this shim behaving exactly + as it did: every field resolves under ``BootstrapSettings``' own + ``env_prefix="SM_"``, so the names are namespaced either way. + """ + return (init_settings, env_settings, dotenv_settings, file_secret_settings) diff --git a/framework/hosting/tests/test_auth_provider_setting.py b/framework/hosting/tests/test_auth_provider_setting.py index 348623ff..34310cd5 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 a DB-backed class with an env-backed one. + + ``HostSettings`` is a ``DbBackedSettings`` (GH #283) and comes first in + the MRO, so it would otherwise strip the environment source from the + bootstrap half — where ``SM_DATABASE_URL`` and friends are read by design, + before any database exists to read them from. + """ + + 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/branding/branding/settings.py b/modules/branding/branding/settings.py index 92eeb53f..2b7a163d 100644 --- a/modules/branding/branding/settings.py +++ b/modules/branding/branding/settings.py @@ -12,7 +12,8 @@ from __future__ import annotations from pydantic import field_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import SettingsConfigDict +from simple_module_core.settings_base import DbBackedSettings from branding.constants import ( BANNER_SEVERITY_INFO, @@ -27,9 +28,12 @@ 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 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 e9e9745c..aa4be676 100644 --- a/modules/keycloak/keycloak/settings.py +++ b/modules/keycloak/keycloak/settings.py @@ -3,14 +3,18 @@ from __future__ import annotations from pydantic import Field, 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.settings_base import DbBackedSettings -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 01e936be..19bb4bd5 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -133,13 +133,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/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 83143b73..f928a7ed 100644 --- a/modules/users/users/settings.py +++ b/modules/users/users/settings.py @@ -13,17 +13,21 @@ import os from pydantic import Field, 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.settings_base import DbBackedSettings _PLACEHOLDER_RESET_SECRET = "dev-reset-token-secret-change-me" _PLACEHOLDER_VERIFY_SECRET = "dev-verify-token-secret-change-me" -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 From 28c5bd0a19fce32ad61f3c079aeb81464f14654d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 06:15:47 +0000 Subject: [PATCH 4/5] feat(branding): admin-configurable footer links (#282) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since 0.0.32 there was no supported way for a deployment to change the footer links: `BRAND_FOOTER_LINKS` was a module-level constant in `@simple-module-py/ui`, and `BrandingFooter` mapped over it directly. The configurable footer shipped in 0.0.21 (#222) was removed by #273/#275, which took the only override with it — so every app advertised `antosubash/simple_module_python` under "Docs", "Changelog" and "GitHub" on every page, guest and authenticated. The one workaround, aliasing the brand module in the host's `vite.config.ts`, silently diverges from the package on every bump. `footer_links` joins the other branding values: DB-backed, in the `branding` shared prop as `footerLinks`, and edited at `/admin/branding` without a redeploy. `BrandingFooter` takes an optional `links` prop and falls back to `BRAND_FOOTER_LINKS` when it is absent, null or empty, so a deployment that never sets any keeps exactly the footer it has today — and clearing the list is how you go back to them. Deliberately just `{label, href}` and a cap of 6. What #273 removed had grown columns, social icons and a tagline; what hosts actually lost was the ability to stop advertising the framework's repository. `href` is checked against an allow-list — `http://`, `https://`, `mailto:` or a site-relative path starting with a single `/`. The value is rendered straight into an `` on every page, signed-in or not, so `javascript:` and `data:` would make this screen a stored-XSS sink for anyone holding `branding.manage`; scheme-relative `//host` is rejected too, since it reads as a relative path but navigates off-site. Labels are bounded and reject control characters, as `app_name` already does. Also fixes the change detection in `apply_changes_and_reload`, which compared `changes` against the settings *attribute*. A field typed as a list of models holds model instances while `changes` carries the plain dicts a DTO dumps to, so an unchanged list never compared equal and was rewritten to the store on every save. It now compares against the dumped current value, which is identical for scalars. --- docs/modules/branding.md | 20 ++- .../branding/components/FooterLinksField.tsx | 103 ++++++++++++ modules/branding/branding/constants.py | 55 +++++++ .../branding/branding/contracts/schemas.py | 41 +++++ modules/branding/branding/locales/en.json | 10 +- modules/branding/branding/pages/Manage.tsx | 25 ++- modules/branding/branding/service.py | 1 + modules/branding/branding/settings.py | 11 +- modules/branding/branding/shared_props.py | 5 + modules/branding/tests/test_branding.py | 3 + modules/branding/tests/test_footer_links.py | 151 ++++++++++++++++++ modules/settings/settings/reload.py | 9 +- packages/i18n/src/generated-resources.ts | 8 + packages/i18n/src/keys.generated.ts | 8 + .../ui/src/components/BrandingFooter.test.tsx | 30 ++++ packages/ui/src/components/BrandingFooter.tsx | 16 +- packages/ui/src/layouts/PublicLayout.tsx | 7 +- packages/ui/src/layouts/SidebarLayout.tsx | 7 +- packages/ui/src/types.ts | 6 + 19 files changed, 504 insertions(+), 12 deletions(-) create mode 100644 modules/branding/branding/components/FooterLinksField.tsx create mode 100644 modules/branding/tests/test_footer_links.py 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/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..0f8ff5fc 100644 --- a/modules/branding/branding/constants.py +++ b/modules/branding/branding/constants.py @@ -115,3 +115,58 @@ 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 /" +) + + +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". + """ + 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") + 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 359f9510..378a0715 100644 --- a/modules/branding/branding/pages/Manage.tsx +++ b/modules/branding/branding/pages/Manage.tsx @@ -18,6 +18,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'; @@ -55,6 +56,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 @@ -67,6 +69,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); @@ -74,7 +80,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); @@ -102,6 +116,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), @@ -207,6 +224,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 2b7a163d..4f721a06 100644 --- a/modules/branding/branding/settings.py +++ b/modules/branding/branding/settings.py @@ -11,7 +11,7 @@ from __future__ import annotations -from pydantic import field_validator +from pydantic import Field, field_validator from pydantic_settings import SettingsConfigDict from simple_module_core.settings_base import DbBackedSettings @@ -24,6 +24,7 @@ clean_banner_message, normalize_banner_severity, ) +from branding.contracts.schemas import FooterLink, bounded_footer_links DEFAULT_APP_NAME = "SimpleModule" @@ -47,6 +48,14 @@ class BrandingSettings(DbBackedSettings): 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..1c0d38ec --- /dev/null +++ b/modules/branding/tests/test_footer_links.py @@ -0,0 +1,151 @@ +"""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 + + 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/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/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 478fe935..6a23651a 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -162,6 +162,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 1043e40b..a37ec81a 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -213,6 +213,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..d3eb106f 100644 --- a/packages/ui/src/components/BrandingFooter.test.tsx +++ b/packages/ui/src/components/BrandingFooter.test.tsx @@ -17,6 +17,36 @@ 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 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..b51f555f 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,7 +51,7 @@ export function BrandingFooter({ />