Close unauthenticated stored injection via predictable API key#131
Open
ShahrearMSf wants to merge 1 commit into
Open
Close unauthenticated stored injection via predictable API key#131ShahrearMSf wants to merge 1 commit into
ShahrearMSf wants to merge 1 commit into
Conversation
Root cause
The REST route POST /wp-json/notificationx/v1/notification/{id} (and its legacy
alias /wp-json/notificationx/notification/{id}) accepted md5(home_url()) as its
only "authentication". Since the value is derivable from the public homepage URL
(and NotificationX printed it in its own admin docs), any anonymous visitor could
POST arbitrary social-proof entries — fake purchases, signups, reviews — that
NotificationX then persisted to wp_nx_entries and served back to every visitor as
authentic-looking popups.
The fix
* Free — Integration.php now generates a 32-char random key per site
(nx_integration_api_key option), validated with hash_equals(). The predictable
md5(home_url()) key is accepted only during a 14-day post-upgrade grace window
and only on sites that actually had legacy Zapier/IFTTT notifications
configured before the upgrade — never on fresh installs, and never lazily
opened by an attacker's own probe (the seeder runs on plugins_loaded and is
version-gated by nx_integration_key_seeded_version). Every accepted legacy
request updates nx_integration_legacy_key_last_used_at so a persistent admin
notice can nudge the customer to rotate before/after the deadline. The id-
oracle GET path and both REST route registrations (v1 + the pre-namespaced
legacy alias) go through the same is_valid_api_key() gate.
* Pro — Zapier and IFTTT settings UIs now advertise the new random key via
method_exists( '\NotificationX\Core\Rest\Integration', 'get_api_key' ). A
fallback to md5(home_url()) is preserved so Pro does not fatal against older
Free versions that lack get_api_key(). The IFTTT setup-doc echo of the API
key is now wrapped in esc_html(), and the reflected $_GET['post'] on the same
line is now escaped too.
How the fix was tested (14 phases, locally on test.local)
1. Deploy + PHP lint + smoke — all changed files lint clean, homepage 200,
wp-admin 302, wp-json 200, debug.log clean.
2. Fresh install (no zap notif ever) — grace never opens, legacy key rejected,
new key accepted, id-oracle safe.
3. Existing legacy site (enabled Zapier notif) — grace opens for 14 days, both
legacy and new key POST succeed, entries persist to wp_nx_entries,
legacy_key_last_used_at gets set so the admin notice can fire.
4. Post-grace (backdated 15 days) — legacy REJECTED, new key still works.
5. Legacy endpoint alias behaves identically to /v1/.
6. Attacker probe on a truly fresh site (all options false) — POST rejected,
grace_started_at and legacy_key_last_used_at both remain false. Attacker
cannot lazy-open the grace window.
7. Non-Zapier types unaffected — WooCommerce order transition still populates
entries; frontend REST /notice returns 200 with the notification; the
multiorder_combine path still returns its expected combined title.
8. PHP fatal sweep — 13 admin/frontend URLs, all HTTP 200, 0 fatals, 0
warnings, 0 notices.
9. Pro UI — Zapier default, IFTTT default, and IFTTT doc() HTML all show the
new random key. md5(home_url()) does not leak into the doc output.
10. Cross-version matrix — new Free + old Pro loads without fatal and still
rejects the legacy attack via Free's grace gate; old Free + new Pro loads
without fatal because Pro's method_exists() guard falls back to md5().
11. Patchstack PoC replay — all 8 gist steps rejected post-fix. Verified the
exact same PoC succeeds on master (control) so the fix is what closes it.
12. PHP compatibility — no PHP 8.0+-only features (no nullsafe, match,
constructor promotion, readonly, enum). hash_equals is PHP 5.6+; bool
return type hint is 7.0+. Minimum PHP required by the change: 7.1.
13. Repo hygiene — clean, no untracked, no staged, no whitespace errors.
14. JS console sweep — only pre-existing WP-core React SVG warning; no new
errors introduced by the change.
Reported this vulnerability to the team more than two months ago; pushing the
fix here to help unblock the dev team.
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.
Root cause
The REST route
POST /wp-json/notificationx/v1/notification/{id}(and its legacy alias/wp-json/notificationx/notification/{id}) acceptedmd5(home_url())as its only "authentication". Since the value is derivable from the public homepage URL — and NotificationX printed it in its own admin docs — any anonymous visitor could POST arbitrary social-proof entries (fake purchases, signups, reviews) that NotificationX then persisted towp_nx_entriesand served back to every visitor as authentic-looking popups.The fix
Integration.phpnow generates a 32-char random key per site (nx_integration_api_keyoption), validated withhash_equals().md5(home_url())key is accepted only during a 14-day post-upgrade grace window, and only on sites that actually had legacy Zapier / IFTTT notifications configured before the upgrade. Never on fresh installs. Never lazily opened by an attacker's own probe — the seeder runs onplugins_loadedand is version-gated bynx_integration_key_seeded_version.nx_integration_legacy_key_last_used_at, and a persistent admin notice nudges the customer to rotate before the deadline (warning-class during grace, error-class post-grace).GETpath and both REST route registrations (/v1/+ the pre-namespaced legacy alias) go through the sameis_valid_api_key()gate.Integration::get_api_key()returns the stored key without any grace-window side effects, so Pro can safely read it from its Settings UIs.Coordinates with
Pro companion PR on branch
81266-sec-issue— updates the Zapier and IFTTT settings UIs to advertise the new random key through\NotificationX\Core\Rest\Integration::get_api_key(), wrapped inmethod_exists()so old Pro against new Free still boots.Tested
/200;wp-admin/302;wp-json/200;debug.logclean.wp_nx_entries;legacy_key_last_used_atset so admin notice fires./v1/.grace_started_at+legacy_key_last_used_atremain false. Attacker cannot lazy-open the grace window./wp-json/notificationx/v1/noticereturns 200;multiorder_combinestill returns its expected combined title.method_exists()guard falls back tomd5().master(control) so the fix is what closes it.hash_equalsis PHP 5.6+;boolreturn type hint is 7.0+. Minimum PHP required by the change: 7.1.Card
Notes