Repository navigation
fix(coordinator): require an authenticated location before clearing decrypt budget - #1117
Conversation
…ecrypt budget The shared consecutive-decrypt-failure budget (which gates re-authentication) was cleared whenever a decode produced a non-empty result. The decode layer, however, returns a single metadata_only sentinel row when no encrypted report decrypts but the secrets bundle still yields key material (e.g. a phone without a fresh fix). That row is non-empty but carries no authenticated decrypt, so: - background push (fcm_receiver_ha): _note_decrypt_success_for_entry fired on it - poll cycle (polling): cycle_had_successful_decrypt was set on it Report-less metadata_only pushes/polls interleaved with real failures could keep a stale E2EE key below the reauth threshold, leaving the account stuck with no location updates and no reauth prompt. Introduce a shared predicate is_real_location_record() in decrypt_locations.py (the sentinel's producer, the single source of truth for the metadata_only contract) and gate both success signals on it. A genuine decrypted record still clears the budget; the metadata still flows downstream. Manual locate stays upward-only (unchanged). Addresses the Codex review on PR #182. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72335b8ef2
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not record: | ||
| return False | ||
| return record.get("metadata_only") is not True |
There was a problem hiding this comment.
When the only decoded rows are SEMANTIC, no encrypted location was authenticated: the decryptor appends those rows with decrypted_location=b"" and continues before the crypto path, and the payload has metadata_only absent. This predicate therefore returns true for semantic-only pushes/polls, so those report-less updates can still clear the shared decrypt-failure reauth budget between real DecryptionError cycles and leave a stale E2EE key below the threshold; require evidence of an encrypted/decrypted coordinate report or explicitly exclude semantic-only rows before returning true.
Useful? React with 👍 / 👎.
…budget SEMANTIC rows skip the crypto path (decrypted_location=b"", continue before authentication) and carry no metadata_only flag, so the previous `metadata_only is not True` denylist wrongly treated them as proof that the account-wide shared key still decrypts. A semantic-only push/poll interleaved with real DecryptionError cycles could clear the reauth budget and strand a stale E2EE key below the threshold. Replace the denylist with an allowlist on positive proof: a record clears the budget only if it carries authenticated coordinates (latitude and longitude not None), the sole output of the crypto path. This rejects metadata_only sentinels, SEMANTIC rows, and any future report-less shape across both consumer paths (poll cycle + background push) without changing own/foreign coordinate-report handling. manual locate stays upward-only. Tests: predicate unit matrix extended (SEMANTIC, partial-coordinate, 0.0), plus poll-cycle and background-push regression tests proving a SEMANTIC-only result does not reset the counter. Mutation-checked (denylist revert and single-axis predicate both turn the new tests red). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 742824aa7d
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # SEMANTIC row (no decrypted coordinates) -- prove nothing, so | ||
| # one report-less device must not mask another device's stale | ||
| # key for the whole cycle. | ||
| if is_real_location_record(location): |
There was a problem hiding this comment.
When a locate response contains both a decrypted coordinate report and a newer SEMANTIC/metadata row, api.async_get_device_location() has already selected just one record before this polling code sees it, and the selector prioritizes the newest last_seen. Checking only that selected location here can leave cycle_had_successful_decrypt false even though the same response did decrypt coordinates successfully, so the shared decrypt-failure counter is not reset and a later intermittent failure can still trigger reauth. Please carry the “any real coordinate record” proof from the full record list (or an explicit flag) instead of testing only the selected row.
Useful? React with 👍 / 👎.
…ted record The poll path cleared the shared decrypt-failure (reauth) budget only when the single display-selected location record was an authenticated coordinate report. api.async_get_device_location collapses a device's response to one record via _select_best_location, which ranks by newest last_seen. A newer report-less SEMANTIC/metadata row can therefore outrank and hide a sibling coordinate report that decrypted successfully in the same response, so the poll loop missed the proof, left the counter intact, and a later transient failure could trip a spurious reauth. The decrypt proof is a property of the whole response, not of one record. Add a shared any_real_location_record(records) predicate in decrypt_locations.py and evaluate it over the full candidate list. async_get_device_location carries the full-list verdict to the poll loop via an internal _decrypt_proven hint (popped before caching, like _report_hint); the gate prefers the hint and falls back to the single record only when it is absent (legacy/mocks). The background push path now uses the same shared helper instead of an inline any(...), keeping both automatic paths on one full-list contract. Manual locate stays upward-only and strips the hint at the source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74dd6640aa
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| api = GoogleFindMyAPI(cache=_StubCache()) | ||
| return await api.async_get_device_location("dev-1", "Tracker") | ||
|
|
||
| best = asyncio.run(_run()) |
There was a problem hiding this comment.
tests/AGENTS.md says tests under tests/ must never call asyncio.run() because Home Assistant's pytest-asyncio harness already manages the event loop; this newly added test starts its own loop (and the next added test repeats the pattern), which can clash with async fixtures or session cleanup when the module runs under the HA harness. Please mark the test async and await the helper directly instead.
Useful? React with 👍 / 👎.
… allowlist tests/AGENTS.md forbids asyncio.run() inside tests because Home Assistant's pytest-asyncio harness already manages the event loop (asyncio_mode = auto); a competing loop breaks fixtures. Codex flagged the two location-selection tests added for the decrypt-proof fix. Convert all four asyncio.run() call sites in tests/test_api_location_selection.py to async def + direct await (the two newly added decrypt-proof tests plus two pre-existing same-file tests). test_sync_wrappers_guard_when_loop_running now runs in the managed loop, which is exactly its guarded condition, so its assertions are unchanged. The file is now free of asyncio.run(), so remove it from LEGACY_ALLOWLIST in tests/test_guard_asyncio_run_antipattern.py; the guard now enforces the no-asyncio.run rule on this file going forward. The remaining ad-hoc loop in test_sync_wrappers_use_provided_session_loop uses asyncio.new_event_loop()/run_until_complete to capture the session-bound loop and is intentionally left as-is (the AST guard only matches asyncio.run). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Addresses the Codex review on PR #182 (commit
5f83158b): the sharedconsecutive-decrypt-failure budget that gates re-authentication was cleared on
any non-empty decode result, including a
metadata_onlysentinel row thatproves no successful decrypt.
The bug
async_decrypt_location_response_locationsreturns a singlemetadata_onlysentinel row when no encrypted report decrypts but the secrets bundle still
yields key material (e.g. a phone without a fresh fix). That row is non-empty
but carries only key material, not the result of an authenticated E2EE
decrypt. Two automatic paths treated its non-emptiness as positive proof of the
account-wide key:
Auth/fcm_receiver_ha.py):_note_decrypt_success_for_entryfired after the
if not locationsguard.coordinator/polling.py):cycle_had_successful_decryptwasset for any truthy
location(a metadata_only row reaches it via the_select_best_locationfallback).Report-less
metadata_onlypushes/polls interleaved with real decrypt failurescould keep a stale E2EE key below the reauth threshold, leaving the account
stuck with no location updates and no reauth prompt. Codex flagged the
background path; the poll path had the same defect (breadth sweep).
The fix
A shared predicate
is_real_location_record()indecrypt_locations.py(the sentinel's producer, the single source of truth for the
metadata_onlycontract;
metadata_only=Trueis set in exactly one place). Both successsignals are now gated on it:
if any(is_real_location_record(r) for r in locations): _note_decrypt_success_for_entry(...)if is_real_location_record(location): cycle_had_successful_decrypt = TrueA genuine decrypted record still clears the budget; the metadata still flows
downstream unchanged. Manual locate stays upward-only (only ever increments,
never resets — unchanged).
Tests
path (and the metadata still passes through).
(modelled on the existing idle-cycle test).
is_real_location_record(real /False/Noneflag / sentinel / empty /None).test red.
ruff check+mypy --strict(sources + tests) green; broader regressiongreen; changed lines covered.
Why a separate PR
Codex reviewed PR #182 (the
1.7.2→ upstream PR). This fix branches fromjleinenbach:1.7.2and targets1.7.2, so after merge it flows into #182automatically.
Review note (Codex round 7)
Codex flagged that the round‑6 predicate
is_real_location_record()used adenylist on a single sentinel (
metadata_only is not True).SEMANTICrowsskip the crypto path (
decrypted_location=b"",continuebeforeauthentication) and carry no
metadata_onlyflag, so they passed the predicatewithout proving a decrypt. The predicate is now an allowlist on positive
proof: a record clears the reauth budget only if it carries authenticated
coordinates (
latitudeandlongitudeboth notNone) — the sole output ofthe crypto path. This rejects
metadata_onlysentinels,SEMANTICrows, andany future report‑less shape across both consumer paths (poll cycle and
background push), without changing own/foreign coordinate‑report handling, and
keeps
manual locateupward‑only. Added a predicate unit matrix (SEMANTIC,partial‑coordinate,
0.0) and poll/background regression tests; mutation‑checked(reverting to the denylist and dropping one coordinate axis both fail the new
tests).
Review note (Codex round 8)
Codex flagged that the poll path derived the decrypt proof from the single
display-selected record returned by
async_get_device_location. That selectorranks by newest
last_seen, so a newer report-less SEMANTIC/metadata row canhide a sibling coordinate report that decrypted in the same response, leaving
cycle_had_successful_decryptfalse and risking a spurious reauth.Fix: the decrypt proof is now computed over the full response via a shared
any_real_location_record(records)predicate (single source of truth indecrypt_locations.py).async_get_device_locationcarries the full-listverdict to the poll loop through an internal
_decrypt_provenhint (poppedbefore caching, like
_report_hint); the gate prefers the hint and falls backto the single record only when it is absent. The background push path now uses
the same shared helper instead of an inline
any(...), so both automatic pathsshare one full-list contract. Manual locate stays upward-only and strips the
hint at the source. New mutation-sharp tests cover the helper, the API hint, and
the poll reset; full suite green (4010 passed).
Review note round 9 (test hygiene, Codex commit
74dd6640): Codex flaggedasyncio.run()in the newly addedtest_api_location_selection.pytests, whichtests/AGENTS.mdforbids (HA manages the event loop;asyncio_mode = auto). Converted all fourasyncio.run()call sites in that file toasync def+await(the two new decrypt-proof tests plus two pre-existing same-file tests) and removed the file fromLEGACY_ALLOWLISTintest_guard_asyncio_run_antipattern.py, so the guard now enforces the rule there.test_sync_wrappers_guard_when_loop_runningkeeps its semantics (it now runs inside the managed loop, exactly its guarded condition). The deliberateasyncio.new_event_loop()loop-capture test is intentionally left as-is. No production code changed; guard + affected suites green, ruff + mypy --strict clean.