Fix #282, #283, #284 — footer link override, settings env namespacing, package-update pin style - #287
Draft
antosubash wants to merge 6 commits into
Draft
Fix #282, #283, #284 — footer link override, settings env namespacing, package-update pin style#287antosubash wants to merge 6 commits into
antosubash wants to merge 6 commits into
Conversation
`smpy package-update` rewrote every constraint to `name>=<latest>`, 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 `>=<latest>`. `--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.
…#283) `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.
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.
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 `<a href>` 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.
Deploying simple-module-python with
|
| Latest commit: |
88d4c81
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://09445789.simple-module-python.pages.dev |
| Branch Preview URL: | https://claude-github-issues-rnukh8.simple-module-python.pages.dev |
Four defects found reviewing this branch.
**`!=1.0.*` never excluded the latest, producing an unsatisfiable pin.**
`version_key` maps `*` to 0, so `!=1.0.*` compared numerically read as
`!=1.0.0` and `_excluded_by` returned nothing. `>=1.0,!=1.0.*` with latest
1.0.5 was rewritten to `>=1.0.5,!=1.0.*`, which nothing can satisfy — and
the tool's own closing message tells you to run `uv sync` next. Wildcards
now match on a release-segment prefix and never go through the numeric
comparison.
**`~=` and `==X.Y.*` were treated as pure floors, ignoring their implicit
ceilings.** `~=1.4` means `>=1.4, ==1.*`, so bumping it to `~=2.0.0` moved
it to a different compatible band entirely instead of taking the documented
"upper bound excludes latest → skip and report" path. `==1.0.*` with latest
1.0.5 was narrowed to `==1.0.5`, though the band already allowed it.
`~=` now moves only within its band, and a wildcard band that already
covers the latest is left exactly as written.
**`clean_footer_href` accepted `/\evil.example.com`.** The guard was
`startswith("/") and not startswith("//")`, but browsers normalise `\` to
`/` in the authority position of a special-scheme URL, so that href reads
as a site-relative path and navigates to `https://evil.example.com` — the
exact bypass the `//host` rule exists to close, reachable by anyone with
`branding.manage` and landing on every page including the anonymous public
shell. Raw backslashes are now rejected outright; `%5C` still works for one
in a path, since percent-decoding happens after the authority is parsed.
**`BrandingFooter` keyed its links on `link.href`.** That is
admin-supplied data and nothing enforces href uniqueness, so two links to
the same target collided on their React key. Keyed on the index instead,
matching `FooterLinksField`.
The constraint-rewriting tests move to `test_cli_package_update_pins.py`
to stay under the 300-line cap, and the two helpers they shared with the
original file become `fake_pypi` / `write_pyproject` fixtures — this
directory has no `__init__.py`, so its conftest is where helpers are shared.
#290 landed a config-to-settings pass that overlaps this branch's #283 work, so three of the five conflicts are about which env story wins: - HostSettings takes main's side. #290 makes it deliberately env-readable ("Precedence is env → DB → defaults") and reads it pre-app in merge_host_settings, so DbBackedSettings would break the setup wizard. main's env_prefix="SM_" already closes the bare-name hole #283 reported, which also makes the Settings shim's source override unnecessary — reverted. - users, keycloak and the other module settings keep DbBackedSettings: main left those on bare SettingsConfigDict(extra="ignore"), so the hole is still open there. - background_tasks keeps env_prefix and gains main's SM_REDIS_URL fallback. With the prefix set, pydantic's env source answers the legacy SM_BG_TASKS_BROKER_URL / _RESULT_BACKEND before any default factory runs, so the deprecation warning moves to a validator that fires when the legacy var is the value's actual source. SidebarLayout hit 301 lines once main's growth met the footerLinks prop; hoisting the prop to a local keeps the call on one line and the file at 299. uv run pytest 2379 passed, make test-js 139 passed, make lint clean, make doctor 0 diagnostics. Claude-Session: https://claude.ai/code/session_012k5QBVkMXJqJLVqtWbZJht
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three open issues that had no PR. Each is a separate commit; a fourth generalises #283's root cause, which turned out to affect five more settings classes, and a fifth fixes findings from reviewing the branch.
Closes #282, closes #283, closes #284.
#284 —
smpy package-updaterewrote pin style, not just versionspackage-updaterewrote every constraint toname>=, so bumping versions silently changed a project's pinning policy. A host pinned at==0.0.32came back as>=0.0.32while the module wheels it depends on still pin exactly, leaving the effective version decided by whichever wheel pins hardest.The bump now changes the version and nothing else:
==0.0.32==0.0.33>=0.0.32>=0.0.33~=1.4.2~=1.4.9— same compatible band~=1.4~=1.4means>=1.4, ==1.*==1.0.*==1.0.*>=1.0,!=1.0.*>0.0.1>0.0.33would exclude the release being installed>=0.1,<1.02.0.0 excluded by <1.0>=0.1,<3.0>=2.0.0,<3.0>=0.0.33— no pin style to preserveWildcards and
~=are the subtle half.version_keymaps*to 0, so comparing!=1.0.*numerically reads it as!=1.0.0and it never fires — which produced>=1.0.5,!=1.0.*, a specifier nothing can satisfy, right before the tool tells you to runuv sync. Wildcards now match on a release-segment prefix and never reach the numeric comparison, and~=moves only within the band it implies.--loosenrestores 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, where before they were dropped.Requirement parsing moves to
requirements.pyand the PyPI lookup topypi.py, keeping every file under the 300-line cap.#283 —
BackgroundTasksSettingsread unprefixed env varsSettingsConfigDict(extra="ignore")with noenv_prefixdoesn't disable env reads — it un-namespaces them, so pydantic resolved each field from its bare name. The documentedSM_BG_TASKS_*names did nothing whilebroker_url,result_backend,retention_daysand friends were live reads.Setting
env_prefix=ENV_PREFIXgives the class one rule and aligns it with the docstring,settings.env_vars, the docker-compose recipe,tasks.pyand_assert_broker_isolated— all of which already assumed the prefix. The one-offdefault_factoryreads and theenv_boolcall ontask_always_eagerbecome redundant and go — the env source answers those names now. (The broker and result-backend factories come back in the merge below, forSM_REDIS_URLrather than for their own names.)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.
Breaking: deployments relying on the accidental bare names must rename them to
SM_BG_TASKS_*.The same bug in five more classes
The cause wasn't specific to background_tasks. Verified on
main, with these variables set in the environment:enabled,password,backend,base_urlandclient_secretare 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_urlis the origin of password-reset links.DbBackedSettings(new, insimple_module_core.settings_base) keeps only the init source, so values come from the constructor — which is how DB hydration already sets them — and nothing else. site_lock, users, file_storage, settings and branding now subclass it. This is what_module_settings.pyalready told the admin UI was true, and what the 2026-04-21 DB-backed-settings plan intended by droppingenv_prefix.background_tasksdeliberately keepsBaseSettings+ an explicit prefix: its broker URL must be readable before any DB row exists.HostSettingswas in that list until #290 landed onmain. #290 makes it deliberately env-readable — its docstring states "Precedence is env → DB → the defaults declared here" — and_preapp_config.merge_host_settingsreads it beforecreate_appbuilds anything, so dropping the env source would break the first-run setup wizard. Itsenv_prefix="SM_"already closes the bare-name hole this issue reported, which is the part that mattered; the merge takesmain's version, and theSettingsshim goes back tomain's too, since the source override it needed only existed to work aroundHostSettingshaving no environment.background_tasksalso picks up #290'sSM_REDIS_URL, with one interaction worth naming: withenv_prefixset, pydantic's env source answersSM_BG_TASKS_BROKER_URL/SM_BG_TASKS_RESULT_BACKENDbefore any default factory runs, so #290's deprecation warning — which lived inside that factory — would have gone silent. It moves to a validator that fires when the legacy variable is the value's actual source, and #290's own warning tests pass unchanged.#282 — footer links hardcoded to the framework repo
BRAND_FOOTER_LINKSwas a module-level constant andBrandingFootermapped over it directly, so every app advertisedantosubash/simple_module_pythonon every page. The configurable footer from #222 was removed by #273/#275, taking the only override with it.footer_linksjoins the other branding values: DB-backed, in thebrandingshared prop asfooterLinks, and edited at/admin/brandingwithout a redeploy.BrandingFootertakes an optionallinksprop and falls back toBRAND_FOOTER_LINKSwhen it is absent, null or empty — so a deployment that never sets any keeps today's footer, and clearing the list is how you go back to it.Deliberately just
{label, href}, capped at 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.hrefis checked against an allow-list —http://,https://,mailto:, or a site-relative path starting with a single/. The value is rendered straight into an<a href>on every page, signed-in or not, sojavascript:anddata:would make this screen a stored-XSS sink for anyone holdingbranding.manage. Scheme-relative//hostis rejected, and so are raw backslashes: browsers normalise\to/in the authority position, so/\evil.example.comreads as a same-site path but navigates off-site.%5Cstill works for a backslash in a path, since percent-decoding happens after the authority is parsed.Also fixes change detection in
apply_changes_and_reload, which comparedchangesagainst the settings attribute. A field typed as a list of models holds model instances whilechangescarries plain dicts, so an unchanged list never compared equal and was rewritten on every save.Verification
On the merge of
main(88d4c81):uv run pytest— 2379 passed, 46 deselectedmake test-js— 139 passed across 23 filesmake lint— clean (the one Biome warning, an unused import inbackground_tasks/pages/components/ExecutionRow.tsx, is pre-existing onmainand untouched here)make doctor— 0 diagnosticsNew tests:
framework/cli/tests/test_cli_package_update_pins.py(pin styles, wildcards,~=bands, upper bounds, extras/markers,--loosen),framework/core/tests/test_settings_base.py,modules/background_tasks/tests/test_bg_settings_env.py,modules/branding/tests/test_footer_links.py(including the href allow-list), andpackages/ui/src/components/BrandingFooter.test.tsx.Notes for review
mainmoved four merges ahead while this branch sat, and #290 reworked the same env-vs-DB question #283 touched — see the merge commit88d4c81for how each conflict was decided. One non-conflict casualty:SidebarLayout.tsxcrossed the 300-line cap once #290's growth met thefooterLinksprop, so the prop is hoisted to a local to keep the call on one line.Two design forks in the issues were resolved as follows, both the option the issue leaned toward:
--loosenis the opt-out.https://claude.ai/code/session_012k5QBVkMXJqJLVqtWbZJht
Generated by Claude Code