Security: enforce object-level authorization across the ability services (1.4.0) - #25
Merged
Merged
Conversation
Security audit of the whole plugin found 9 HIGH and 1 MEDIUM finding, all
caused by one architectural defect: authorization was expressed entirely as a
registration-time PRIMITIVE capability check, and src/Services/ contained zero
current_user_can() calls. A primitive capability says "may edit posts in
general", never "may edit THIS post", so any ability whose permission_callback
sat one notch too low granted unrestricted access to every object of that type.
It also meant WordPress's and WooCommerce's own protections — which live in the
meta-capability layer (map_meta_cap, wc_modify_map_meta_cap, get_editable_roles)
— never ran at all.
Verified against WP and WooCommerce source: wp_update_post(), wp_delete_post(),
wp_delete_attachment(), update_post_meta(), wp_set_password() and
WP_User::set_role() perform no capability check of their own. Authorization is
the caller's job, and this plugin was not doing it.
New, focused helpers carry the policy so it cannot drift:
Helpers/Capability object-level (meta) capability checks
Helpers/ProtectedMeta protected-key and privilege-key policy
Services/Meta/MetaGuard single entry point for the 12 meta methods
Findings fixed:
1 bulk-posts / update-post / delete-post had no per-object check, so a
Contributor could rewrite, publish and reassign any post of any type, and
permanently delete arbitrary posts site-wide (wp_delete_post($id, true)).
Every bulk item is now authorized individually.
2 The WooCommerce order abilities were gated on can_edit_posts (Contributor).
All 14 order-scoped registrations now use can_manage_orders, which already
existed and was already used by the sibling order abilities. The product
meta abilities move to a new can_manage_products for the same reason.
3 reset-password reset ANY account, including the administrator, silently
(send_notification:false) and returned the new plaintext password.
4 update-user validated the role against every registered role instead of
get_editable_roles(), so a delegated user manager could grant itself
'administrator'. Role changes now require promote_user on the target, and
the role is validated before anything is written so a rejected role cannot
leave the account half-updated.
5 wc-get-order-meta leaked protected order meta two ways: include_protected
was caller-supplied, and the single-key branch skipped the filter entirely
(exposing _order_key, _transaction_id, _customer_ip_address).
6 list-posts defaulted post_status to 'any', which makes WP_Query build only
negative status clauses and skip its capability mapping. Expanded to an
explicit list plus perm=editable; get-post now requires read_post.
7 The post and comment meta abilities had no per-object check, and the
single-key read branch had no protected-key filter at all.
8 delete-media/update-media acted on ANY attachment for anyone holding
upload_files, with force defaulting to true — permanent deletion including
the files on disk. force now defaults to false (trash).
9 set-user-meta could write {prefix}capabilities, which IS the role
assignment, bypassing both role validation and promote_user. Role, level
and session-token keys are now refused for every caller at every
capability, on the meta ability and on update-user's inline meta map alike.
10 wc-list-order-notes exposed internal staff/gateway notes to a Contributor.
Tests: 4 new suites (52 cases) covering every finding, RED before these
changes and GREEN after. The current_user_can and wp_register_ability stubs
became controllable so both the allowed and denied branch are exercised;
WP_Post, WP_User and WP_Query stubs were added.
composer phpcs, composer analyse (PHPStan L5) and composer test all green
(423 tests).
delete_theme() passed the caller's slug straight to core with no basename(), no validate_file() and no allowlist. WP_Theme::exists() looks like a guard but is not one: it only fails on theme_not_found, so '../plugins' resolves to an existing directory and passes, and core's delete_theme() then recursively deletes it — wp-content/plugins, uploads, or anything else the PHP user can unlink — with no containment check of its own. This is administrator-gated (install_themes), and an administrator already has code execution, so it is not a privilege boundary crossing and was not counted among the audit findings. It is worth fixing anyway because the ability is AI-agent-facing and annotated idempotent: a hallucinated or malformed slug turns a routine cleanup into an unrecoverable deletion with no confirmation. Mirrors the allowlist delete_plugin() already applies via get_plugins().
…valid Both found by testing against the live WooCommerce store rather than by the unit suite. 1. get-post leaked every protected meta key of any PUBLISHED post to anyone holding edit_posts. The capability check added earlier is correct and does its job — read_post is legitimately satisfied for a published post — but format_post() then returned the whole meta map, and collect_post_meta() stripped only _edit_lock, _edit_last and _thumbnail_id. Protected keys are where plugins keep API keys, licence keys, form payloads and, on non-HPOS stores, order PII. They are now hidden from non-administrators. Confirmed live: a Contributor reading a published post received _lwsec_secret before this change and an empty meta map after, while an administrator still receives the value. 2. bulk-posts returned HTTP 500 (ability_invalid_output) whenever any item failed: the output schema declares failed_ids as an array of integers, but the loop pushed [ 'id' => ..., 'reason' => ... ]. The defect predates this branch — it was reachable only when a post did not exist — but the new per-item authorization makes a failed item the normal path for a lower-privileged caller, so it went from rare to routine. failed_ids now carries integers as declared. CommentManager::bulk_comments had the same latent bug and is fixed with it. Test stubs gained the functions and WP_Post properties this exercises, and get_post_meta() now handles the "all meta" (empty key) branch the way WordPress does, which is what let the leak pass unit testing in the first place.
Contributor
Author
✅ Verified end-to-end on the live WooCommerce storeTested on croco2 (WP 7.0.3, WooCommerce 11.0.0, HPOS on) with real Contributor, Author and shop_manager accounts + application passwords, over the actual REST surface. The premise confirmed live — WooCommerce 11.0.0 still grants shop_manager the primitive capability the old code checked, while denying the meta capability the new code checks: Attacks — all closed
Admin regression — 8/8 unchanged
Two defects the live run caught that the unit suite did notBoth now fixed with regression tests (commit
Scratch users and posts removed from the test site afterwards. Local gate re-run: phpcs clean, PHPStan L5 no errors, 425/425 tests. |
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.
Fixes all 10 findings from the full-plugin security audit (9 HIGH, 1 MEDIUM).
Root cause
Authorization was expressed entirely as a registration-time primitive capability check, and
src/Services/contained zerocurrent_user_can()calls. A primitive capability means "may edit posts in general" — never "may edit THIS post". So any ability whosepermission_callbacksat one notch too low granted unrestricted access to every object of that type, and WordPress's and WooCommerce's own protections — which live in the meta-capability layer (map_meta_cap,wc_modify_map_meta_cap,get_editable_roles()) — never ran at all.Verified against WP and WooCommerce source:
wp_update_post(),wp_delete_post(),wp_delete_attachment(),update_post_meta(),wp_set_password()andWP_User::set_role()perform no capability check of their own. They are data-layer functions; authorization is the caller's job, and this plugin was not doing it.What an attacker could do (before this PR)
bulk-posts); rewrite/publish/reassign any post, page or product; read every author's drafts and private posts with full private meta; on WooCommerce: list every customer's name/email/phone, read protected order meta (_order_key,_transaction_id,_customer_ip_address), delete orders, create refunds, read internal staff notesforcedefaulted totrue)administratorviaroleor by writing{prefix}capabilitiesuser metashop_managermatters because WooCommerce grantsedit_usersto it unconditionally via auser_has_capfilter (wc-user-functions.php:684); its intended restriction (wc_modify_map_meta_cap) only runs for the meta caps this plugin never called.The fix
Object-level checks now run in the service layer, where the target ID is finally known. Three new focused helpers hold the policy so it cannot drift:
Helpers/Capability(112 lines) — object-level meta capability checksHelpers/ProtectedMeta(114 lines) — protected-key and privilege-key policyServices/Meta/MetaGuard(144 lines) — single entry point for the 12 meta methodsPer finding: per-object checks on posts/meta/media/users; the 14 WooCommerce order abilities moved to the existing
can_manage_orders;promote_user+get_editable_roles()on role changes (validated before any write, so a rejected role can't half-update an account); role/level/session-token meta keys refused for everyone; protected-key filter applied to single-key reads;post_statusno longer'any'plusperm => 'editable'.Tests
4 new suites, 52 cases, covering every finding — RED before, GREEN after. The
current_user_canandwp_register_abilitystubs became controllable so both the allowed and denied branch are exercised (previouslycurrent_user_canalways returnedfalse, so neither was); addedWP_Post,WP_User,WP_Queryand several function stubs.Gate:
composer phpcsclean ·composer analyse(PHPStan L5) no errors ·composer test423/423.Lower-privileged roles lose access they previously (incorrectly) had. Any integration running as Contributor, Author or WooCommerce shop_manager may now get
403 forbiddenwhere it previously succeeded — that is the fix working, but it is a breaking change for such setups. Administrators are unaffected. Hence the minor bump to 1.4.0.Two smaller behaviour changes worth calling out:
delete-medianow defaults to the trash instead of permanent deletion (passforce: truefor the old behaviour).list-postsno longer returns every status by default.Also included (separate commit)
delete-themepassed the caller's slug straight to core with novalidate_file()and no allowlist, so../pluginsrecursively deletedwp-content/plugins. This is administrator-gated and therefore not a privilege boundary crossing — it was excluded from the audit findings — but the ability is AI-agent-facing and annotated idempotent, so a hallucinated slug could destroy an unrelated directory. Now allowlisted againstwp_get_themes(), mirroring whatdelete_plugin()already does.Known follow-up (deliberately not in this PR)
UserManager.php(405) andMediaManager.php(404) crossed the project's 400-line hard limit, andMetaManager.php(529) /PostManager.php(821) were already above it and grew. The new logic went into new classes — the legacy files only received delegating guard calls — but the call sites still add lines. Splitting them is a separate refactor task requiring characterization tests first, and the project rules forbid mixing a security fix with a refactor.Audit areas confirmed clean, for the record: no SQL injection (all 5
$wpdbfiles verified), no XSS (the remote plugin registry is correctly escaped), no CSRF gap, no arbitrary option write, no object injection, no code-execution sink, and the MCP transport/discovery surface is correctlymanage_options-gated.