fix(settings): mask credentials on the raw store and the suggestion list - #306
Merged
Merged
Conversation
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
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying simple-module-python with
|
| Latest commit: |
651c3ad
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fd0aa72e.simple-module-python.pages.dev |
| Branch Preview URL: | https://fix-settings-credential-mask.simple-module-python.pages.dev |
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.
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.
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.
Closes #293
The problem
The hi-fi work taught the module-settings editor to hide credentials by value as well as by name —
embeds_credentialmarks a DSN with a password in its authority as secret, and_module_settingsmasks value, default and env reading on that rule. The other two screens showing the same data never got the rule.SettingService._outmasked only exact matches againstSENSITIVE_KEYS, the single-entry frozenset{"host.secret_key"}. An override namedusers.smtp_password, or any override holding apostgresql://user:pw@host/db, rendered in clear text in the browse table and pre-filled into the edit form — for anyone holdingsettings.view.known_keys._from_definitionbuilt an unmaskeddefault. It hard-coded"is_secret": Falseand passeddefinition.defaultstraight through, where its sibling_from_fieldmasks it. The New-override suggestion list renders that default verbatim. Inert today because no registry declaration ships a credential default, but it is one declaration away.The fix
The shared rule moves into
_secretsasis_named_secret(name, value_type)/conceals_secret(name, value, value_type), and all three read paths run through it —_module_settingsnow calls the helpers it used to inline.The store's placeholder-echo guard (
_is_placeholder_write) keys on whether the stored row is masked rather than on the one allowlisted key, so an admin who opens a credential row and clicks Save without touching the field still leaves the real value alone — on every credential row now, not justhost.secret_key. A row that was never masked stores the placeholder as a real edit.Plus the four smaller items from the issue:
embeds_credentialstronly_strip_mask_sentinelsModuleSettingField.is_secret)KnownKey.module/.descriptionpage/per_pagestrso a bookmarked link never 422s — schema advertised stringsintwith aBeforeValidatorthat substitutes the default;?page=bananastill renders, schema says integerVerification
uv run pytest modules/settings— 217 passed (24 new)modules/settings/tests/test_store_secret_masking.py: revertingis_maskedto the old single-key allowlist fails 7 of its 13 tests, including the end-to-end browse-table render. Restored, all pass.ruff format --check/ruff check/ty check modules/settings— passnpx biome check modules/settings,npx tsc -p modules/settings --noEmit,npx vitest run modules/settings(17 passed) — passnode scripts/check_untranslated_strings.mjs,scripts/check_file_size.py— passNot done here
The issue's masking rule is still name/value heuristics rather than a declared
secret=Trueon the field. Widening that is a bigger change to the settings contract and is not in scope here.