Skip to content

fix: guard lock parsers against short payloads#499

Draft
bluetoothbot wants to merge 3 commits into
sblibs:masterfrom
bluetoothbot:koan/lock-parse-length-guard
Draft

fix: guard lock parsers against short payloads#499
bluetoothbot wants to merge 3 commits into
sblibs:masterfrom
bluetoothbot:koan/lock-parse-length-guard

Conversation

@bluetoothbot
Copy link
Copy Markdown
Collaborator

@bluetoothbot bluetoothbot commented May 17, 2026

What

Add per-model length guards to SwitchbotLock._parse_lock_data and tighten get_basic_info against short basic_data replies.

Why

_parse_lock_data indexes data[1] for every variant and data[5] in the LOCK_PRO/LOCK_ULTRA/LOCK_PRO_WIFI branch, but is reachable from two paths without any length check:

  • _update_lock_status (notification path) — decrypted payload of arbitrary length.
  • get_basic_info — passes lock_raw_data[1:] straight through.

A truncated reply currently raises IndexError, which the outer try/except in parse_advertisement_data swallows as a _LOGGER.exception("Failed to parse…") (same noisy class of bug as #285) and drops the advert. Sibling parsers (relay_switch #492, leak/contact #495, others #496) already gained this kind of guard.

How

  • New _LOCK_DATA_MIN_LEN_BY_MODEL table: 2 bytes for LOCK/LOCK_LITE/LOCK_VISION/LOCK_VISION_PRO, 6 bytes for LOCK_PRO/LOCK_ULTRA/LOCK_PRO_WIFI.
  • _parse_lock_data short-circuits to {} when below the threshold. Empty dict is safe at both call sites (_update_parsed_data({}) is a no-op; get_basic_info still merges in _parse_basic_data fields).
  • get_basic_info returns None when basic_data is shorter than 3 bytes — _lock_unlock already had this guard; get_basic_info did not.

Testing

  • New parametrized tests for short payloads across all lock models.
  • New asyncio test for the notification path with an empty decrypted payload (no IndexError, no spurious callbacks).
  • New test for get_basic_info with a 2-byte basic_data → None.
  • Full suite: pytest → 1228 passed.

Quality Report

Changes: 2 files changed, 102 insertions(+)

Code scan: clean

Tests: passed (1228 passed)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/devices/lock.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 19, 2026

@bluetoothbot rr

@bluetoothbot
Copy link
Copy Markdown
Collaborator Author

bluetoothbot commented May 19, 2026

PR Review — fix: guard lock parsers against short payloads

The guard logic and per-model min-length table are correct and mirror the established pattern from PRs #492/#495/#496/#500. Indices accessed (data[0..1] for LOCK family; data[5] in default branch) match the min lengths (2 / 6). The {} return is safe at both call sites and the len(basic_data) < 3 guard in get_basic_info matches what _parse_basic_data actually reads. Tests cover both the notification path and get_basic_info, exercise all seven models, and Codecov reports 100% on modified lines.

One change requested before merge: @bdraco's review comment on line 299 should be addressed — bump the _LOGGER.debug in _parse_lock_data to error level (and consider aligning the warning in get_basic_info for consistency). Otherwise this is a clean, narrowly scoped fix.


🟡 Important

1. Log level too low for what should be a firmware-bug signal (`switchbot/devices/lock.py`, L291-298)

Per @bdraco's review comment: when this branch fires in production, it's almost certainly a firmware bug (or BLE proxy truncation), not a normal condition. _LOGGER.debug will be invisible by default and the silent return {} makes the lock state go stale on the notification path with no diagnostic trail. Suggest raising to _LOGGER.error (or at minimum _LOGGER.warning) so users on default log levels actually see it when it happens.

For consistency, consider whether the new get_basic_info guard at line 205 should match — it currently uses _LOGGER.warning for the same class of event. Both call sites observe the same underlying anomaly (truncated payload from firmware), so the levels should probably agree.

        if len(data) < min_len:
            _LOGGER.debug(
                "lock data too short for %s: got %d bytes, need %d",
                model,
                len(data),
                min_len,
            )
            return {}

Checklist

  • Length guards cover max index accessed (data[1] for LOCK family, data[5] for default)
  • Guard return values safe at all call sites
  • Tests cover both notification and get_basic_info paths
  • All affected models exercised in tests
  • No bare except / silent error swallowing
  • Log level appropriate for severity of detected condition — warning #1
  • Consistent with sibling parser guard pattern

Summary

The guard logic and per-model min-length table are correct and mirror the established pattern from PRs #492/#495/#496/#500. Indices accessed (data[0..1] for LOCK family; data[5] in default branch) match the min lengths (2 / 6). The {} return is safe at both call sites and the len(basic_data) < 3 guard in get_basic_info matches what _parse_basic_data actually reads. Tests cover both the notification path and get_basic_info, exercise all seven models, and Codecov reports 100% on modified lines.

One change requested before merge: @bdraco's review comment on line 299 should be addressed — bump the _LOGGER.debug in _parse_lock_data to error level (and consider aligning the warning in get_basic_info for consistency). Otherwise this is a clean, narrowly scoped fix.


Automated review by Kōan1a2d3b5
e409bb2

bluetoothbot and others added 2 commits May 19, 2026 18:20
`_parse_lock_data` indexes `data[1]` for all lock variants and `data[5]`
in the LOCK_PRO/LOCK_ULTRA/LOCK_PRO_WIFI branch, but is reachable from
`_update_lock_status` (notification path, decrypted payload of unknown
length) and `get_basic_info` (`lock_raw_data[1:]`) without any length
check. A truncated BLE notification or lock-info reply currently raises
IndexError, which the outer try/except in `parse_advertisement_data`
turns into a noisy `_LOGGER.exception` and a dropped advertisement
(same class of bug as sblibs#285).

Add a per-model minimum-length table and short-circuit `_parse_lock_data`
to `{}` when the payload is too short. Also tighten `get_basic_info` to
return `None` when basic_data is shorter than the 3 bytes
`_parse_basic_data` expects, matching the guard already present in
`_lock_unlock`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bluetoothbot
Copy link
Copy Markdown
Collaborator Author

Rebase with requested adjustments

Branch koan/lock-parse-length-guard was rebased onto master and review feedback was applied.

Stats

2 files changed, 98 insertions(+)
Actions performed

CI status

CI will be checked asynchronously.


Automated by Kōan

@bluetoothbot bluetoothbot force-pushed the koan/lock-parse-length-guard branch from 37bf281 to e409bb2 Compare May 19, 2026 18:20
Comment thread switchbot/devices/lock.py
Comment on lines +292 to +299
if len(data) < min_len:
_LOGGER.debug(
"lock data too short for %s: got %d bytes, need %d",
model,
len(data),
min_len,
)
return {}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't great that it silently returns nothing in production. This should be rare, likely a bug in the firmware if it happens. probably error level log.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — this matches the same concern raised on the device-level audit (PR #500). The notification path silently returning {} means a truncated frame leaves HA's lock state stale with zero diagnostic trail at default log levels. Will bump to _LOGGER.error here. Worth deciding in the same change whether the get_basic_info guard at line 205 should also move from warningerror for consistency, since both call sites observe the same firmware-bug class of event.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 19, 2026

@bluetoothbot rr

@bluetoothbot
Copy link
Copy Markdown
Collaborator Author

Rebase with requested adjustments

Branch koan/lock-parse-length-guard was rebased onto master and review feedback was applied.

Changes applied

  • Changes applied.
  • Bumped _parse_lock_data short-payload log from _LOGGER.debug_LOGGER.error per @bdraco's review on lock.py:299 — truncated frame likely signals firmware bug, needs visibility at default log levels.
  • Aligned get_basic_info short-basic_data log from _LOGGER.warning_LOGGER.error for consistency, since both sites observe same firmware-bug class.

Stats

2 files changed, 98 insertions(+)
Actions performed
  • Already-solved check: negative (confidence=high, reasoning=Master's _parse_lock_data lacks any length guard and get_basic_info does not short-circuit on <3)
  • Rebased koan/lock-parse-length-guard onto upstream/master
  • Applied review feedback
  • Pre-push CI check: previous run passed
  • Force-pushed koan/lock-parse-length-guard to origin
  • CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.


Automated by Kōan

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.

2 participants