fix(fcm): survive an undecodable push frame instead of dying on it (#373) - #374
Merged
Merged
Conversation
) One push message the library cannot decrypt was enough to end real-time push permanently, and silently. `_decrypt_raw_data` decodes the `crypto-key` and `encryption` header values without padding, while padding the two stored key values in the same function. Those headers are URL-safe base64 that may legitimately arrive without trailing `=`, so an unpadded one raises `binascii.Error`. That is a `ValueError`, so the listen loop's `except (OSError, EOFError)` misses it, it reaches the outer `except Exception`, and the client shuts down. The severe part is where it raises: before the library appends the persistent id and sends the selective ack. The message is therefore never acknowledged, so it is redelivered on the next connection — and the supervision from #285 faithfully restarts straight back into it. The reporter measured the same message killing the client 16 times over 3.5 hours, each death 3-9 ms after receiving it, surviving a host reboot because the queue is server-side. Three changes: - `install_fcm_decrypt_guard` pads both header values before delegating, and contains any remaining decode failure by returning empty bytes. The handler then stays on its normal path and reaches the acknowledgement, so one event is lost instead of every future one. - Repeated deaths on the same persistent_id raise a Repair. The failure is otherwise invisible: alarm state comes from polling and HTS, never from push, so nothing looks wrong from the outside. - At that same threshold the stored FCM registration is discarded and renewed once, which is what the reporter did by hand. Without it the only recovery available to a user is editing `.storage`. Upstream fixes the root cause in sdb9696/firebase-messaging#37, open and mergeable since June with no release carrying it; the guard goes away when a release ships it. Also fixes two things found on the way: `test_start_without_firebase_messaging` simulated an absent package by nulling only the parent module, which stops working once anything imports a submodule, and `strings.json` was missing the `fcm_not_configured` entry that `translations/en.json` already had. Refs #373, #359
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the failure @wip3out3r documented in #359 and that #373 tracks: a single push message the FCM library cannot decrypt ends real-time push permanently, and silently.
The mechanism, verified against the library source
_decrypt_raw_datadecodes thecrypto-keyandencryptionheader values without padding, while padding the two stored key values it decodes in the same function. Those headers are URL-safe base64 that may legitimately arrive with the trailing=stripped, so an unpadded one raisesbinascii.Error.That is a
ValueError, so the listen loop's innerexcept (OSError, EOFError)does not catch it. It reaches the outerexcept Exception, which logsUnknown error: Incorrect padding, shutting down FcmPushClientand terminates the client.The severe part is where it raises. In
_handle_messagethe decrypt runs before the two statements that acknowledge the message, so neither the persistent-id bookkeeping nor the selective ack ever executes. The message is never acknowledged, so it is redelivered on the next connection, and the supervision added in #285 faithfully restarts straight back into it. Measured by the reporter: the same message killed the client 16 times over 3.5 hours, each death 3–9 ms after receiving it, and a host reboot did not clear it because the queue lives server-side.What this changes
1. Pad both header values, and contain any remaining failure.
install_fcm_decrypt_guardpatches_decrypt_raw_datato padcrypto-key/encryptionbefore delegating — the same+ "========"treatment the library already applies to the private key and secret — and returnsb""if the decrypt still fails. The handler then continues on its normal path, logs its own "Failed to decrypt data" line, hands the callback an empty payload (noENCODED_DATA, so our notification handler ignores it), and reaches the acknowledgement. One event is lost instead of every future one.2. Surface repeated deaths as a Repair. Three consecutive terminations sharing the same last-received
persistent_idraisefcm_push_stuck. Deaths with no push received at all carry no id and deliberately do not accumulate, so an ordinary network outage cannot be misreported as a poisoned message. This exists because the failure is otherwise invisible: alarm state comes from polling and the HTS path and never from push, so nothing looks broken while real-time events are gone — the reporter only found it while investigating something unrelated.3. Renew the registration once at that threshold. The replayed frame is queued server-side against a specific registration, so a new identity is the only exit. That is what the reporter did by hand; without this the only recovery available to a user is editing
.storage. It runs at most once per streak — an unbounded series of registrations against the Firebase project is exactly what #227 exists to prevent — and re-arms after a healthy run, which also clears the Repair.Tests
_pad_urlsafe_b64across all four length remainders, including a test that pins the bug itself (unpadded input raises without the fix) so the premise is checked rather than assumed.FcmPushClient, not only a double: the unpatched class genuinely raisesbinascii.Erroron unpadded input, and the guard contains it. A fixture snapshots and restores the class so this holds regardless of suite order.fcm_push_stuckis translated in all 14 shipped locales.Two incidental fixes found on the way
test_start_without_firebase_messagingsimulated an absent package by nulling only the parentfirebase_messagingkey, but the code imports the submodulefirebase_messaging.fcmregister, and a cached submodule stays importable when the parent isNone. The test silently stopped simulating anything as soon as another test imported the library, and fell through into the realStore. It now nulls both.strings.jsonwas missing thefcm_not_configuredentry thattranslations/en.jsonalready had.Upstream
The root cause is fixed properly in sdb9696/firebase-messaging#37, reported as sdb9696/firebase-messaging#40, and independently hit by the Home Assistant Ring integration and Fermax Blue. That PR has been open and mergeable since June with no maintainer movement, and the newest release predates it. The guard is removable once a release ships the fix — same standing upstream wait as #297.
Verified:
make checkgreen on 3.12 (2100 passed, 90% coverage) and the suite green on 3.13, plus three randomised orderings to confirm no cross-test pollution remains.Refs #373, #359