fix(settings): mask credentials on the raw store and the suggestion list

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 —

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"]

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:

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)),
     )

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": []}
 

modules/settings/settings/endpoints/views.py

@@ -80,18 +80,21 @@ 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.
 
     Moved off the section root: it is a database view, and an admin who clicks
     "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(

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)

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_<PACKAGE>_<FIELD>` label for this field. */
   env_var: string;
   /** The declaring class reads env at all (it declares an `env_prefix`). */