Skip to content

Select Web Push header parameters by name instead of by position - #43

Open
gentslava wants to merge 1 commit into
sdb9696:mainfrom
gentslava:fix/webpush-header-parameters
Open

gentslava wants to merge 1 commit into
sdb9696:mainfrom
gentslava:fix/webpush-header-parameters

Conversation

@gentslava

Copy link
Copy Markdown

Fixes #42, and supplies the field evidence #37 was waiting for.

Summary

Crypto-Key and Encryption are lists of ;-separated parameters (RFC 8188 §2.1, RFC 8291 §4) whose order is not fixed, but _handle_data_message slices the prefix off by position:

crypto_key = self._app_data_by_key(msg, "crypto-key")[3:]  # strip dh=
salt = self._app_data_by_key(msg, "encryption")[5:]  # strip salt=

A sender that signs with VAPID adds p256ecdsa next to dh, so the slice leaves <dh>; p256ecdsa=<…>. urlsafe_b64decode silently discards ;, the space and the label characters and decodes the whole thing, producing 137 bytes where a P-256 point needs 65 — ValueError: Invalid EC key. from from_encoded_point, or binascii.Error: Incorrect padding first, depending on the total length.

The failure lands before self.callback(...) and before the selective ACK at fcmpushclient.py:605-608, so the message is neither delivered nor acknowledged. The server redelivers it on every reconnect and the client dies again within a second of every login.

Field evidence

@MultivitaminJuice, this is the answer to the open question in #37:

An earlier local version also parsed ;-separated header parameters (dh=…;keyid=…) … but I never actually saw ;-parameters in my own captured headers, so I dropped both as speculative — happy to add back if you've seen such headers in the wild.

They are in the wild. A Russian intercom operator (Elektronny Gorod / «Мой Дом») started signing its pushes with VAPID. A diagnostic probe on a live Home Assistant install captured the shapes — lengths and parameter names only, no key material:

crypto-key: len=189 shape=dh=87, p256ecdsa=87
encryption: len=29  shape=salt=24

Two details worth keeping:

  • dh arrives unpadded (87 chars) while salt arrives padded (24 chars). So padding alone is neither necessary nor sufficient — with only Harden encrypted push payload decoding #37 applied, binascii.Error: Incorrect padding simply becomes ValueError: Invalid EC key. dh decodes only once it is separated from the VAPID parameter.
  • It is not a regression here: _decrypt_raw_data is byte-identical across 0.4.0–0.4.5, and the same code decrypted 16 of that operator's CALL_INCOMING pushes two months earlier, when crypto-key still carried only dh. The change is on the sender's side.

This also means #42's suggested removeprefix("dh=") does not fix the symptom #42 reports: on this header it removes the label but leaves the ; p256ecdsa=… tail attached.

Changes

  • _webpush_header_param(header, name) looks a parameter up by name, so its position no longer matters, and restores the base64url padding the wire format strips.
  • A header holding a bare value with no name= prefix is still accepted as that value.
  • A header that has parameters but not the requested one now raises ValueError with a clear message instead of corrupting the key silently. That composes with the per-message isolation in Harden encrypted push payload decoding #37 and the ValueError handling in Recover from transient read failures instead of aborting the client #38: the bad message gets skipped instead of taking the listener down.

Testing

  • uv run pytest tests/ — 24 passed, including a regression test that encrypts with real http_ece, hands _handle_data_message the exact production header shape and asserts the callback receives the payload. Reverting to the positional slice fails it with binascii.Error: Incorrect padding.
  • Parametrised coverage for parameter order, VAPID-style multi-parameter headers, missing padding, bare values and the missing-parameter error.
  • ruff check, ruff format --check and mypy clean.
  • Running in production on a Home Assistant custom integration since 2026-08-13; a real doorbell press now reaches Home Assistant, where every push previously killed the listener.

Relationship to the other open PRs

Independent of both, and they compose:

Happy to adjust naming, split the error-raising behaviour out, or add the parameter parsing to #37 instead if that is the shorter route to a release.

`Crypto-Key` and `Encryption` are lists of `;`-separated parameters (RFC 8188
2.1, RFC 8291 4) whose order is not fixed, but `_handle_data_message` sliced the
prefix off by position - `[3:]` for `dh=`, `[5:]` for `salt=`.

A sender that signs with VAPID adds `p256ecdsa` next to `dh`, so the slice
leaves `<dh>; p256ecdsa=<...>`. `urlsafe_b64decode` silently discards `;`, the
space and the label characters and decodes the whole thing, producing 137 bytes
where a P-256 point needs 65: `ValueError: Invalid EC key.` from
`from_encoded_point`, or `binascii.Error: Incorrect padding` first, depending on
the total length.

The failure lands before `self.callback(...)` and before the selective ACK, so
the message is neither delivered nor acknowledged and the server redelivers it
on every reconnect - the client dies again within a second of every login.

`_webpush_header_param` looks the parameter up by name, so its position no
longer matters, and restores the base64url padding that the wire format strips.
A header holding a bare value with no `name=` prefix is still accepted, and a
header that has parameters but not the requested one now raises `ValueError`
with a clear message instead of corrupting the key silently.

Observed against a Russian intercom operator that started signing its pushes
with VAPID: `crypto-key` arrives as `dh=<87 chars>; p256ecdsa=<87 chars>` and
`encryption` as `salt=<24 chars>` - so `dh` is unpadded while `salt` is padded,
which is why padding alone does not fix it. The same code decrypted that
operator's pushes fine two months earlier, when `crypto-key` still carried only
`dh`; `_decrypt_raw_data` is byte-identical across 0.4.0-0.4.5, so this is a
change on the sender's side rather than a regression here.

Tests: `uv run pytest tests/` green, including a regression test that encrypts
with real http_ece, hands `_handle_data_message` the exact production header
shape and asserts the callback receives the payload. Reverting to the positional
slice fails it. `ruff check`, `ruff format --check` and `mypy` clean.

Refs sdb9696#42.
@rk6246

rk6246 commented Aug 14, 2026

Copy link
Copy Markdown

Field evidence corroborating this PR's diagnosis, from a Home Assistant install running the core ring integration (ring-doorbell==0.9.14), Python 3.14.

The failure is fully deterministic across restarts, which I think supports the "neither delivered nor acknowledged, so the server redelivers on every reconnect" mechanism described here rather than a transient malformed payload. On 2026-08-13 Home Assistant Core was restarted five times, and the listener died on 5 of 5:

Core start (BST) Crash Delay
09:44:57 09:46:38 101s
10:08:20 10:09:34 74s
10:15:54 10:17:06 72s
17:07:15 17:07:42 27s
20:43:42 20:44:58 76s

Each crash is preceded 2-3 seconds earlier by Ring entity setup (event.front_door_ding). Once it fires, FcmPushClient stays down for the remainder of the session, so push is dead until the next restart, which then reproduces it again.

Traceback, identical every time:

ERROR (MainThread) [firebase_messaging.fcmpushclient] Unknown error: Incorrect padding, shutting down FcmPushClient.
Traceback (most recent call last):
  File "/usr/local/lib/python3.14/site-packages/firebase_messaging/fcmpushclient.py", line 688, in _listen
    await self._handle_message(msg)
  File "/usr/local/lib/python3.14/site-packages/firebase_messaging/fcmpushclient.py", line 605, in _handle_message
    self._handle_data_message(msg)
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/usr/local/lib/python3.14/site-packages/firebase_messaging/fcmpushclient.py", line 439, in _handle_data_message
    decrypted = self._decrypt_raw_data(
        self.credentials, crypto_key, salt, msg.raw_data
    )
  File "/usr/local/lib/python3.14/site-packages/firebase_messaging/fcmpushclient.py", line 378, in _decrypt_raw_data
    crypto_key = urlsafe_b64decode(crypto_key_str.encode("ascii"))
  File "/usr/local/lib/python3.14/base64.py", line 131, in urlsafe_b64decode
    return b64decode(s)
  File "/usr/local/lib/python3.14/base64.py", line 85, in b64decode
    return binascii.a2b_base64(s, strict_mode=validate)
binascii.Error: Incorrect padding

The 439 / 378 line numbers match the traceback quoted in #37 exactly, so this is the same build. I wasn't able to read the installed distribution version directly to confirm (the library lives inside the Home Assistant Core container and Docker access is blocked on this host), so I'm reporting the line numbers rather than guessing a version.

This lands as Incorrect padding rather than the Invalid EC key. of #42 -- consistent with the note here that which of the two surfaces depends on the total length after the positional slice.

Happy to capture the raw crypto-key / encryption header values if that would help confirm the p256ecdsa parameter is present, though that needs a patched build to log them before the decode, which I can't currently deploy on this host.

@slettmayer

Copy link
Copy Markdown

A data point in favour of this approach: I've been running the equivalent of this
patch against 0.4.5 in Home Assistant (Ring doorbell), applied at runtime rather
than as a fork, and a real VAPID-signed push now decrypts end to end where it
previously terminated the client permanently.

Selecting by name is the right call over widening the slice — the parameter order
in Crypto-Key is not fixed (RFC 8188 §2.1, RFC 8291 §4), so anything positional
stays fragile. One thing worth confirming is covered: the dh value arrives
unpadded in the VAPID form (87 chars) but padded in the single-parameter
form (88), so the base64url padding has to be restored after extraction or the
VAPID case still raises binascii.Error.

I wrote a standalone check that builds real ECE payloads with real ECDH keys and
asserts unpatched 0.4.5 crashes while the fixed version works, across both
parameter orders and both paddings:

https://github.com/slettmayer/ha-fcm-header-fix/blob/main/scripts/test_fcm_header_fix.py

Please take it for this PR's tests if it's useful. I've written up the full
failure chain — in particular why the crash landing before the selective ACK
makes this unrecoverable by any restart — in
#44.

Also worth pairing this with #37: this PR fixes the current crash, #37 stops the
next undecryptable payload from causing the same permanent death.

@zondajag zondajag left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks like a step in the right direction. Haven't been able to test this locally myself but this would explain the unrecoverable failures people see after a while of re-establishing a new authenticated ring API connection. @sdb9696

I actually started seeing things work again after applying this custom integration:
home-assistant/core#179149 (comment)

@hmchan

hmchan commented Sep 13, 2026

Copy link
Copy Markdown

Independent confirmation from production, plus one gap that I think is worth closing alongside this.

Confirmation

Hit this on a Home Assistant integration talking to a vendor push service that started signing with VAPID. The header:

crypto-key: dh=BFK6U_9LD_Lw...AFk; p256ecdsa=BDOU99-h67Hc...jfAnG4
encryption: salt=-iM1SbbqqvKkY_6aeWY6cw==

The positional slice leaves ; p256ecdsa=... attached to the dh value. urlsafe_b64decode silently discards the ; and the space and folds the second key into the output, yielding 137 bytes instead of a 65 byte P-256 point, which from_encoded_point then rejects. The stray = from p256ecdsa= landing mid-string also produces binascii.Error: Incorrect padding first, depending on the dh value's length — so the two symptoms people are reporting are the same root cause.

What makes it nasty is that it is unrecoverable rather than transient. The exception reaches _listen, which shuts the client down, and because the message is never acked the server redelivers it on the next connection. A supervisor that restarts the client just re-enters the same crash: mine did so every 60 seconds for 25 hours — 1392 cycles, zero messages delivered, and nothing above debug level to say why.

_webpush_header_param fixes it. I have been running the equivalent for 35 hours across 2700+ messages with no decrypt failures.

The gap

Both extraction calls sit outside the try/except that #37 adds around decryption, so two inputs still take the whole connection down:

Input Outcome
crypto-key present with parameters but no dh ValueError from _webpush_header_param escapes to _listen
crypto-key app_data absent entirely RuntimeError from _app_data_by_key escapes to _listen

Each then produces the same unacked-redelivery wedge as the original bug. With #37 also applied the log reads Unknown error: no 'dh' parameter in header 'p256ecdsa', shutting down FcmPushClient — different message, identical outcome.

Moving the two extraction calls inside the same try as the decryption, and catching RuntimeError alongside ValueError, closes both: a malformed message is then skipped and acked while the connection survives. Worth doing in the same change as this one, I think, since the whole point is that one bad message should not be fatal.

Happy to open a small PR stacked on this if that is useful — I have it implemented with tests, on top of this PR and #37.

@zondajag

Copy link
Copy Markdown

@sdb9696 (Steven B.) hasn't been active since June from what I can see. What usually happens when a project this widely used appears to get abandoned? Are there any other reviewers?

This branch has not been deployed

No deployments
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.

Invalid EC key

5 participants