fix: settings-mutating routes preserve masked secrets (C2 + H4 + M3) (#224)

Internal security audit (May 2026): three findings, one root cause.
PR #215 added `_strip_masked_values` + `_deep_merge_dict` on the
central `/api/web/settings` POST path so a round-trip POST of the
masked GET response did not overwrite the on-disk credential with
the literal sentinel. The audit then found two OTHER mutation routes
that bypassed that pipeline.

### C2 (CRITICAL) — POST /api/storage/config

`StorageBackendConfig.post` wholesale-set the per-backend dict:

    storage['azure_keyvault'] = data.get('azure_keyvault', {})

When the UI POSTed back the masked GET, the sentinel `'********'`
rode inside that dict and the deep-merge propagated it to the leaf
— overwriting the on-disk credential. Same shape for AWS / Vault /
Infisical.

Fix: call `_strip_masked_values` BEFORE `atomic_update`, same as
the settings POST path.

### H4 (HIGH) — POST /api/notifications/config

`s['notifications'] = data` wholesale-replaced the subtree. SMTP
`smtp_password` and webhook URLs with embedded auth tokens returned
masked on GET were destroyed on the round-trip POST. Toggling a
non-secret notifications field (e.g. `enabled`) silently wiped the
SMTP password.

Fix: strip masked values and deep-merge against the existing
on-disk subtree inside the `settings_manager.update` mutator.

### M3 (MEDIUM) — `notifications` missing from `_DEEP_MERGE_SETTINGS_KEYS`

Even a future caller using `atomic_update({'notifications': ...})`
would shallow-merge and wipe siblings. Adding `notifications` to
the deep-merge registry closes the gap for any caller.

### Changes

- `modules/core/settings.py` — `_DEEP_MERGE_SETTINGS_KEYS` now
  includes `notifications`. Comment expanded to point at the audit
  reasoning.
- `modules/api/resources.py::StorageBackendConfig.post` — strip
  masked values via `_strip_masked_values` before `atomic_update`.
- `modules/web/misc_routes.py::api_notifications_config` (POST) —
  strip masked values, deep-merge against the existing on-disk
  notifications subtree inside the mutator.

### Tests

`tests/test_audit_settings_mutating_routes.py` (new, 9 cases):

- Registry: `notifications` is in `_DEEP_MERGE_SETTINGS_KEYS`;
  `certificate_storage` + `ca_providers` still present (defensive
  pin so a future refactor cannot silently drop them).
- StorageBackendConfig (HTTP integration test):
  - Round-trip with masked `client_secret` preserves on-disk value.
  - Genuine rotation (real new value) overwrites.
  - Sibling backend's secret survives a single-backend update.
- Notifications POST (handler-logic level — avoids the state
  pollution that registering the full misc_routes blueprint
  causes when paired with the CertificateManager lock tests):
  - Round-trip with masked `smtp_password` preserves on-disk.
  - Genuine rotation overwrites.
  - Empty-string secret preserves (same #215 semantic).
  - Webhook URL list-merge contract pinned (list overlay
    replaces wholesale — current behaviour; documented for any
    future list-merge improvement).

Full local unit suite: 901 passed, 2 skipped, 115 deselected.

Author: fabriziosalmi

modules/api/resources.py

@@ -1896,24 +1896,33 @@ def post(self):
                 if backend_type not in valid_backends:
                     return {'error': 'Invalid backend type'}, 400
 
-                # Build only the storage subtree and merge atomically so we
-                # don't race with concurrent writes or wipe other settings keys.
-                current = settings_manager.load_settings()
-                storage = dict(current.get('certificate_storage') or {})
-                storage['backend'] = backend_type
-
+                # Build only the storage subtree to merge atomically.
+                # Audit C2 (May 2026): the prior version wholesale-set
+                # the per-backend dict (e.g. `storage['azure_keyvault']
+                # = data.get('azure_keyvault', {})`). When the UI POSTs
+                # the round-trip from GET /api/web/settings, the masked
+                # sentinel `'********'` rides inside that dict and the
+                # deep-merge propagates it down to the leaf — overwriting
+                # the on-disk credential with the literal sentinel. The
+                # fix is the same shape PR #215 applied to the settings
+                # POST path: strip the sentinel BEFORE the merge so any
+                # masked / empty secret field falls back to its on-disk
+                # value, only genuinely-changed values overwrite.
+                from ..core.settings import _strip_masked_values
+                storage_update = {'backend': backend_type}
                 if backend_type == 'local_filesystem':
-                    storage['cert_dir'] = data.get('cert_dir', 'certificates')
+                    storage_update['cert_dir'] = data.get('cert_dir', 'certificates')
                 elif backend_type == 'azure_keyvault':
-                    storage['azure_keyvault'] = data.get('azure_keyvault', {})
+                    storage_update['azure_keyvault'] = data.get('azure_keyvault', {})
                 elif backend_type == 'aws_secrets_manager':
-                    storage['aws_secrets_manager'] = data.get('aws_secrets_manager', {})
+                    storage_update['aws_secrets_manager'] = data.get('aws_secrets_manager', {})
                 elif backend_type == 'hashicorp_vault':
-                    storage['hashicorp_vault'] = data.get('hashicorp_vault', {})
+                    storage_update['hashicorp_vault'] = data.get('hashicorp_vault', {})
                 elif backend_type == 'infisical':
-                    storage['infisical'] = data.get('infisical', {})
+                    storage_update['infisical'] = data.get('infisical', {})
 
-                success = settings_manager.atomic_update({'certificate_storage': storage})
+                clean_payload = _strip_masked_values({'certificate_storage': storage_update})
+                success = settings_manager.atomic_update(clean_payload)
 
                 if success:
                     return {

modules/core/settings.py

@@ -223,11 +223,22 @@ def _strip_masked_values(payload):
 # Top-level settings keys whose value is a nested dict that should be
 # deep-merged rather than wholesale-replaced on save. Each of these stores
 # multiple secret-bearing subtrees (per-backend storage credentials, per-CA
-# EAB credentials), so the user editing one field in the UI must not blow
-# away the others or the previously-saved secret for the same field.
+# EAB credentials, per-channel notification credentials), so the user
+# editing one field in the UI must not blow away the others or the
+# previously-saved secret for the same field.
+#
+# Audit finding M3 (May 2026): `notifications` was originally absent
+# from this list. The dedicated notifications POST route in
+# `modules/web/misc_routes.py` wholesale-replaced the subtree, so the
+# masked-sentinel + sibling-preservation logic that PR #215 added for
+# `certificate_storage` / `ca_providers` did not protect SMTP and
+# webhook credentials. Including `notifications` here AND routing the
+# misc_routes POST through `_strip_masked_values` + `_deep_merge_dict`
+# (the same shape as the settings POST path) closes the gap.
 _DEEP_MERGE_SETTINGS_KEYS = frozenset({
     'certificate_storage',
     'ca_providers',
+    'notifications',
 })
 
 

modules/web/misc_routes.py

@@ -184,8 +184,25 @@ def api_notifications_config():
             if not isinstance(data, dict):
                 return jsonify({'error': 'Body must be a JSON object'}), 400
 
+            # Audit H4 (May 2026): the prior `s['notifications'] = data`
+            # wholesale-replaced the subtree. The UI round-trips a GET
+            # response — where SMTP `smtp_password` and webhook URLs
+            # arrive masked as `'********'` — back into POST, and the
+            # plain assignment overwrote real on-disk secrets with the
+            # literal sentinel. Strip the sentinel BEFORE the write
+            # (same shape PR #215 + audit C2 fix applied elsewhere)
+            # and deep-merge against the existing notifications block
+            # so a partial UI submit (e.g. toggling `enabled` without
+            # re-typing the SMTP password) does not destroy siblings.
+            from modules.core.settings import _strip_masked_values, _deep_merge_dict
+            clean_data = _strip_masked_values(data)
+
             def _mutator(s):
-                s['notifications'] = data
+                existing = s.get('notifications')
+                if isinstance(existing, dict) and isinstance(clean_data, dict):
+                    s['notifications'] = _deep_merge_dict(existing, clean_data)
+                else:
+                    s['notifications'] = clean_data
                 return s
 
             settings_manager.update(_mutator)