fix(push): VAPID key format silently broke every web-push send - #2166
Merged
Conversation
Web push has been 100% broken for all users. load_or_create_vapid_keypair returns the private key as a PEM string, but pywebpush feeds it to py_vapid's Vapid.from_string, which base64-decodes and DER-parses its input. A raw PEM fails that as 'Could not deserialize key data' on EVERY send, and the per-subscription handler caught it as a generic warning and returned 'failed', so nothing ever pushed and there was no obvious signal. Convert the PEM to base64url-DER once at fan-out (_vapid_signing_key), verified live against Apple's push service (web.push.apple.com returns 201 and the notification arrives on the device). Also: - move the VAPID sub off the reserved .local domain to mailto:info@taos.my; Apple is stricter than other services about the sub claim. - log an ERROR (not the swallowed per-send warning) when the key cannot be converted, so a global misconfig is loud instead of disabling all push invisibly. A 100% failure that logs nothing is how this hid. - regression test: the converted key must be accepted by Vapid.from_string.
📝 WalkthroughWalkthroughWeb-push notifications now convert stored VAPID PEM keys to pywebpush-compatible signing keys before fan-out, use a routable VAPID subject address, and return zeroed results when conversion fails. A regression test verifies that the converted key is accepted by pywebpush. ChangesVAPID web-push updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix web-push by converting VAPID PEM to pywebpush key format
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| data_str = json.dumps(_build_payload(row)) | ||
| results = await asyncio.gather( | ||
| *[_send_one(sub, data_str, private_pem, store) for sub in subs], | ||
| *[_send_one(sub, data_str, signing_key, store) for sub in subs], |
There was a problem hiding this comment.
WARNING: _send_one's third parameter is still named private_pem but now receives the converted signing_key. This mismatch is misleading and error-prone — a future maintainer could pass the raw PEM here, reintroducing the same 100%-failure bug.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Warnings Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 changed + 1 related)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 70.7K · Output: 8.1K · Cached: 470.9K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_notifications_push.py (1)
526-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the send-path wiring as well.
This test proves the converter output is parseable, but it would still pass if
send_web_pushlater passed the raw PEM to_send_one. Add a focused test that stubs_send_oneand asserts the converted key is passed once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_notifications_push.py` around lines 526 - 543, Add a focused test for send_web_push that stubs _send_one, invokes the send path with a generated VAPID key, and asserts _send_one is called once with the converted signing key from _vapid_signing_key rather than the raw PEM. Keep the existing parseability regression test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_notifications_push.py`:
- Around line 526-543: Add a focused test for send_web_push that stubs
_send_one, invokes the send path with a generated VAPID key, and asserts
_send_one is called once with the converted signing key from _vapid_signing_key
rather than the raw PEM. Keep the existing parseability regression test
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53f10033-0dbb-43bf-b9ae-90205f9138cf
📒 Files selected for processing (2)
tests/test_notifications_push.pytinyagentos/notifications_push.py
Code Review by Qodo
1. Desktop push still uses PEM
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+208
to
+227
| def _vapid_signing_key(private_pem: str) -> str: | ||
| """Convert the stored VAPID PEM into the base64url-DER form pywebpush wants. | ||
|
|
||
| ``load_or_create_vapid_keypair`` returns the private key as a PEM string, but | ||
| ``pywebpush.webpush(vapid_private_key=...)`` feeds it to ``py_vapid`` via | ||
| ``Vapid.from_string``, which base64-decodes and DER-parses its input. A raw | ||
| PEM fails that as "Could not deserialize key data" on EVERY send, and the | ||
| per-subscription handler swallowed it as a generic warning -- so web push was | ||
| 100% broken for every user with no obvious signal. Convert once at fan-out. | ||
| """ | ||
| from cryptography.hazmat.primitives import serialization | ||
|
|
||
| key = serialization.load_pem_private_key(private_pem.encode(), password=None) | ||
| der = key.private_bytes( | ||
| serialization.Encoding.DER, | ||
| serialization.PrivateFormat.PKCS8, | ||
| serialization.NoEncryption(), | ||
| ) | ||
| return base64.urlsafe_b64encode(der).decode().rstrip("=") | ||
|
|
There was a problem hiding this comment.
1. Desktop push still uses pem 🐞 Bug ≡ Correctness
This PR converts the stored VAPID private key from PEM to the base64url-DER string that pywebpush/py_vapid expects, but only in tinyagentos.notifications_push.send_web_push(). The parallel BrowserApp web-push sender (tinyagentos.routes.desktop_browser.push.send) still forwards the raw PEM string as vapid_private_key, which this PR documents as failing py_vapid parsing before any network call.
Agent Prompt
### Issue description
`tinyagentos/routes/desktop_browser/push.py` still passes the VAPID **PEM** string (from `load_or_create_vapid_keypair`) directly as `vapid_private_key` into `pywebpush.webpush()`. This PR shows that `py_vapid.Vapid.from_string` (used under the hood by `pywebpush`) expects a **base64url-encoded DER** string and that raw PEM breaks parsing.
### Issue Context
- OS notification push (`tinyagentos/notifications_push.py`) now converts PEM → base64url(DER) via `_vapid_signing_key()` and uses the converted string for fan-out.
- BrowserApp push (`tinyagentos/routes/desktop_browser/push.py`) uses the same VAPID keypair loader (`tinyagentos/routes/desktop_browser/vapid.py`) and still passes the PEM string through unchanged.
### Fix Focus Areas
- tinyagentos/routes/desktop_browser/push.py[80-123]
- tinyagentos/routes/desktop_browser/push.py[206-219]
- tinyagentos/notifications_push.py[208-227]
### Implementation notes
- Reuse the existing conversion logic (either import `_vapid_signing_key` or move it to a small shared utility module so both push senders use the same conversion).
- Convert once per `send()` fan-out (similar to `send_web_push`) and pass the converted value as `vapid_private_key`.
- Consider adding a regression test in the desktop_browser push test suite similar to `test_vapid_signing_key_is_accepted_by_pywebpush_vapid` to ensure the desktop path also provides a `Vapid01.from_string`-accepted key.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
hognek
pushed a commit
to hognek/tinyagentos
that referenced
this pull request
Jul 29, 2026
…fc#2166) Web push has been 100% broken for all users. load_or_create_vapid_keypair returns the private key as a PEM string, but pywebpush feeds it to py_vapid's Vapid.from_string, which base64-decodes and DER-parses its input. A raw PEM fails that as 'Could not deserialize key data' on EVERY send, and the per-subscription handler caught it as a generic warning and returned 'failed', so nothing ever pushed and there was no obvious signal. Convert the PEM to base64url-DER once at fan-out (_vapid_signing_key), verified live against Apple's push service (web.push.apple.com returns 201 and the notification arrives on the device). Also: - move the VAPID sub off the reserved .local domain to mailto:info@taos.my; Apple is stricter than other services about the sub claim. - log an ERROR (not the swallowed per-send warning) when the key cannot be converted, so a global misconfig is loud instead of disabling all push invisibly. A 100% failure that logs nothing is how this hid. - regression test: the converted key must be accepted by Vapid.from_string.
hognek
pushed a commit
to hognek/tinyagentos
that referenced
this pull request
Jul 30, 2026
…fc#2166) Web push has been 100% broken for all users. load_or_create_vapid_keypair returns the private key as a PEM string, but pywebpush feeds it to py_vapid's Vapid.from_string, which base64-decodes and DER-parses its input. A raw PEM fails that as 'Could not deserialize key data' on EVERY send, and the per-subscription handler caught it as a generic warning and returned 'failed', so nothing ever pushed and there was no obvious signal. Convert the PEM to base64url-DER once at fan-out (_vapid_signing_key), verified live against Apple's push service (web.push.apple.com returns 201 and the notification arrives on the device). Also: - move the VAPID sub off the reserved .local domain to mailto:info@taos.my; Apple is stricter than other services about the sub claim. - log an ERROR (not the swallowed per-send warning) when the key cannot be converted, so a global misconfig is loud instead of disabling all push invisibly. A 100% failure that logs nothing is how this hid. - regression test: the converted key must be accepted by Vapid.from_string.
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
1 participant
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.
What
Web push has been 100% broken for every user, silently. Found while live-debugging why a push never reached an Apple Watch (it was neither the network, the device, nor the cloud relay).
Root cause
load_or_create_vapid_keypairreturns the VAPID private key as a PEM string, butpywebpush.webpush(vapid_private_key=...)feeds it topy_vapid.Vapid.from_string, which base64-decodes and DER-parses its input. A raw PEM fails that asCould not deserialize key databefore any network call, on every send. The per-subscriptionexcept Exceptionhandler caught it, logged a generic warning, and returned"failed"-- so no push ever went out and there was no clear signal. Classic swallowed 100%-failure.Fix (verified live against Apple)
_vapid_signing_key()converts the PEM to the base64url-DER form pywebpush expects, once at fan-out. Confirmed:web.push.apple.comreturns HTTP 201 and the notification arrives on the device (verified on a real Apple Watch).submoved off the reserved.localdomain tomailto:info@taos.my; Apple is stricter than other push services about thesubclaim.Vapid.from_string(raw PEM would fail it).Impact
Fixes web-push notifications for all taOS users (desktop PWA and mobile/watch), not just the case that surfaced it. Needs deploying to the live instance to take effect there.
Summary by CodeRabbit