Skip to content

Commit e913e03

Browse files
antosubashantosubash
authored
fix(settings): mask credentials on the raw store and the suggestion list (#306)
* fix(settings): mask credentials on the raw store and the suggestion list The hi-fi work taught the module-settings editor to hide credentials by value as well as by name — `embeds_credential` marks a DSN with a password in its authority as secret, and `_module_settings` masks value, default and env reading on that rule. The other two screens showing the same data never got it. `SettingService._out` masked only exact matches against `SENSITIVE_KEYS`, a single-entry frozenset holding `host.secret_key`. So an override named `users.smtp_password`, or any override holding a `postgresql://user:pw@host/db`, rendered in clear text in the browse table and pre-filled into the edit form for anyone holding `settings.view`. And `known_keys._from_definition` hard-coded `is_secret: False` and passed `definition.default` straight through, where its sibling `_from_field` masks it — inert only because no registry declaration ships a credential default today. Lift the shared rule into `_secrets` as `is_named_secret` / `conceals_secret`, and run all three read paths through it. The store's placeholder-echo guard now keys on whether the stored row is masked rather than on the one allowlisted key, so saving an untouched form still leaves the real value alone — on every credential row, not just `host.secret_key`. Four smaller things in the same area: - `embeds_credential` walks lists and dict values. A `list[str]` of broker URLs holds the same material as the bare string; returning False for anything non-`str` would have shown one in full. - `_strip_mask_sentinels` takes the set of fields the editor actually rendered masked. Keying on the sentinel alone silently dropped the write on any field whose real value happened to be eight bullets. - `known_keys` no longer ships `module` and `description`. Both are declared on the `KnownKey` interface and neither is rendered. - The store's `page`/`per_page` are typed `int` with a `BeforeValidator` that substitutes the default. A bookmarked `?page=banana` still renders rather than 422ing, but the OpenAPI schema stops advertising page numbers as strings. Closes #293 * fix(settings): keep the mask on the screens, not on the value CI caught what the settings-only test run did not: masking in ``SettingService._out`` reaches every consumer of the service, and two of them are not screens. ``SettingsStore`` is what applies overrides to the live settings objects at boot, so a masked read wrote ``"********"`` over each stored credential — which turned up as five ``users`` auth-screen failures, a ``reset_password_token_secret`` that no longer verified the tokens it had signed. And ``SettingsAccessor.get`` — the documented read path other modules use — would have handed a module the placeholder instead of its own API key. Split the two readings of a row. ``list_by_scope_unmasked`` serves the store, and ``get_resolved_value`` reads the entity directly; both are spelled out rather than reached by a flag, so every caller that sees through the mask is one grep away. ``resolve`` still returns the masked view, because that is what ``/api/settings/resolve`` renders. * refactor(settings): move row masking out of service into its own module Adding the unmasked read paths pushed ``service.py`` to 325 lines, past the 300-line cap. Split on the seam that was already there: the four module-level functions answering "may this row's value be shown, and is this write the mask being echoed back?" move to ``_row_masking``, next to the field-level rules in ``_secrets`` they delegate to. ``SettingService`` keeps the querying.
1 parent c7e4b7a commit e913e03

14 files changed

Lines changed: 569 additions & 126 deletions

‎modules/settings/settings/_module_settings.py‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@
1919
from settings._secrets import ( # noqa: F401
2020
_NEVER_SECRET_TYPES,
2121
SECRET_MASK,
22+
conceals_secret,
2223
embeds_credential,
24+
is_named_secret,
2325
is_secret_field,
2426
mask,
2527
)
@@ -129,14 +131,7 @@ def _field_view(
129131
info = cls.model_fields[name]
130132
raw_value = getattr(settings, name)
131133
value_type = value_type_for_field(cls, name)
132-
# A numeric field whose name merely contains a secret-ish word was being
133-
# masked and made uneditable — `reset_password_token_lifetime_seconds` is
134-
# an int, but it matches on "password" exactly as the real secrets do.
135-
# Phrased as "exempt the types that cannot hold a credential" rather than
136-
# "mask only strings" so the failure direction is safe: an unexpected type
137-
# (e.g. `str | None`, which resolves to "json") stays masked instead of
138-
# silently exposing a secret.
139-
named_secret = value_type not in _NEVER_SECRET_TYPES and is_secret_field(name)
134+
named_secret = is_named_secret(name, value_type)
140135
extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {}
141136
default = resolve_default(info)
142137
env_var = f"{prefix}{name.upper()}"
@@ -147,10 +142,7 @@ def _field_view(
147142
raw_env = os.environ[live_env_var] if env_set and live_env_var is not None else None
148143

149144
def hidden(value: Any) -> bool:
150-
"""Mask per *value*, not just per name: a DSN carrying a password is a
151-
secret even on a field called ``broker_url``, while the same field's
152-
password-free default is still worth showing."""
153-
return named_secret or embeds_credential(value)
145+
return conceals_secret(name, value, value_type)
154146

155147
# ``is_secret`` drives the editor's input type and the "reveal" affordance,
156148
# so it has to be true when *any* of the three readings is being hidden —

‎modules/settings/settings/_row_masking.py‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
"""Whether a stored row's value may be shown, and how the mask is honoured.
2+
3+
Split out of ``service`` so the "is this a secret, and is this write the mask
4+
being echoed back?" question lives in one file, next to the field-level rules in
5+
``_secrets`` that it delegates to. ``service`` imports these; nothing else
6+
should need to, because the point of funnelling every read through ``out`` is
7+
that there is no second way to serialize a row.
8+
"""
9+
10+
from __future__ import annotations
11+
12+
from settings._secrets import conceals_secret
13+
from settings.constants import SENSITIVE_KEYS, SENSITIVE_PLACEHOLDER
14+
from settings.contracts.schemas import SettingOut
15+
from settings.models import Setting
16+
17+
18+
def is_masked(entity: Setting) -> bool:
19+
"""Whether this row's stored value must not leave the service.
20+
21+
:data:`SENSITIVE_KEYS` names the one key the hosting layer owns
22+
(``host.secret_key``). Everything else is judged by the same name/value
23+
rule the module editor uses, because the store is the *same data* seen
24+
through a different screen: an override named ``users.smtp_password``, or
25+
any override holding a ``postgresql://user:pw@host/db``, was rendered in
26+
clear text in the browse table and pre-filled into the edit form purely
27+
because the allowlist had a single entry in it.
28+
"""
29+
return entity.key in SENSITIVE_KEYS or conceals_secret(
30+
entity.key, entity.value, entity.value_type
31+
)
32+
33+
34+
def out(entity: Setting) -> SettingOut:
35+
"""Serialize a row, masking values that must not leave the service.
36+
37+
Every read path funnels through here so a secret cannot be read back by
38+
listing it, resolving it, or fetching it by id.
39+
"""
40+
out = SettingOut.model_validate(entity)
41+
if is_masked(entity):
42+
return out.model_copy(update={"value": SENSITIVE_PLACEHOLDER})
43+
return out
44+
45+
46+
def is_placeholder_write(entity: Setting, value: object) -> bool:
47+
"""Whether this write is the mask being echoed back, not a real new value.
48+
49+
The admin edit form GETs the row, pre-fills its input from the response,
50+
and PUTs it back. For a masked row that response carries ``"********"``, so
51+
an admin who opens the row and clicks Save — without touching the field —
52+
would otherwise overwrite a real credential with a fixed, publicly-known
53+
string. On ``host.secret_key`` that silently invalidates every session and
54+
makes every future cookie forgeable.
55+
56+
Gated on the stored row rather than on the sentinel alone, so a non-secret
57+
row whose real value happens to be eight asterisks still saves.
58+
59+
Treated as "leave it alone" rather than rejected, so the rest of the form
60+
still saves and an admin who genuinely types a new value can still set one.
61+
"""
62+
return is_masked(entity) and value == SENSITIVE_PLACEHOLDER
63+
64+
65+
def drop_placeholder_write(entity: Setting, changes: dict) -> None:
66+
"""Strip a masked-value echo out of an update payload, in place."""
67+
if "value" in changes and is_placeholder_write(entity, changes["value"]):
68+
del changes["value"]

‎modules/settings/settings/_secrets.py‎

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,34 @@ def is_secret_field(name: str) -> bool:
3030
return bool(_SECRET_PATTERNS.search(name))
3131

3232

33+
def is_named_secret(name: str, value_type: str | None = None) -> bool:
34+
"""True if the *name* marks this field as credential material.
35+
36+
``value_type`` exempts the types that cannot hold one, so a numeric field
37+
whose name merely contains a secret-ish word is not masked and made
38+
uneditable — ``reset_password_token_lifetime_seconds`` is an int, but it
39+
matches on "password" exactly as the real secrets do. Phrased as "exempt
40+
the types that cannot hold a credential" rather than "mask only strings" so
41+
the failure direction is safe: an unexpected type (``str | None`` resolves
42+
to "json") stays masked instead of silently exposing a secret.
43+
"""
44+
if value_type in _NEVER_SECRET_TYPES:
45+
return False
46+
return is_secret_field(name)
47+
48+
49+
def conceals_secret(name: str, value: Any, value_type: str | None = None) -> bool:
50+
"""True if this name/value pair must be masked on the way out.
51+
52+
The one rule every read path shares: mask per *value* as well as per name,
53+
so a DSN carrying a password is hidden even on a field called
54+
``broker_url``, while that field's password-free default is still shown.
55+
"""
56+
return is_named_secret(name, value_type) or embeds_credential(value)
57+
58+
3359
def embeds_credential(value: Any) -> bool:
34-
"""True if ``value`` is a URL carrying a password in its authority.
60+
"""True if ``value`` carries a password in a URL authority.
3561
3662
The name rule alone cannot see these: ``broker_url``, ``result_backend``,
3763
``redis_url`` and ``database_url`` match nothing in
@@ -43,15 +69,26 @@ def embeds_credential(value: Any) -> bool:
4369
Judged on the value rather than the name because the name is the thing
4470
that was wrong. A DSN without a password stays visible: hiding
4571
``redis://localhost:6379/0`` helps nobody debug why the queue is idle.
72+
73+
Containers are walked rather than dismissed: a ``list[str]`` of broker
74+
URLs or a ``dict`` of per-tenant DSNs holds exactly the same material as
75+
the bare string, and returning False for anything non-``str`` meant one
76+
such field would have been shown in full.
4677
"""
47-
if not isinstance(value, str) or "://" not in value:
48-
return False
49-
try:
50-
return bool(urlsplit(value).password)
51-
except ValueError:
52-
# A malformed authority (an unclosed IPv6 literal, say) is not a
53-
# credential, but it must not take the settings screen down either.
54-
return False
78+
if isinstance(value, str):
79+
if "://" not in value:
80+
return False
81+
try:
82+
return bool(urlsplit(value).password)
83+
except ValueError:
84+
# A malformed authority (an unclosed IPv6 literal, say) is not a
85+
# credential, but it must not take the settings screen down either.
86+
return False
87+
if isinstance(value, (list, tuple, set, frozenset)):
88+
return any(embeds_credential(item) for item in value)
89+
if isinstance(value, dict):
90+
return any(embeds_credential(item) for item in value.values())
91+
return False
5592

5693

5794
def mask(value: Any) -> Any:

‎modules/settings/settings/browse_query.py‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@
1111
from __future__ import annotations
1212

1313
from dataclasses import dataclass
14+
from typing import Annotated, Any
15+
16+
from pydantic import BeforeValidator
1417

1518
from settings.constants import ALL_SCOPES, DEFAULT_PER_PAGE, MAX_PER_PAGE, SCOPE_ALL
1619
from settings.contracts.schemas import SettingScope
@@ -32,18 +35,33 @@ def scope_filter(self) -> SettingScope | None:
3235
return None if self.scope == SCOPE_ALL else SettingScope(self.scope)
3336

3437

35-
def _int_or(raw: str, fallback: int) -> int:
36-
try:
37-
return int(raw)
38-
except (TypeError, ValueError):
39-
return fallback
38+
def _lenient_int(fallback: int) -> BeforeValidator:
39+
"""Coerce a query param to ``int``, substituting ``fallback`` for garbage.
40+
41+
Declared as a validator on an ``int``-annotated param rather than by typing
42+
the param ``str``: both accept ``?page=banana`` without a 422, but only this
43+
one leaves the OpenAPI schema advertising an integer. A generated client
44+
that reads the schema was otherwise told to send page numbers as strings.
45+
"""
46+
47+
def coerce(raw: Any) -> int:
48+
try:
49+
return int(raw)
50+
except (TypeError, ValueError):
51+
return fallback
52+
53+
return BeforeValidator(coerce)
54+
55+
56+
PageParam = Annotated[int, _lenient_int(1)]
57+
PerPageParam = Annotated[int, _lenient_int(DEFAULT_PER_PAGE)]
4058

4159

42-
def parse(scope: str, q: str, page: str, per_page: str) -> BrowseQuery:
60+
def parse(scope: str, q: str, page: int, per_page: int) -> BrowseQuery:
4361
"""Read the four query params, substituting defaults for anything unusable."""
4462
return BrowseQuery(
4563
scope=scope if scope in ALL_SCOPES else SCOPE_ALL,
4664
q=q,
47-
page=max(_int_or(page, 1), 1),
48-
per_page=max(1, min(_int_or(per_page, DEFAULT_PER_PAGE), MAX_PER_PAGE)),
65+
page=max(page, 1),
66+
per_page=max(1, min(per_page, MAX_PER_PAGE)),
4967
)

‎modules/settings/settings/endpoints/module_api.py‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import json
1010
from typing import Any
1111

12-
from fastapi import APIRouter, Depends, HTTPException, Request, Response, status
12+
from fastapi import APIRouter, Depends, FastAPI, HTTPException, Request, Response, status
1313
from pydantic import ValidationError
1414
from simple_module_hosting.permissions import RequiresPermission
1515

@@ -37,19 +37,38 @@
3737
_DELETE = [Depends(RequiresPermission(PERM_DELETE))]
3838

3939

40-
def _strip_mask_sentinels(changes: dict[str, Any]) -> dict[str, Any]:
41-
"""Drop any field whose submitted value is the UI mask sentinel.
40+
def _masked_fields(app: FastAPI, package: str) -> frozenset[str]:
41+
"""The fields this package's editor rendered as dots.
4242
43-
Keyed on the sentinel alone rather than on ``is_secret_field(name)``: a
44-
value can be masked because it *is* a credential (a DSN with a password in
45-
it) on a field whose name says nothing of the sort, and storing the row of
46-
dots that the editor rendered would overwrite the real connection string.
47-
Nothing legitimately equals :data:`SECRET_MASK`.
43+
``ModuleSettingField.is_secret`` is the same flag that drove the input type
44+
and the reveal affordance, so it answers exactly the question here: which
45+
submitted values could be an echo of the mask rather than a real edit.
46+
"""
47+
return frozenset(
48+
field.name
49+
for view in collect_module_settings(app)
50+
if view.package == package
51+
for field in view.fields
52+
if field.is_secret
53+
)
54+
55+
56+
def _strip_mask_sentinels(masked: frozenset[str], changes: dict[str, Any]) -> dict[str, Any]:
57+
"""Drop the mask sentinel on the fields that were rendered masked.
58+
59+
Not on ``is_secret_field(name)``: a value can be masked because it *is* a
60+
credential (a DSN with a password in it) on a field whose name says nothing
61+
of the sort, and storing the row of dots the editor rendered would overwrite
62+
the real connection string.
63+
64+
Not on the sentinel alone either, which is where this started — that silently
65+
dropped the write on any field whose real value happened to equal
66+
:data:`SECRET_MASK`, with no error and no saved row to show for it.
4867
"""
4968
return {
5069
name: value
5170
for name, value in changes.items()
52-
if not (isinstance(value, str) and value == SECRET_MASK)
71+
if not (name in masked and isinstance(value, str) and value == SECRET_MASK)
5372
}
5473

5574

@@ -86,7 +105,7 @@ async def update_module(
86105
detail="This module has a dedicated settings page; edit it there instead.",
87106
)
88107

89-
cleaned = _strip_mask_sentinels(changes)
108+
cleaned = _strip_mask_sentinels(_masked_fields(request.app, package), changes)
90109
if not cleaned:
91110
return {"ok": True, "changed": []}
92111

‎modules/settings/settings/endpoints/views.py‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -80,18 +80,21 @@ async def browse(
8080
service: SettingService = Depends(get_setting_service),
8181
scope: str = SCOPE_ALL,
8282
q: str = "",
83-
page: str = "1",
84-
per_page: str = str(DEFAULT_PER_PAGE),
83+
page: browse_query.PageParam = 1,
84+
per_page: browse_query.PerPageParam = DEFAULT_PER_PAGE,
8585
) -> InertiaResponse:
8686
"""The raw key/value store, filtered/searched/paged on the server.
8787
8888
Moved off the section root: it is a database view, and an admin who clicks
8989
"Settings" is nearly always after a module's form, not a table of rows
9090
keyed by dotted strings.
9191
92-
The filters are strings because they reach us from urls people edit and
93-
bookmark; ``browse_query.parse`` substitutes defaults for anything
94-
unusable rather than 422ing a link that used to work.
92+
The filters reach us from urls people edit and bookmark, so nothing here
93+
422s a link that used to work: an unusable ``scope`` falls back to the
94+
``all`` tab, and the two page params carry a validator that substitutes
95+
their default instead of rejecting the request. ``browse_query.parse``
96+
then clamps, so the ``filters``/``pagination`` props can echo exactly what
97+
the query ran with.
9598
"""
9699
query = browse_query.parse(scope, q, page, per_page)
97100
items, total = await service.list_filtered(

‎modules/settings/settings/known_keys.py‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,13 @@
1919
from fastapi.encoders import jsonable_encoder
2020

2121
from settings._module_settings import collect_module_settings
22+
from settings._secrets import conceals_secret, mask
2223

23-
_DEFINITION_MODULE = "Registry"
24-
"""Module label for keys declared through ``SettingsRegistry`` rather than a
25-
pydantic settings class. They have no owning package to name — the registry
26-
records intent, not a field on some module's settings object."""
2724

28-
29-
def _from_field(package: str, module_name: str, field) -> dict[str, Any]:
25+
def _from_field(package: str, field) -> dict[str, Any]:
3026
return {
3127
"key": f"{package}.{field.name}",
3228
"type": field.type,
33-
"description": field.description,
34-
"module": module_name,
3529
"env_var": field.env_var,
3630
# See ``ModuleSettingField.env_readable``: a bundled module's SM_* var
3731
# 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]:
4640

4741

4842
def _from_definition(definition) -> dict[str, Any]:
43+
"""A registry declaration, masked on the same rule as a module field.
44+
45+
``_from_field`` receives a default that ``_module_settings`` has already
46+
masked; this builder reads the declaration directly, so it has to apply the
47+
rule itself. Hard-coding ``is_secret: False`` and passing ``default``
48+
straight through put the raw value into the New-override suggestion list,
49+
which renders it verbatim.
50+
"""
51+
value_type = str(definition.value_type)
52+
default = jsonable_encoder(definition.default)
53+
secret = conceals_secret(definition.key, default, value_type)
4954
return {
5055
"key": definition.key,
51-
"type": str(definition.value_type),
52-
"description": definition.description,
53-
"module": _DEFINITION_MODULE,
56+
"type": value_type,
5457
"env_var": "",
5558
"env_readable": False,
5659
"env_set": False,
5760
"env_value": None,
58-
"default": definition.default,
61+
"default": mask(default) if secret else default,
5962
"requires_restart": False,
60-
"is_secret": False,
63+
"is_secret": secret,
6164
"choices": None,
6265
}
6366

@@ -72,7 +75,7 @@ def build(app: FastAPI) -> list[dict[str, Any]]:
7275
suggestions: dict[str, dict[str, Any]] = {}
7376
for view in collect_module_settings(app):
7477
for field in view.fields:
75-
entry = _from_field(view.package, view.module_name, field)
78+
entry = _from_field(view.package, field)
7679
suggestions[entry["key"]] = entry
7780

7881
registry = getattr(getattr(app.state, "settings", None), "registry", None)

‎modules/settings/settings/pages/components/KeyField.tsx‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ import type { ValueType } from '../types';
77
export interface KnownKey {
88
key: string;
99
type: string;
10-
description: string;
11-
module: string;
1210
/** `SM_<PACKAGE>_<FIELD>` label for this field. */
1311
env_var: string;
1412
/** The declaring class reads env at all (it declares an `env_prefix`). */

0 commit comments

Comments
 (0)