diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index 28810be9..672920e0 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -19,7 +19,9 @@ from settings._secrets import ( # noqa: F401 _NEVER_SECRET_TYPES, SECRET_MASK, + conceals_secret, embeds_credential, + is_named_secret, is_secret_field, mask, ) @@ -129,14 +131,7 @@ def _field_view( info = cls.model_fields[name] raw_value = getattr(settings, name) value_type = value_type_for_field(cls, name) - # A numeric field whose name merely contains a secret-ish word was being - # masked and made uneditable — `reset_password_token_lifetime_seconds` is - # an int, but it matches on "password" exactly as the real secrets do. - # Phrased as "exempt the types that cannot hold a credential" rather than - # "mask only strings" so the failure direction is safe: an unexpected type - # (e.g. `str | None`, which resolves to "json") stays masked instead of - # silently exposing a secret. - named_secret = value_type not in _NEVER_SECRET_TYPES and is_secret_field(name) + named_secret = is_named_secret(name, value_type) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} default = resolve_default(info) env_var = f"{prefix}{name.upper()}" @@ -147,10 +142,7 @@ def _field_view( raw_env = os.environ[live_env_var] if env_set and live_env_var is not None else None def hidden(value: Any) -> bool: - """Mask per *value*, not just per name: a DSN carrying a password is a - secret even on a field called ``broker_url``, while the same field's - password-free default is still worth showing.""" - return named_secret or embeds_credential(value) + return conceals_secret(name, value, value_type) # ``is_secret`` drives the editor's input type and the "reveal" affordance, # so it has to be true when *any* of the three readings is being hidden — diff --git a/modules/settings/settings/_row_masking.py b/modules/settings/settings/_row_masking.py new file mode 100644 index 00000000..cb1b83e0 --- /dev/null +++ b/modules/settings/settings/_row_masking.py @@ -0,0 +1,68 @@ +"""Whether a stored row's value may be shown, and how the mask is honoured. + +Split out of ``service`` so the "is this a secret, and is this write the mask +being echoed back?" question lives in one file, next to the field-level rules in +``_secrets`` that it delegates to. ``service`` imports these; nothing else +should need to, because the point of funnelling every read through ``out`` is +that there is no second way to serialize a row. +""" + +from __future__ import annotations + +from settings._secrets import conceals_secret +from settings.constants import SENSITIVE_KEYS, SENSITIVE_PLACEHOLDER +from settings.contracts.schemas import SettingOut +from settings.models import Setting + + +def is_masked(entity: Setting) -> bool: + """Whether this row's stored value must not leave the service. + + :data:`SENSITIVE_KEYS` names the one key the hosting layer owns + (``host.secret_key``). Everything else is judged by the same name/value + rule the module editor uses, because the store is the *same data* seen + through a different screen: an override named ``users.smtp_password``, or + any override holding a ``postgresql://user:pw@host/db``, was rendered in + clear text in the browse table and pre-filled into the edit form purely + because the allowlist had a single entry in it. + """ + return entity.key in SENSITIVE_KEYS or conceals_secret( + entity.key, entity.value, entity.value_type + ) + + +def out(entity: Setting) -> SettingOut: + """Serialize a row, masking values that must not leave the service. + + Every read path funnels through here so a secret cannot be read back by + listing it, resolving it, or fetching it by id. + """ + out = SettingOut.model_validate(entity) + if is_masked(entity): + return out.model_copy(update={"value": SENSITIVE_PLACEHOLDER}) + return out + + +def is_placeholder_write(entity: Setting, value: object) -> bool: + """Whether this write is the mask being echoed back, not a real new value. + + The admin edit form GETs the row, pre-fills its input from the response, + and PUTs it back. For a masked row that response carries ``"********"``, so + an admin who opens the row and clicks Save — without touching the field — + would otherwise overwrite a real credential with a fixed, publicly-known + string. On ``host.secret_key`` that silently invalidates every session and + makes every future cookie forgeable. + + Gated on the stored row rather than on the sentinel alone, so a non-secret + row whose real value happens to be eight asterisks still saves. + + Treated as "leave it alone" rather than rejected, so the rest of the form + still saves and an admin who genuinely types a new value can still set one. + """ + return is_masked(entity) and value == SENSITIVE_PLACEHOLDER + + +def drop_placeholder_write(entity: Setting, changes: dict) -> None: + """Strip a masked-value echo out of an update payload, in place.""" + if "value" in changes and is_placeholder_write(entity, changes["value"]): + del changes["value"] diff --git a/modules/settings/settings/_secrets.py b/modules/settings/settings/_secrets.py index baff1998..82e4e659 100644 --- a/modules/settings/settings/_secrets.py +++ b/modules/settings/settings/_secrets.py @@ -30,8 +30,34 @@ def is_secret_field(name: str) -> bool: return bool(_SECRET_PATTERNS.search(name)) +def is_named_secret(name: str, value_type: str | None = None) -> bool: + """True if the *name* marks this field as credential material. + + ``value_type`` exempts the types that cannot hold one, so a numeric field + whose name merely contains a secret-ish word is not masked and made + uneditable — ``reset_password_token_lifetime_seconds`` is an int, but it + matches on "password" exactly as the real secrets do. Phrased as "exempt + the types that cannot hold a credential" rather than "mask only strings" so + the failure direction is safe: an unexpected type (``str | None`` resolves + to "json") stays masked instead of silently exposing a secret. + """ + if value_type in _NEVER_SECRET_TYPES: + return False + return is_secret_field(name) + + +def conceals_secret(name: str, value: Any, value_type: str | None = None) -> bool: + """True if this name/value pair must be masked on the way out. + + The one rule every read path shares: mask per *value* as well as per name, + so a DSN carrying a password is hidden even on a field called + ``broker_url``, while that field's password-free default is still shown. + """ + return is_named_secret(name, value_type) or embeds_credential(value) + + def embeds_credential(value: Any) -> bool: - """True if ``value`` is a URL carrying a password in its authority. + """True if ``value`` carries a password in a URL authority. The name rule alone cannot see these: ``broker_url``, ``result_backend``, ``redis_url`` and ``database_url`` match nothing in @@ -43,15 +69,26 @@ def embeds_credential(value: Any) -> bool: Judged on the value rather than the name because the name is the thing that was wrong. A DSN without a password stays visible: hiding ``redis://localhost:6379/0`` helps nobody debug why the queue is idle. + + Containers are walked rather than dismissed: a ``list[str]`` of broker + URLs or a ``dict`` of per-tenant DSNs holds exactly the same material as + the bare string, and returning False for anything non-``str`` meant one + such field would have been shown in full. """ - if not isinstance(value, str) or "://" not in value: - return False - try: - return bool(urlsplit(value).password) - except ValueError: - # A malformed authority (an unclosed IPv6 literal, say) is not a - # credential, but it must not take the settings screen down either. - return False + if isinstance(value, str): + if "://" not in value: + return False + try: + return bool(urlsplit(value).password) + except ValueError: + # A malformed authority (an unclosed IPv6 literal, say) is not a + # credential, but it must not take the settings screen down either. + return False + if isinstance(value, (list, tuple, set, frozenset)): + return any(embeds_credential(item) for item in value) + if isinstance(value, dict): + return any(embeds_credential(item) for item in value.values()) + return False def mask(value: Any) -> Any: diff --git a/modules/settings/settings/browse_query.py b/modules/settings/settings/browse_query.py index 0f00199d..adf5885c 100644 --- a/modules/settings/settings/browse_query.py +++ b/modules/settings/settings/browse_query.py @@ -11,6 +11,9 @@ from __future__ import annotations from dataclasses import dataclass +from typing import Annotated, Any + +from pydantic import BeforeValidator from settings.constants import ALL_SCOPES, DEFAULT_PER_PAGE, MAX_PER_PAGE, SCOPE_ALL from settings.contracts.schemas import SettingScope @@ -32,18 +35,33 @@ def scope_filter(self) -> SettingScope | None: return None if self.scope == SCOPE_ALL else SettingScope(self.scope) -def _int_or(raw: str, fallback: int) -> int: - try: - return int(raw) - except (TypeError, ValueError): - return fallback +def _lenient_int(fallback: int) -> BeforeValidator: + """Coerce a query param to ``int``, substituting ``fallback`` for garbage. + + Declared as a validator on an ``int``-annotated param rather than by typing + the param ``str``: both accept ``?page=banana`` without a 422, but only this + one leaves the OpenAPI schema advertising an integer. A generated client + that reads the schema was otherwise told to send page numbers as strings. + """ + + def coerce(raw: Any) -> int: + try: + return int(raw) + except (TypeError, ValueError): + return fallback + + return BeforeValidator(coerce) + + +PageParam = Annotated[int, _lenient_int(1)] +PerPageParam = Annotated[int, _lenient_int(DEFAULT_PER_PAGE)] -def parse(scope: str, q: str, page: str, per_page: str) -> BrowseQuery: +def parse(scope: str, q: str, page: int, per_page: int) -> BrowseQuery: """Read the four query params, substituting defaults for anything unusable.""" return BrowseQuery( scope=scope if scope in ALL_SCOPES else SCOPE_ALL, q=q, - page=max(_int_or(page, 1), 1), - per_page=max(1, min(_int_or(per_page, DEFAULT_PER_PAGE), MAX_PER_PAGE)), + page=max(page, 1), + per_page=max(1, min(per_page, MAX_PER_PAGE)), ) diff --git a/modules/settings/settings/endpoints/module_api.py b/modules/settings/settings/endpoints/module_api.py index 21221d2e..08568e0d 100644 --- a/modules/settings/settings/endpoints/module_api.py +++ b/modules/settings/settings/endpoints/module_api.py @@ -9,7 +9,7 @@ import json from typing import Any -from fastapi import APIRouter, Depends, HTTPException, Request, Response, status +from fastapi import APIRouter, Depends, FastAPI, HTTPException, Request, Response, status from pydantic import ValidationError from simple_module_hosting.permissions import RequiresPermission @@ -37,19 +37,38 @@ _DELETE = [Depends(RequiresPermission(PERM_DELETE))] -def _strip_mask_sentinels(changes: dict[str, Any]) -> dict[str, Any]: - """Drop any field whose submitted value is the UI mask sentinel. +def _masked_fields(app: FastAPI, package: str) -> frozenset[str]: + """The fields this package's editor rendered as dots. - Keyed on the sentinel alone rather than on ``is_secret_field(name)``: a - value can be masked because it *is* a credential (a DSN with a password in - it) on a field whose name says nothing of the sort, and storing the row of - dots that the editor rendered would overwrite the real connection string. - Nothing legitimately equals :data:`SECRET_MASK`. + ``ModuleSettingField.is_secret`` is the same flag that drove the input type + and the reveal affordance, so it answers exactly the question here: which + submitted values could be an echo of the mask rather than a real edit. + """ + return frozenset( + field.name + for view in collect_module_settings(app) + if view.package == package + for field in view.fields + if field.is_secret + ) + + +def _strip_mask_sentinels(masked: frozenset[str], changes: dict[str, Any]) -> dict[str, Any]: + """Drop the mask sentinel on the fields that were rendered masked. + + Not on ``is_secret_field(name)``: a value can be masked because it *is* a + credential (a DSN with a password in it) on a field whose name says nothing + of the sort, and storing the row of dots the editor rendered would overwrite + the real connection string. + + Not on the sentinel alone either, which is where this started — that silently + dropped the write on any field whose real value happened to equal + :data:`SECRET_MASK`, with no error and no saved row to show for it. """ return { name: value for name, value in changes.items() - if not (isinstance(value, str) and value == SECRET_MASK) + if not (name in masked and isinstance(value, str) and value == SECRET_MASK) } @@ -86,7 +105,7 @@ async def update_module( detail="This module has a dedicated settings page; edit it there instead.", ) - cleaned = _strip_mask_sentinels(changes) + cleaned = _strip_mask_sentinels(_masked_fields(request.app, package), changes) if not cleaned: return {"ok": True, "changed": []} diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index ea27408e..d06c468d 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -80,8 +80,8 @@ async def browse( service: SettingService = Depends(get_setting_service), scope: str = SCOPE_ALL, q: str = "", - page: str = "1", - per_page: str = str(DEFAULT_PER_PAGE), + page: browse_query.PageParam = 1, + per_page: browse_query.PerPageParam = DEFAULT_PER_PAGE, ) -> InertiaResponse: """The raw key/value store, filtered/searched/paged on the server. @@ -89,9 +89,12 @@ async def browse( "Settings" is nearly always after a module's form, not a table of rows keyed by dotted strings. - The filters are strings because they reach us from urls people edit and - bookmark; ``browse_query.parse`` substitutes defaults for anything - unusable rather than 422ing a link that used to work. + The filters reach us from urls people edit and bookmark, so nothing here + 422s a link that used to work: an unusable ``scope`` falls back to the + ``all`` tab, and the two page params carry a validator that substitutes + their default instead of rejecting the request. ``browse_query.parse`` + then clamps, so the ``filters``/``pagination`` props can echo exactly what + the query ran with. """ query = browse_query.parse(scope, q, page, per_page) items, total = await service.list_filtered( diff --git a/modules/settings/settings/known_keys.py b/modules/settings/settings/known_keys.py index b8ac63b1..0e1665d3 100644 --- a/modules/settings/settings/known_keys.py +++ b/modules/settings/settings/known_keys.py @@ -19,19 +19,13 @@ from fastapi.encoders import jsonable_encoder from settings._module_settings import collect_module_settings +from settings._secrets import conceals_secret, mask -_DEFINITION_MODULE = "Registry" -"""Module label for keys declared through ``SettingsRegistry`` rather than a -pydantic settings class. They have no owning package to name — the registry -records intent, not a field on some module's settings object.""" - -def _from_field(package: str, module_name: str, field) -> dict[str, Any]: +def _from_field(package: str, field) -> dict[str, Any]: return { "key": f"{package}.{field.name}", "type": field.type, - "description": field.description, - "module": module_name, "env_var": field.env_var, # See ``ModuleSettingField.env_readable``: a bundled module's SM_* var # is a label, not a fallback, and the panel must say so. @@ -46,18 +40,27 @@ def _from_field(package: str, module_name: str, field) -> dict[str, Any]: def _from_definition(definition) -> dict[str, Any]: + """A registry declaration, masked on the same rule as a module field. + + ``_from_field`` receives a default that ``_module_settings`` has already + masked; this builder reads the declaration directly, so it has to apply the + rule itself. Hard-coding ``is_secret: False`` and passing ``default`` + straight through put the raw value into the New-override suggestion list, + which renders it verbatim. + """ + value_type = str(definition.value_type) + default = jsonable_encoder(definition.default) + secret = conceals_secret(definition.key, default, value_type) return { "key": definition.key, - "type": str(definition.value_type), - "description": definition.description, - "module": _DEFINITION_MODULE, + "type": value_type, "env_var": "", "env_readable": False, "env_set": False, "env_value": None, - "default": definition.default, + "default": mask(default) if secret else default, "requires_restart": False, - "is_secret": False, + "is_secret": secret, "choices": None, } @@ -72,7 +75,7 @@ def build(app: FastAPI) -> list[dict[str, Any]]: suggestions: dict[str, dict[str, Any]] = {} for view in collect_module_settings(app): for field in view.fields: - entry = _from_field(view.package, view.module_name, field) + entry = _from_field(view.package, field) suggestions[entry["key"]] = entry registry = getattr(getattr(app.state, "settings", None), "registry", None) diff --git a/modules/settings/settings/pages/components/KeyField.tsx b/modules/settings/settings/pages/components/KeyField.tsx index e8be929a..15e63e48 100644 --- a/modules/settings/settings/pages/components/KeyField.tsx +++ b/modules/settings/settings/pages/components/KeyField.tsx @@ -7,8 +7,6 @@ import type { ValueType } from '../types'; export interface KnownKey { key: string; type: string; - description: string; - module: string; /** `SM__` label for this field. */ env_var: string; /** The declaring class reads env at all (it declares an `env_prefix`). */ diff --git a/modules/settings/settings/service.py b/modules/settings/settings/service.py index 8d1a6a47..00285a3f 100644 --- a/modules/settings/settings/service.py +++ b/modules/settings/settings/service.py @@ -6,12 +6,11 @@ from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession +from settings._row_masking import drop_placeholder_write, is_placeholder_write, out from settings.constants import ( ALL_SCOPES, DEFAULT_PER_PAGE, SCOPE_ALL, - SENSITIVE_KEYS, - SENSITIVE_PLACEHOLDER, SYSTEM_SCOPE_ID, VALUE_TYPE_STRING, ) @@ -25,42 +24,6 @@ from settings.models import Setting -def _out(entity: Setting) -> SettingOut: - """Serialize a row, masking values that must not leave the service. - - Every read path funnels through here so a secret cannot be read back by - listing it, resolving it, or fetching it by id. The only masked key today - is the session-signing key the hosting layer persists at boot, and that - reader goes straight to SQL — so masking here costs the app nothing. - """ - out = SettingOut.model_validate(entity) - if out.key in SENSITIVE_KEYS: - return out.model_copy(update={"value": SENSITIVE_PLACEHOLDER}) - return out - - -def _is_placeholder_write(key: str, value: object) -> bool: - """Whether this write is the mask being echoed back, not a real new value. - - The admin edit form GETs the row, pre-fills its input from the response, - and PUTs it back. For a masked key that response carries ``"********"``, so - an admin who opens ``host.secret_key`` and clicks Save — without touching - the field — would otherwise overwrite the session-signing key with a fixed, - publicly-known string, silently invalidating every session and making every - future cookie forgeable. - - Treated as "leave it alone" rather than rejected, so the rest of the form - still saves and an admin who genuinely types a new key can still set one. - """ - return key in SENSITIVE_KEYS and value == SENSITIVE_PLACEHOLDER - - -def _drop_placeholder_write(key: str, changes: dict) -> None: - """Strip a masked-value echo out of an update payload, in place.""" - if "value" in changes and _is_placeholder_write(key, changes["value"]): - del changes["value"] - - class SettingService: """Async CRUD + scope resolution for key/value settings. @@ -77,7 +40,7 @@ async def list_all(self) -> list[SettingOut]: result = await self.db.execute( select(Setting).order_by(Setting.scope, Setting.scope_id, Setting.key) ) - return [_out(row) for row in result.scalars()] + return [out(row) for row in result.scalars()] async def list_filtered( self, @@ -104,7 +67,7 @@ async def list_filtered( .limit(per_page) ) result = await self.db.execute(stmt) - return [_out(row) for row in result.scalars()], int(total or 0) + return [out(row) for row in result.scalars()], int(total or 0) async def count_by_scope(self, q: str | None = None) -> dict[str, int]: """Per-scope tallies for the filter tabs, plus ``all``. @@ -141,13 +104,33 @@ def _filter_conditions(scope: SettingScope | None, q: str | None) -> list: async def list_by_scope( self, scope: SettingScope, scope_id: str = SYSTEM_SCOPE_ID ) -> list[SettingOut]: - stmt = ( + result = await self.db.execute(self._scope_stmt(scope, scope_id)) + return [out(row) for row in result.scalars()] + + async def list_by_scope_unmasked( + self, scope: SettingScope, scope_id: str = SYSTEM_SCOPE_ID + ) -> list[SettingOut]: + """The same rows with their real values, for code that *applies* them. + + The masking in :func:`_out` is for the screens. Hydration is not a + screen: ``SettingsStore`` feeds these values back into the live module + settings objects at boot, so a masked read writes a row of dots over the + real secret — a mailer that cannot authenticate, and a + ``reset_password_token_secret`` that no longer verifies the tokens it + signed. This is the one read that must see through the mask, and it is + spelled out rather than reached by passing a flag so that every caller + of it is one grep away. + """ + result = await self.db.execute(self._scope_stmt(scope, scope_id)) + return [SettingOut.model_validate(row) for row in result.scalars()] + + @staticmethod + def _scope_stmt(scope: SettingScope, scope_id: str): + return ( select(Setting) .where(Setting.scope == scope.value, Setting.scope_id == scope_id) .order_by(Setting.key) ) - result = await self.db.execute(stmt) - return [_out(row) for row in result.scalars()] # ── Lookup ────────────────────────────────────────────────────── @@ -155,28 +138,38 @@ async def get_by_id(self, setting_id: int) -> SettingOut | None: entity = await self.db.get(Setting, setting_id) if entity is None: return None - return _out(entity) + return out(entity) async def get_scoped(self, scope: SettingScope, scope_id: str, key: str) -> SettingOut | None: entity = await self._find(scope, scope_id, key) - return _out(entity) if entity is not None else None + return out(entity) if entity is not None else None - async def resolve( + async def _resolve_entity( self, key: str, user_id: str | None = None, tenant_id: str | None = None, - ) -> SettingOut | None: + ) -> Setting | None: + """First match walking USER > TENANT > SYSTEM, unserialized.""" if user_id: entity = await self._find(SettingScope.USER, user_id, key) if entity is not None: - return _out(entity) + return entity if tenant_id: entity = await self._find(SettingScope.TENANT, tenant_id, key) if entity is not None: - return _out(entity) - entity = await self._find(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, key) - return _out(entity) if entity is not None else None + return entity + return await self._find(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, key) + + async def resolve( + self, + key: str, + user_id: str | None = None, + tenant_id: str | None = None, + ) -> SettingOut | None: + """The resolved row as the API returns it — masked if it is a secret.""" + entity = await self._resolve_entity(key, user_id=user_id, tenant_id=tenant_id) + return out(entity) if entity is not None else None async def get_resolved_value( self, @@ -185,8 +178,16 @@ async def get_resolved_value( tenant_id: str | None = None, default: str | None = None, ) -> str | None: - found = await self.resolve(key, user_id=user_id, tenant_id=tenant_id) - return found.value if found is not None else default + """The resolved *value*, unmasked — this is what consumers act on. + + ``SettingsAccessor`` is the read path other modules use to get a value + they are about to use, not one they are about to render. Masking here + would hand a module the placeholder instead of its own API key. The + masked view of the same row is :meth:`resolve`, which is what the API + returns. + """ + entity = await self._resolve_entity(key, user_id=user_id, tenant_id=tenant_id) + return entity.value if entity is not None else default # ── Mutations ─────────────────────────────────────────────────── @@ -195,19 +196,19 @@ async def create(self, data: SettingCreate) -> SettingOut: self.db.add(entity) await self.db.flush() await self.db.refresh(entity) - return _out(entity) + return out(entity) async def update(self, setting_id: int, data: SettingUpdate) -> SettingOut | None: entity = await self.db.get(Setting, setting_id) if entity is None: return None changes = data.model_dump(exclude_unset=True) - _drop_placeholder_write(entity.key, changes) + drop_placeholder_write(entity, changes) for field, value in changes.items(): setattr(entity, field, value) await self.db.flush() await self.db.refresh(entity) - return _out(entity) + return out(entity) async def upsert_scoped( self, @@ -230,7 +231,7 @@ async def upsert_scoped( ) self.db.add(entity) else: - if not _is_placeholder_write(key, data.value): + if not is_placeholder_write(entity, data.value): entity.value = data.value if data.value_type is not None: entity.value_type = data.value_type.value @@ -239,7 +240,7 @@ async def upsert_scoped( entity.description = data.description await self.db.flush() await self.db.refresh(entity) - return _out(entity) + return out(entity) async def delete(self, setting_id: int) -> bool: entity = await self.db.get(Setting, setting_id) diff --git a/modules/settings/settings/store.py b/modules/settings/settings/store.py index 9940bdc0..7618ff40 100644 --- a/modules/settings/settings/store.py +++ b/modules/settings/settings/store.py @@ -2,6 +2,13 @@ Wraps the existing SettingService. Keys are namespaced ``.`` to avoid collision with free-form user-defined setting keys. + +Reads are deliberately *unmasked*. This store is what applies overrides to the +live settings objects at boot, not what renders them: the masking on the +service's ordinary read path is for the admin screens, and going through it here +would write a row of dots over every real credential the deployment had stored. +The display side masks independently, from the settings object it is given — +see ``_module_settings._field_view``. """ from __future__ import annotations @@ -24,7 +31,7 @@ def __init__(self, service: SettingService) -> None: async def get_overrides(self, package: str) -> dict[str, tuple[str, str]]: """Return ``{field_name: (raw_value, value_type)}`` for a package.""" prefix = f"{package}." - items = await self._service.list_by_scope(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + items = await self._service.list_by_scope_unmasked(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) out: dict[str, tuple[str, str]] = {} for item in items: if not item.key.startswith(prefix): @@ -42,7 +49,7 @@ async def all_override_fields(self) -> dict[str, frozenset[str]]: SYSTEM scope per package, so calling it in a loop over the installed modules is one full read per module for the same rows. """ - items = await self._service.list_by_scope(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + items = await self._service.list_by_scope_unmasked(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) out: dict[str, set[str]] = {} for item in items: package, sep, field_name = item.key.partition(".") @@ -65,7 +72,7 @@ async def clear_override(self, package: str, field: str) -> None: ) async def list_packages_with_overrides(self) -> list[str]: - items = await self._service.list_by_scope(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + items = await self._service.list_by_scope_unmasked(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) pkgs: set[str] = set() for item in items: if "." not in item.key: diff --git a/modules/settings/tests/test_known_keys_meta.py b/modules/settings/tests/test_known_keys_meta.py index 9c6d5f39..a1bae926 100644 --- a/modules/settings/tests/test_known_keys_meta.py +++ b/modules/settings/tests/test_known_keys_meta.py @@ -37,8 +37,6 @@ async def test_every_suggestion_carries_the_full_meta_shape( expected = { "key", "type", - "description", - "module", "env_var", "env_set", "env_readable", @@ -124,7 +122,6 @@ async def test_a_declared_key_is_suggested( entry = _by_key(await _known_keys(authenticated_client), "orders.checkout.require_terms") assert entry["default"] == "true" - assert entry["description"] == "Show the terms checkbox on checkout." assert entry["choices"] is None assert entry["env_readable"] is False diff --git a/modules/settings/tests/test_settings_secret_urls.py b/modules/settings/tests/test_settings_secret_urls.py index 2006f615..a7cd7847 100644 --- a/modules/settings/tests/test_settings_secret_urls.py +++ b/modules/settings/tests/test_settings_secret_urls.py @@ -17,6 +17,7 @@ _INERTIA = {"X-Inertia": "true", "X-Inertia-Version": "1.0"} _CREATE = "/admin/settings/create" +_WITH_PASSWORD = "postgresql://svc:hunter2@db.internal/app" class _Dsns(BaseSettings): @@ -111,9 +112,51 @@ class TestMaskSentinelIsNeverStored: def test_a_masked_dsn_echoed_back_is_dropped(self): """The editor renders the mask; submitting it must not overwrite the real DSN with a row of dots. The name rule does not know - ``broker_url`` is a secret, so the sentinel has to be the signal.""" + ``broker_url`` is a secret, so the field's rendered ``is_secret`` is + what says the sentinel could be an echo.""" from settings.endpoints.module_api import _strip_mask_sentinels - assert _strip_mask_sentinels({"broker_url": SECRET_MASK, "queue": "celery"}) == { + masked = frozenset({"broker_url"}) + assert _strip_mask_sentinels(masked, {"broker_url": SECRET_MASK, "queue": "celery"}) == { "queue": "celery" } + + def test_the_sentinel_is_stored_on_a_field_that_was_never_masked(self): + """A field the editor rendered in clear text is not echoing anything, so + a value that happens to equal the mask is a real edit. Dropping it on the + sentinel alone silently discarded the write with nothing to show for it.""" + from settings.endpoints.module_api import _strip_mask_sentinels + + changes = {"queue": SECRET_MASK} + assert _strip_mask_sentinels(frozenset(), changes) == changes + + +class TestContainersAreWalked: + """A DSN does not stop being a credential for sitting inside a list. + + ``embeds_credential`` returned False for anything that was not a ``str``, + so a ``list[str]`` of broker URLs or a ``dict`` of per-tenant DSNs would + have been rendered in full. + """ + + def test_a_dsn_in_a_list_is_a_credential(self): + from settings._secrets import embeds_credential + + assert embeds_credential(["redis://localhost:6379/0", _WITH_PASSWORD]) is True + + def test_a_dsn_in_a_dict_value_is_a_credential(self): + from settings._secrets import embeds_credential + + assert embeds_credential({"primary": _WITH_PASSWORD}) is True + + def test_a_nested_container_is_walked(self): + from settings._secrets import embeds_credential + + assert embeds_credential({"shards": [{"dsn": _WITH_PASSWORD}]}) is True + + def test_password_free_containers_stay_visible(self): + from settings._secrets import embeds_credential + + assert embeds_credential(["redis://localhost:6379/0"]) is False + assert embeds_credential({"n": 5, "flag": True, "url": "https://example.com"}) is False + assert embeds_credential(None) is False diff --git a/modules/settings/tests/test_store_filters.py b/modules/settings/tests/test_store_filters.py index f361998f..23b61162 100644 --- a/modules/settings/tests/test_store_filters.py +++ b/modules/settings/tests/test_store_filters.py @@ -198,3 +198,29 @@ async def test_counts_by_scope_names_every_scope_even_at_zero(self, db_session) counts = await SettingService(db_session).count_by_scope() assert counts == {"all": 1, "system": 1, "tenant": 0, "user": 0} + + +class TestPaginationSchema: + """Lenient parsing, honest OpenAPI. + + ``page``/``per_page`` were typed ``str`` so a bookmarked link with a stale + value would never 422. The intent is right; the side effect was a schema + advertising page numbers as strings to every generated client. + """ + + async def test_the_schema_advertises_integers( + self, app: FastAPI, authenticated_client: httpx.AsyncClient + ) -> None: + schema = (await authenticated_client.get("/openapi.json")).json() + params = {p["name"]: p["schema"] for p in schema["paths"][_STORE]["get"]["parameters"]} + assert params["page"]["type"] == "integer" + assert params["per_page"]["type"] == "integer" + + async def test_an_unparseable_page_still_renders( + self, seeded: FastAPI, authenticated_client: httpx.AsyncClient + ) -> None: + """A link someone edited by hand falls back to page 1 rather than 422ing.""" + props = await _props(authenticated_client, "?page=banana&per_page=nonsense") + + assert props["pagination"]["page"] == 1 + assert props["settings"] diff --git a/modules/settings/tests/test_store_secret_masking.py b/modules/settings/tests/test_store_secret_masking.py new file mode 100644 index 00000000..3ab5c4c5 --- /dev/null +++ b/modules/settings/tests/test_store_secret_masking.py @@ -0,0 +1,231 @@ +"""The raw key/value store hides credentials on the same rule as the editor. + +``SettingService._out`` masked only exact matches against ``SENSITIVE_KEYS``, +a single-entry frozenset holding ``host.secret_key``. The store screen shows +the *same data* as the module editor through a different lens, so an override +named ``users.smtp_password`` — or any override holding a DSN with a password +in its authority — rendered in clear text in the browse table and pre-filled +into the edit form, for anyone holding ``settings.view``. +""" + +from __future__ import annotations + +import httpx +import pytest +from fastapi import FastAPI +from settings._row_masking import is_masked +from settings.constants import SENSITIVE_PLACEHOLDER, VALUE_TYPE_INT, VALUE_TYPE_STRING +from settings.contracts.schemas import SettingCreate, SettingScope +from settings.models import Setting +from settings.service import SettingService + +_DSN = "postgresql://svc:hunter2@db.internal/app" + + +def _row(key: str, value: str, value_type: str = VALUE_TYPE_STRING) -> Setting: + return Setting( + scope=SettingScope.SYSTEM.value, scope_id="", key=key, value=value, value_type=value_type + ) + + +class TestWhatCountsAsMasked: + @pytest.mark.parametrize( + "key,value", + [ + ("host.secret_key", "s3kr3t"), + ("users.smtp_password", "hunter2"), + ("users.mailer_api_key", "sk-live-abc"), + ("background_tasks.broker_url", _DSN), + ], + ) + def test_credentials_are_masked(self, key: str, value: str) -> None: + assert is_masked(_row(key, value)) is True + + @pytest.mark.parametrize( + "key,value,value_type", + [ + # A DSN with no password in it is not a credential — hiding + # `redis://localhost:6379/0` helps nobody debug an idle queue. + ("background_tasks.broker_url", "redis://localhost:6379/0", VALUE_TYPE_STRING), + ("users.smtp_host", "smtp.example.com", VALUE_TYPE_STRING), + # A numeric field matching "password" by name only. Masking this + # made a lifetime uneditable in the module editor; the store must + # not reintroduce that. + ("users.reset_password_token_lifetime_seconds", "3600", VALUE_TYPE_INT), + ], + ) + def test_ordinary_values_stay_visible(self, key: str, value: str, value_type: str) -> None: + assert is_masked(_row(key, value, value_type)) is False + + +class TestReadPathsMask: + async def test_browse_table_masks_a_password_override(self, db_session) -> None: + service = SettingService(db_session) + await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="users.smtp_password", + value="hunter2", + value_type=VALUE_TYPE_STRING, + ) + ) + items, _ = await service.list_filtered() + assert [i.value for i in items] == [SENSITIVE_PLACEHOLDER] + + async def test_get_by_id_masks_a_dsn_override(self, db_session) -> None: + service = SettingService(db_session) + created = await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="background_tasks.broker_url", + value=_DSN, + value_type=VALUE_TYPE_STRING, + ) + ) + assert created.value == SENSITIVE_PLACEHOLDER + fetched = await service.get_by_id(created.id) + assert fetched is not None + assert fetched.value == SENSITIVE_PLACEHOLDER + + async def test_the_store_screen_masks_it_too( + self, app: FastAPI, authenticated_client: httpx.AsyncClient + ) -> None: + """End to end: the browse table an admin actually reads.""" + async with app.state.sm.db.session_factory() as session: + session.add(_row("users.smtp_password", "hunter2")) + session.add(_row("background_tasks.broker_url", _DSN)) + session.add(_row("users.smtp_host", "smtp.example.com")) + await session.commit() + + resp = await authenticated_client.get( + "/admin/settings/store", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"} + ) + assert resp.status_code == 200 + rows = {r["key"]: r["value"] for r in resp.json()["props"]["settings"]} + + assert rows["users.smtp_password"] == SENSITIVE_PLACEHOLDER + assert rows["background_tasks.broker_url"] == SENSITIVE_PLACEHOLDER + assert rows["users.smtp_host"] == "smtp.example.com" + + +class TestSavingAnUntouchedForm: + async def test_echoing_the_mask_leaves_the_real_value_alone(self, db_session) -> None: + """The edit form pre-fills from the masked read. An admin who opens a + credential row and clicks Save without touching the field must not + overwrite it with the placeholder.""" + from settings.contracts.schemas import SettingUpdate + + service = SettingService(db_session) + created = await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="users.smtp_password", + value="hunter2", + value_type=VALUE_TYPE_STRING, + ) + ) + await service.update( + created.id, SettingUpdate(value=SENSITIVE_PLACEHOLDER, description="note") + ) + + stored = await db_session.get(Setting, created.id) + assert stored.value == "hunter2" + assert stored.description == "note" + + async def test_a_real_new_credential_still_saves(self, db_session) -> None: + from settings.contracts.schemas import SettingUpdate + + service = SettingService(db_session) + created = await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="users.smtp_password", + value="hunter2", + value_type=VALUE_TYPE_STRING, + ) + ) + await service.update(created.id, SettingUpdate(value="correct-horse")) + + stored = await db_session.get(Setting, created.id) + assert stored.value == "correct-horse" + + async def test_the_placeholder_is_stored_on_a_row_that_is_not_masked(self, db_session) -> None: + """Nothing was hidden on this row, so the write is a real edit rather + than an echo — dropping it would discard it with nothing to show.""" + from settings.contracts.schemas import SettingUpdate + + service = SettingService(db_session) + created = await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="ui.divider", + value="—", + value_type=VALUE_TYPE_STRING, + ) + ) + await service.update(created.id, SettingUpdate(value=SENSITIVE_PLACEHOLDER)) + + stored = await db_session.get(Setting, created.id) + assert stored.value == SENSITIVE_PLACEHOLDER + + +class TestTheMaskStopsAtTheScreen: + """Masking is for the screens. The code that *applies* a setting has to see + the real value, or hydration writes a row of dots over every credential the + deployment stored — a mailer that cannot authenticate, and a + ``reset_password_token_secret`` that no longer verifies the tokens it signed. + """ + + async def test_the_store_reads_through_the_mask(self, db_session) -> None: + from settings.constants import SYSTEM_SCOPE_ID + from settings.store import SettingsStore + + service = SettingService(db_session) + await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id=SYSTEM_SCOPE_ID, + key="users.reset_password_token_secret", + value="s3kr3t", + value_type=VALUE_TYPE_STRING, + ) + ) + overrides = await SettingsStore(service).get_overrides("users") + + assert overrides["reset_password_token_secret"] == ("s3kr3t", VALUE_TYPE_STRING) + + async def test_a_consumer_module_reads_the_real_value(self, db_session) -> None: + """``SettingsAccessor.get`` is what another module calls to obtain a + value it is about to use, not to render.""" + service = SettingService(db_session) + await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="orders.stripe_api_key", + value="sk-live-abc", + value_type=VALUE_TYPE_STRING, + ) + ) + assert await service.get_resolved_value("orders.stripe_api_key") == "sk-live-abc" + + async def test_the_resolve_endpoint_still_masks_the_same_row(self, db_session) -> None: + """The two readings of one row: the API renders, the consumer acts.""" + service = SettingService(db_session) + await service.create( + SettingCreate( + scope=SettingScope.SYSTEM, + scope_id="", + key="orders.stripe_api_key", + value="sk-live-abc", + value_type=VALUE_TYPE_STRING, + ) + ) + resolved = await service.resolve("orders.stripe_api_key") + assert resolved is not None + assert resolved.value == SENSITIVE_PLACEHOLDER