feat: add options to redact slot names and PINs from debug logs#640
feat: add options to redact slot names and PINs from debug logs#640firstof9 wants to merge 8 commits into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
+ Coverage 84.14% 91.32% +7.17%
==========================================
Files 10 40 +30
Lines 801 4378 +3577
Branches 0 30 +30
==========================================
+ Hits 674 3998 +3324
- Misses 127 380 +253
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
# Conflicts: # tests/test_coordinator_lifecycle.py
tykeal
left a comment
There was a problem hiding this comment.
Thanks for the third pass at this @firstof9 — the redaction infrastructure (the __repr__ override, the propagation in __post_init__, the options-flow plumbing, and the Schlage provider hookup) is solid and it is clear you have iterated well on feedback from #631 and #638.
The headline concern, though, is that the redaction coverage is incomplete, and partial redaction is a worse user outcome than no redaction at all — it gives users false confidence that enabling redact_slot_names=True actually keeps slot names out of their logs, when in reality several callsites still emit the raw value. A user grabbing home-assistant.log for a bug report would see [REDACTED] in some places and their actual slot names in others, and reasonably conclude the log is safe to share.
Requesting changes to close the two leak paths below; the rest is should-fix / nit and can be folded in alongside.
🔴 Blockers
1. Akuvox provider leaks slot names in three log statements
custom_components/keymaster/providers/akuvox.py is not touched by this PR, but its slot-tagging path logs user-visible slot names directly. On upstream/main:
- L465–471 —
_LOGGER.debug("[AkuvoxProvider] No managed slot available for untagged user '%s'; leaving untouched", original_name, ...) - L478–484 —
_LOGGER.debug("[AkuvoxProvider] Tagged user '%s' (id=%s) as slot %d: '%s'", original_name, device_id, slot_num, tagged_name, ...) - L486–492 —
_LOGGER.error("[AkuvoxProvider] Failed to tag user '%s' for slot %d: %s: %s", original_name, slot_num, ...)
original_name and tagged_name are exactly the values this feature claims to mask. The same fix you applied in providers/schlage.py should land here. Even better, factor the masking into a small helper on providers/_base.py (or helpers.py) — something like _redact_name(value, lock) — so the next provider author cannot forget. Right now the contract "providers must consult lock.redact_slot_names before logging" lives only as tribal knowledge in the Schlage diff.
2. send_manual_notification and call_hass_service debug logs leak slot names through notification payloads
custom_components/keymaster/helpers.py is not touched by this PR either. On upstream/main:
- L169–175 —
_LOGGER.debug("[call_hass_service] service: %s.%s, target: %s, service_data: %s", domain, service, target, service_data,) - L192–198 —
_LOGGER.debug("[send_manual_notification] script: %s.%s, title: %s, message: %s", SCRIPT_DOMAIN, script_name, title, message,)
The notification text is constructed in coordinator.py ~L841–854:
if kmlock.lock_notifications:
message = event_label
if code_slot_num > 0:
if (kmlock.code_slots and kmlock.code_slots.get(code_slot_num)
and kmlock.code_slots[code_slot_num].name):
message = (
f"{message} by {kmlock.code_slots[code_slot_num].name} [{code_slot_num}]"
)…and that string flows into send_manual_notification (logged verbatim) and then into call_hass_service as part of service_data (also logged verbatim). The slot name appears in both debug lines.
Important nuance: the notification itself legitimately contains the slot name — it is user-facing — so do not redact what is sent. Only redact what is logged. Two reasonable shapes:
- Construct a separate redacted view of
message/service_datafor the log line (consult the owningKeymasterLock.redact_slot_namesflag). - Drop sensitive fields from the debug log entirely — log script name, target, and
messagelength /titlepresence rather than full payloads.
Option 2 is cheaper and arguably more appropriate for what these debug lines are actually used to diagnose.
🟡 Should-fix
3. __post_init__ propagation only runs at construction
KeymasterLock.__post_init__ propagates the redact flags onto existing entries in code_slots, but slots added or replaced after construction (subsequent updates, restore-from-disk paths, any provider that builds new CodeSlot objects post-init) will not inherit the flags unless every callsite remembers to set them by hand. That is a fragile invariant.
Two options:
- Centralize all slot creation through a setter /
__setattr__onKeymasterLockthat re-applies the flags to incoming slots. - Make
KeymasterCodeSlot.__repr__look up the parent lock by reference (weakref or back-pointer) and read the flags live, instead of carrying its own copy of the flag.
The second is more invasive but eliminates the synchronization problem entirely.
4. Test coverage gaps in the options flow
The new tests cover form rendering and a basic submit, but I do not see tests for:
- Both checkboxes toggled in both directions (true→false, false→true) end-to-end through a reload.
- That the registered
update_listeneractually triggersasync_reload_entrywhen options change. - Defaults precedence:
entry.options→entry.data→ constant default. - The migration path: a config entry that pre-dates this PR upgrades cleanly and picks up
Truedefaults viaentry.options.get(..., DEFAULT_REDACT_*).
At minimum, one test that toggles a value, asserts the listener fires, and verifies the new value is observable on the reloaded coordinator would close the most important gap.
5. Defaulting to True is a behavior change for existing users
Defaulting both options to True is the right privacy-first call, but existing users who currently triage problems via debug logs will see slots flip to [REDACTED] after upgrade with no warning. Worth deciding deliberately between:
- A release-notes callout describing the new default and where to toggle it.
- A one-time
persistent_notificationon first upgrade pointing users at the option. - Default to
Falseand let users opt in (less safe by default, less surprising).
Not a blocker — just flag it so it is a deliberate decision rather than a side effect of the migration.
🔵 Nitpicks
6. Naming consistency — CONF_REDACT_PINS
See inline. The rest of the codebase uses singular pin per slot (granted, CONF_HIDE_PINS is the existing precedent, but CONF_REDACT_PIN_CODES reads more symmetrically with CONF_REDACT_SLOT_NAMES). Pure cosmetic, take it or leave it.
7. KeymasterCodeSlot.__repr__ approach is fine
For the record: the explicit __repr__ override on a @dataclass (without @dataclass(repr=False)) is correct — explicit method definitions shadow the dataclass-generated __repr__. Verified, no change needed. Calling it out so you know we checked.
Thanks again for the iteration — once Blockers 1 and 2 are closed (and ideally factored through a shared helper so future providers cannot regress), this is in good shape.
| CONF_CHILD_LOCKS_FILE = "child_locks_file" | ||
| CONF_ENTITY_ID = "entity_id" | ||
| CONF_HIDE_PINS = "hide_pins" | ||
| CONF_REDACT_PINS = "redact_pins" |
There was a problem hiding this comment.
[NITPICK] Naming: CONF_REDACT_PINS reads asymmetrically with CONF_REDACT_SLOT_NAMES right below it. Consider CONF_REDACT_PIN_CODES for parity.
| CONF_REDACT_PINS = "redact_pins" | |
| CONF_REDACT_PIN_CODES = "redact_pin_codes" |
(Pure cosmetic — feel free to ignore. If you do rename, the matching DEFAULT_REDACT_PINS/DEFAULT_REDACT_PIN_CODES and all references in config_flow.py, __init__.py, lock.py, migrate.py, text.py, and strings.json would need updating too.)
Code Review Summary🔴 Critical
|
Summary
This PR adds configuration options to allow users to redact slot names and PIN codes from the integration's debug, warning, and error logs. It implements a Home Assistant Options Flow exposing two settings:
redact_slot_namesandredact_pins. When enabled, these settings mask slot names and PIN values in logs (including diagnostic/dataclass representations) with[REDACTED].Both options default to
Trueto prioritize user privacy out of the box, and can be customized/toggled off at any time by selecting "Configure" on the Keymaster integration card in the Home Assistant UI.Proposed change
CONF_REDACT_SLOT_NAMESandCONF_REDACT_PINSalong with their default values toconst.py.KeymasterOptionsFlowHandlerto present checkboxes for the settings, and registered it onKeymasterConfigFlowviaasync_get_options_flow.update_listenerto__init__.pythat automatically reloads the integration when configuration options are updated.__repr__inKeymasterCodeSlotto dynamically masknameandpinvalues if enabled.__post_init__toKeymasterLockto propagate the parent lock's redaction options down to all child slots.coordinator.py(during PIN settings),text.py(when updating or setting entity values), and the Schlage provider inschlage.py(during duplicate checks, tagging actions, and rollbacks).tests/test_config_flow.pyand redaction propagation/representation logic intests/test_coordinator_lifecycle.py.Type of change
Additional information