Skip to content

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

Merged
antosubash merged 3 commits into
mainfrom
fix/settings-credential-mask
Sep 5, 2026
Merged

antosubash merged 3 commits into
mainfrom
fix/settings-credential-mask

Conversation

@antosubash

Copy link
Copy Markdown
Owner

Closes #293

The problem

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 the rule.

  • The raw store showed everything. SettingService._out masked only exact matches against SENSITIVE_KEYS, the single-entry frozenset {"host.secret_key"}. 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.
  • known_keys._from_definition built an unmasked default. It hard-coded "is_secret": False and passed definition.default straight through, where its sibling _from_field masks 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 _secrets as is_named_secret(name, value_type) / conceals_secret(name, value, value_type), and all three read paths run through it — _module_settings now 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 just host.secret_key. A row that was never masked stores the placeholder as a real edit.

Plus the four smaller items from the issue:

before after
embeds_credential str only walks lists, tuples, sets and dict values
_strip_mask_sentinels keyed on the sentinel alone — silently dropped a write on any field whose real value was eight bullets keyed on the fields the editor rendered masked (ModuleSettingField.is_secret)
KnownKey.module / .description shipped on every suggestion, rendered nowhere dropped from the payload and the interface
store page/per_page typed str so a bookmarked link never 422s — schema advertised strings typed int with a BeforeValidator that substitutes the default; ?page=banana still renders, schema says integer

Verification

  • uv run pytest modules/settings217 passed (24 new)
  • New coverage in modules/settings/tests/test_store_secret_masking.py: reverting is_masked to 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 — pass
  • npx biome check modules/settings, npx tsc -p modules/settings --noEmit, npx vitest run modules/settings (17 passed) — pass
  • node scripts/check_untranslated_strings.mjs, scripts/check_file_size.py — pass

Not done here

The issue's masking rule is still name/value heuristics rather than a declared secret=True on the field. Widening that is a bigger change to the settings contract and is not in scope here.

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
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review Completed 2026-09-04T21:18:59.356594Z 770129b PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 4, 2026

Copy link
Copy Markdown

Deploying simple-module-python with  Cloudflare Pages  Cloudflare Pages

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

View logs

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.
@antosubash
antosubash merged commit e913e03 into main Sep 5, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Settings raw store and known-keys list return credential values unmasked

1 participant