Repository navigation
Audit Fixes - #150
Audit Fixes#150
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
============================================
+ Coverage 98.23% 98.32% +0.09%
- Complexity 276 289 +13
============================================
Files 11 11
Lines 794 838 +44
============================================
+ Hits 780 824 +44
Misses 14 14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
VAPID validation can accept invalid or mismatched keys that break push authentication, while subscription recovery and legacy notification links also have unresolved regressions.
3 open issues
What changed in this PR
This PR addresses audit findings in the phpBB Browser Push Notifications extension, strengthening validation and adjusting subscription recovery and notification handling.
Changes:
- Adds endpoint and VAPID key validation, with delivery error handling.
- Updates browser subscription ownership checks, notification text decoding, and click URL resolution.
- Adds regression tests and English/Russian validation and moderation-log messages.
| File | Description |
|---|---|
ucp/controller/webpush.php |
Uses shared endpoint validation and updates notification rendering. |
tests/system/simple_test.php |
Tests moderation-log translations. |
tests/subscription/validator_test.php |
Adds VAPID key and endpoint validation cases. |
tests/notification/webpush_branches_test.php |
Tests handling of invalid VAPID configuration. |
tests/notification/notification_method_webpush_test.php |
Tests delivery isolation for malformed endpoints. |
tests/javascript/webpush_test.php |
Runs browser subscription checks through Node.js. |
tests/javascript/webpush_test.js |
Tests subscription ownership and refresh behavior. |
tests/javascript/push_worker_test.php |
Runs worker URL checks through Node.js. |
tests/javascript/push_worker_test.js |
Tests notification-click URL resolution and filtering. |
tests/controller/controller_webpush_test.php |
Updates endpoint, worker, and notification-text tests. |
tests/controller/controller_acp_test.php |
Tests VAPID validation when saving settings. |
subscription/validator.php |
Adds VAPID and endpoint URL validation helpers. |
styles/all/template/webpush.js |
Prevents automatic refresh of unrecognized subscriptions. |
styles/all/template/push_worker.js.twig |
Resolves notification links relative to the worker URL. |
notification/method/webpush.php |
Validates stored endpoints and handles initialization failures. |
language/ru/webpushnotifications_module_acp.php |
Adds Russian VAPID validation text. |
language/ru/info_mcp_webpushnotifications.php |
Adds Russian subscription-removal log text. |
language/en/webpushnotifications_module_acp.php |
Adds English VAPID validation text. |
language/en/info_mcp_webpushnotifications.php |
Adds English subscription-removal log text. |
controller/acp_controller.php |
Validates VAPID credentials before saving enabled settings. |
Review effort: Balanced
Checking current files and commit/index state. Copilot may be reviewing stale base, staged-only diff, or missing that fixes landed in separate commits. All three Copilot findings are stale/incorrect against current
401/403 no longer delete subscription rows. Cleanup now calls Thus ownership row survives rotation, and JavaScript guard permits automatic stale-key refresh. Tests explicitly verify 401/403 retention.
Legacy payload branch now converts relative URLs to board-absolute URLs before returning JSON: Current payload URLs remain unchanged and resolve against worker location. Tests cover
Copilot stops reading at
See pair validation. Mismatched valid keys have dedicated regression test. No further code changes needed. Copilot review should be rerun against current commit; these comments can be refuted with references above. |
|
|
||
| try { | ||
| const url = new URL(event.notification.data.url, '{{ U_BOARD_URL|e("js") }}/'); | ||
| const url = new URL(event.notification.data.url, self.location.href); |
No description provided.