fix(diagnostics): redact the OCF device name; log description on unknown type
Two things a review of this branch turned up. /oic/d's `n` is free text the owner sets from the SmartThings app, so it may carry a person's name. Nothing in the /device/0 dump has ever exposed it -- it only became reachable when diagnostics started reporting /oic/d earlier in this branch, which would have started carrying it into public issue reports. Redact it. `rt`, the device-type signal the block exists for, is untouched, and no /device/0 resource uses a bare 'n' key, so nothing else changes. The unknown-device-type warning logged only modelNum. That line is what a user pastes into an issue, and modelNum alone can't identify a washer from a dryer -- both report the shared DA_WM_ laundry board, and detection reads the consumer-model code out of `description` for exactly that reason. Log both fields.
This commit is contained in:
@@ -371,13 +371,20 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
warm.add(href)
|
||||
|
||||
model_num = info.get('x.com.samsung.da.modelNum', '')
|
||||
description = info.get('x.com.samsung.da.description', '')
|
||||
if reg is not None:
|
||||
self._log.debug("device type: %s (modelNum=%r)", reg.name, model_num)
|
||||
bound = discover(resources, reg.capabilities, reg.pattern_capabilities,
|
||||
log=unbound.append, tier_log=_tier_log)
|
||||
self.device_type_name = reg.name
|
||||
else:
|
||||
self._log.warning("unknown device type modelNum=%r; using common caps", model_num)
|
||||
# Both fields: detection reads each of them (board token, then
|
||||
# consumer-model code), and this line is what a user pastes into
|
||||
# an issue -- modelNum alone doesn't identify a washer or dryer.
|
||||
self._log.warning(
|
||||
"unknown device type modelNum=%r description=%r; using common caps",
|
||||
model_num, description,
|
||||
)
|
||||
bound = discover(resources, CAPABILITIES, log=unbound.append, tier_log=_tier_log)
|
||||
self.device_type_name = None
|
||||
self.bound = bound
|
||||
|
||||
@@ -20,11 +20,19 @@ _SENSITIVE_SUBSTRINGS = (
|
||||
)
|
||||
|
||||
# Matched whole, not as substrings. OCF's /oic/d and /oic/p identify the unit
|
||||
# with bare two-letter keys -- 'di' (device UUID) and 'pi' (platform UUID) --
|
||||
# which are as identifying as the serial number above but far too short to
|
||||
# match on: 'di' alone is a substring of 'condition', 'display', 'dispenser'
|
||||
# and plenty of other perfectly ordinary appliance fields.
|
||||
_SENSITIVE_EXACT = frozenset({'di', 'pi'})
|
||||
# with bare one- and two-letter keys that the rules above cannot see, being
|
||||
# far too short to match on -- 'di' alone is a substring of 'condition',
|
||||
# 'display', 'dispenser' and plenty of other ordinary appliance fields:
|
||||
#
|
||||
# 'di' -- device UUID, 'pi' -- platform UUID. As identifying as the serial
|
||||
# number above.
|
||||
# 'n' -- /oic/d's device name. Free text the owner can set from the
|
||||
# SmartThings app, so it may well carry a person's name. Nothing
|
||||
# in the /device/0 dump has ever exposed it; it only became
|
||||
# reachable when diagnostics started reporting /oic/d, and the
|
||||
# device-type signal we actually want from that resource is `rt`,
|
||||
# which is not redacted.
|
||||
_SENSITIVE_EXACT = frozenset({'di', 'pi', 'n'})
|
||||
|
||||
|
||||
def _is_sensitive_key(key: str) -> bool:
|
||||
|
||||
@@ -58,7 +58,8 @@ async def test_diagnostics_include_ocf_identity(
|
||||
device_types=('oic.wk.d', 'oic.d.refrigerator'),
|
||||
raw={
|
||||
'/oic/p': {'mnmn': 'Samsung Electronics', 'pi': '12-34-56'},
|
||||
'/oic/d': {'n': 'Family Hub', 'di': 'ab-cd-ef'},
|
||||
'/oic/d': {'n': 'Family Hub', 'di': 'ab-cd-ef',
|
||||
'rt': ['oic.wk.d', 'oic.d.refrigerator']},
|
||||
},
|
||||
)
|
||||
|
||||
@@ -67,11 +68,16 @@ async def test_diagnostics_include_ocf_identity(
|
||||
identity = diag['identity']
|
||||
assert identity['model'] == 'RF9000B'
|
||||
assert identity['device_types'] == ['oic.wk.d', 'oic.d.refrigerator']
|
||||
# The raw payloads ride along redacted -- we don't yet know which of
|
||||
# their fields identify a device type, so none are dropped up front.
|
||||
# The raw payloads ride along whole -- we don't yet know which of their
|
||||
# fields identify a device type, so nothing is dropped up front beyond
|
||||
# what redaction takes out.
|
||||
assert identity['resources']['/oic/p']['mnmn'] == 'Samsung Electronics'
|
||||
assert identity['resources']['/oic/d']['di'] == REDACTED
|
||||
assert identity['resources']['/oic/p']['pi'] == REDACTED
|
||||
# The owner-settable device name is redacted; `rt` -- the reason this
|
||||
# block exists -- is not.
|
||||
assert identity['resources']['/oic/d']['n'] == REDACTED
|
||||
assert identity['resources']['/oic/d']['rt'] == ['oic.wk.d', 'oic.d.refrigerator']
|
||||
|
||||
|
||||
async def test_diagnostics_identity_none_when_unavailable(
|
||||
|
||||
+11
-7
@@ -78,28 +78,32 @@ def test_redacts_bare_ocf_identity_keys():
|
||||
"""/oic/d and /oic/p identify the unit with two-letter keys ('di', 'pi')
|
||||
that the substring rules can't see."""
|
||||
redacted = redact_resources({
|
||||
'/oic/d': {'di': 'ab-cd-ef', 'n': 'Family Hub'},
|
||||
'/oic/d': {'di': 'ab-cd-ef', 'n': "Marc's Fridge",
|
||||
'rt': ['oic.wk.d', 'oic.d.refrigerator']},
|
||||
'/oic/p': {'pi': '12-34-56', 'mnmo': 'RF9000B'},
|
||||
})
|
||||
|
||||
assert redacted['/oic/d']['di'] == REDACTED
|
||||
assert redacted['/oic/p']['pi'] == REDACTED
|
||||
# Non-identifying neighbours in the same payloads survive.
|
||||
assert redacted['/oic/d']['n'] == 'Family Hub'
|
||||
# 'n' is free text the owner sets from the SmartThings app, so it can
|
||||
# carry a person's name -- redacted too. `rt`, the device-type signal
|
||||
# we actually want out of /oic/d, is not.
|
||||
assert redacted['/oic/d']['n'] == REDACTED
|
||||
assert redacted['/oic/d']['rt'] == ['oic.wk.d', 'oic.d.refrigerator']
|
||||
assert redacted['/oic/p']['mnmo'] == 'RF9000B'
|
||||
|
||||
|
||||
def test_bare_key_redaction_does_not_leak_into_substring_matching():
|
||||
"""'di'/'pi' are whole-key matches only -- plenty of ordinary appliance
|
||||
fields contain those two letters and must survive untouched."""
|
||||
"""The bare keys are whole-key matches only -- plenty of ordinary
|
||||
appliance fields contain those letters and must survive untouched."""
|
||||
redacted = redact_resources({
|
||||
'/x': {
|
||||
'condition': 'Normal', 'display': 'On', 'dispenser': 'Cubed',
|
||||
'humidity': '45', 'spinSpeed': '1200',
|
||||
'humidity': '45', 'spinSpeed': '1200', 'name': 'FilterProgress',
|
||||
},
|
||||
})
|
||||
|
||||
assert redacted['/x'] == {
|
||||
'condition': 'Normal', 'display': 'On', 'dispenser': 'Cubed',
|
||||
'humidity': '45', 'spinSpeed': '1200',
|
||||
'humidity': '45', 'spinSpeed': '1200', 'name': 'FilterProgress',
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user