Clean up stale subscriptions (403 errors)#128
Open
iMattPro wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Web Push subscription hygiene by treating HTTP 403 delivery failures as permanent (triggering cleanup), and by enabling the client to detect/repair stale subscriptions (e.g., after VAPID key rotation) by unsubscribing/resubscribing and sending the replacement subscription to the backend.
Changes:
- Treat HTTP 403 “Forbidden” push responses as permanent authorization failures and include them in delivery cleanup.
- Add client-side detection of stale subscriptions (unknown endpoint or VAPID key mismatch) and automatic refresh + re-persist to the backend.
- When saving a refreshed subscription, delete the prior endpoint row (and any existing row for the new endpoint) to avoid leaving stale records behind.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
notification/method/webpush.php |
Treats 403 like 401 for “permanent authorization failure” cleanup decisions. |
ucp/controller/webpush.php |
Accepts previous_endpoint and expirationTime, deletes old endpoint rows, inserts refreshed subscription. |
styles/all/template/webpush.js |
Adds logic to reconcile browser subscription with server state and VAPID key; refreshes subscription when needed. |
tests/notification/notification_method_webpush_test.php |
Adds coverage for 403 handling in is_subscription_unauthorized(). |
tests/controller/controller_webpush_test.php |
Adds coverage ensuring a refreshed subscription replaces the previous endpoint record. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Treats HTTP 403 Forbidden push failures as permanent delivery failures and removes the affected subscription from wpn_push_subscriptions during delivery cleanup. That change is in
notification/method/webpush.php, alongside the existing expired/401 cleanup path.It also now supports client-side resubscription for stale or rotated subscriptions. In
styles/all/template/webpush.js, the frontend checks the current push permission/subscription on load, compares the browser subscription against the server-known endpoints and current VAPID public key, callsunsubscribe()when the subscription is stale, then immediately resubscribes with the current key and posts the replacement payload back to the backend.On the backend,
ucp/controller/webpush.phpnow replaces the old endpoint row when a refreshed subscription is saved instead of leaving the stale record behind.