Commit 0ff91ee0de5 for woocommerce
commit 0ff91ee0de580d96d996bc5b3ae4e590016672df
Author: Vlad Olaru <vlad.olaru@automattic.com>
Date: Wed May 20 06:19:55 2026 +0300
Fix latent shop_manager privilege escalation in inbox-note handler (#64428)
* fix(notes): require install_plugins cap on install-jp-and-wcs-plugins handler
The inbox-note handler `InstallJPAndWCSPlugins::install_jp_and_wcs_plugins`
runs through `POST /wc-analytics/admin/notes/{note_id}/action/{action_id}`,
whose route-level permission check resolves to `manage_woocommerce`. That
cap is also held by `shop_manager`, so a successful call would trigger
plugin install/activation under a non-admin role — privilege escalation
against `install_plugins`.
The call fatals today because the private helper invokes
`OnboardingPlugins::install_plugin()`, a method that does not exist on
that class. The endpoint returns 500 and no plugins install, but a
drive-by repair of the install path would silently turn the latent
escalation into a working one.
Add a `current_user_can( 'install_plugins' )` gate inside the handler,
mirroring the precedent in `WooCommercePayments::install_on_action()`.
The fix is intentionally scoped to the cap check; the broken installer
call is left as-is so the handler stays inert until follow-up work decides
whether to repair or remove it.
Cover the security property with a unit test that calls the handler as a
`shop_manager` and asserts it short-circuits without invoking the dead
installer path. Keep coverage on the existing wrong-note-name guard.
Refs WOOPLUG-6619
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(notes): throw on missing install_plugins cap to prevent silent action save
Returning silently from the cap check still let `Notes::trigger_note_action()`
persist the action's `E_WC_ADMIN_NOTE_ACTIONED` status after the hook
returned, so a `shop_manager` clicking the inbox-note button got a 200
response and saw the note marked actioned even though no install ran.
Throw an exception when the cap check fails so the REST callback aborts
before `$note->save()` runs, restoring the pre-fix behavior of leaving the
note in the inbox for unauthorized users. Update the unit test to assert
the exception.
Refs WOOPLUG-6619
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(notes): document @throws on install_jp_and_wcs_plugins for PHPCS
The branch-level lint (`lint:changes:branch`) flagged a missing
`@throws` tag now that the cap-check raises an exception. Per-file
`lint:php:changes` doesn't catch this because PHPCS only emits the
warning when looking at the full method, not just changed lines.
Refs WOOPLUG-6619
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Add .agents/ to .gitignore
* fix(notes): return 403 on inbox-note action cap failure, align message
The cap-check throw added in earlier commits aborts the action handler
correctly (preventing E_WC_ADMIN_NOTE_ACTIONED persistence), but
Notes::trigger_note_action() fires the hook via do_action() without a
try/catch, and NoteActions::trigger_note_action() never wrapped its
call to NotesFactory::trigger_note_action() either. WP_REST_Server
only converts WP_Error returns — bare \Exception bubbles up to PHP's
default handler and the unauthorized shop_manager sees a 500 instead
of a 403.
Wrap the call site in NoteActions in try/catch and surface the throw
as a WP_Error with rest_authorization_required_code(). The
handler-side throw stays; only the HTTP boundary changes.
Also align the exception message in install_jp_and_wcs_plugins() to
the wording standardized across REST permission checks three days
earlier in #64187 ("You do not have permissions to manage plugins.
Please contact your site administrator."). Reuses an existing
translation string and keeps the error UX consistent.
Refs WOOPLUG-6619
* test(notes): tighten assertions in InstallJPAndWCSPluginsTest
Two passes from the PR review:
- Pin the cap-check test to the specific exception message
(expectExceptionMessage) so a stack swap or a different throw can't
green-light it. Without this, expectException(\Exception::class) on
its own matches any exception thrown anywhere downstream.
- Replace assertTrue(true, ...) in the wrong-note guard test with
addToAssertionCount(1). assertTrue(true) is unconditionally true —
it passes even if the guard is deleted. The implicit
"no exception was thrown" already verifies the guard; the explicit
count keeps PHPUnit from flagging the test as risky.
Refs WOOPLUG-6619
* refactor(notes): narrow note-action catch to typed forbidden exception
The previous commit's catch in NoteActions::trigger_note_action() caught
every \Exception and mapped it to a 403. That worked because today only
our cap-check throws on this hook chain, but `woocommerce_note_action`
is a public hook — a third-party handler that throws for a non-auth
reason (network failure, transient DB error) would have its exception
masked as "forbidden" and its raw message leaked to clients.
Introduce NoteActionForbiddenException and throw it from
install_jp_and_wcs_plugins(). NoteActions now catches only that type;
any other exception bubbles uncaught so genuine server faults surface
as 500s. The exception lives in Internal\Admin\Notes — third parties
hooking the general filter aren't expected to extend it directly.
Also bring the test's assertion in line with the typed exception.
The PHPStan annotation on the new return matches the same baselined
`return.type` issue the file's other WP_Error returns already carry
(broken `@return` docblock — unqualified WP class names resolve in the
namespace); localized inline so the baseline doesn't grow.
Refs WOOPLUG-6619
* docs(notes): clarify cap-check rationale and changelog wording
Two follow-ups from PR review:
- The rationale comment in install_jp_and_wcs_plugins() said the
exception "aborts the REST callback before $note->save() runs". That's
imprecise: NoteActions::trigger_note_action() already calls
$note->set_is_read(true); $note->save() before dispatching this hook,
so the read-state save IS persisted regardless of cap-check outcome.
Reword to specify that only the status-saving $note->save() inside
Notes::trigger_note_action() is what's prevented, and note that the
earlier is_read save is intentional.
- The changelog said the latent installer is "unreachable to users
below administrator". install_plugins is a capability that can be
granted to non-admin roles via add_cap, so the role-based phrasing
overstates the constraint. Rephrase in terms of the capability
itself.
Refs WOOPLUG-6619
* ci(notes): end @phpstan-ignore line with period for Squiz sniff
The Squiz.Commenting.InlineComment.InvalidEndChar sniff applied during
branch-level lint requires inline comments to end with a full stop,
exclamation mark, or question mark. PHPStan's standalone
@phpstan-ignore-next-line directive ends with the error identifier
(return.type) — no terminal punctuation, so CI failed.
Use the documented `-- explanation` suffix PHPStan supports for inline
rationale. PHPStan still parses return.type as the identifier; PHPCS
sees a period at the end of the comment. Both happy.
Refs WOOPLUG-6619
* test(notes): add happy-path cap-check test; polish from review
Follow-ups from the approval review:
- Add test_install_jp_and_wcs_plugins_does_not_throw_for_admin_with_cap
to cover the cap-pass direction. The previous suite would have green-lit
an accidental cap-name typo because no test exercised the path where
current_user_can() returns true. New test wraps the call in try/catch,
fails on NoteActionForbiddenException, and swallows other exceptions —
installer failures in the unit-test runtime are out of scope.
- Replace addToAssertionCount(1) with expectNotToPerformAssertions() in
the wrong-note test to match the EmailPreviewTest/FilesystemUtilTest
idiom — same risky-test suppression, more declarative.
- Loosen the deny-test's message assertion from a full-string match to a
short substring (expectExceptionMessageMatches). The type assertion is
now the behavioral guarantee; the message check survives legitimate
copy edits.
- Expand NoteActionForbiddenException's docblock to document the hook
abort behavior (WP_Hook::do_action doesn't catch — lower-priority
callbacks are skipped) and narrow the "@internal" framing so it's
clear third parties hooking woocommerce_note_action[_*] should not
rely on this class.
The esc_html__ on the throw stays as-is — PHPCS's
WordPress.Security.EscapeOutput.ExceptionNotEscaped sniff requires
escaping for exception messages, and dropping it fails CI. The 7 other
plugin-cap-check sites using plain __() are WP_Error returns, not
throws — different sniff scope.
Refs WOOPLUG-6619
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/.gitignore b/.gitignore
index 77c9226732e..f4dbd5aeef9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@ changes.json
.claude/docs/analysis/
.claude/docs/research/
CLAUDE.local.md
+.agents/
# AI agent scratchpad and temp (scaffold-project)
.agents/scratchpad/**
diff --git a/plugins/woocommerce/changelog/fix-wooplug-6619-install-jp-wcs-plugins-cap-check b/plugins/woocommerce/changelog/fix-wooplug-6619-install-jp-wcs-plugins-cap-check
new file mode 100644
index 00000000000..c3f0b0a57ac
--- /dev/null
+++ b/plugins/woocommerce/changelog/fix-wooplug-6619-install-jp-wcs-plugins-cap-check
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Add install_plugins capability check to the install-jp-and-wcs-plugins inbox-note action handler so the latent installer call is unreachable to users without the `install_plugins` capability.
diff --git a/plugins/woocommerce/src/Admin/API/NoteActions.php b/plugins/woocommerce/src/Admin/API/NoteActions.php
index f51b5d48618..cf043762cbd 100644
--- a/plugins/woocommerce/src/Admin/API/NoteActions.php
+++ b/plugins/woocommerce/src/Admin/API/NoteActions.php
@@ -11,6 +11,7 @@ defined( 'ABSPATH' ) || exit;
use Automattic\WooCommerce\Admin\Notes\Note;
use Automattic\WooCommerce\Admin\Notes\Notes as NotesFactory;
+use Automattic\WooCommerce\Internal\Admin\Notes\NoteActionForbiddenException;
/**
* REST API Admin Note Action controller class.
@@ -79,7 +80,27 @@ class NoteActions extends Notes {
);
}
- $triggered_note = NotesFactory::trigger_note_action( $note, $triggered_action );
+ try {
+ $triggered_note = NotesFactory::trigger_note_action( $note, $triggered_action );
+ } catch ( NoteActionForbiddenException $e ) {
+ // Handlers hooked into `woocommerce_note_action[_*]` throw this typed
+ // exception when the current user lacks the per-action capability the
+ // handler enforces (the route-level permission check is intentionally
+ // coarser). Convert it to a 403 so REST clients get correct HTTP
+ // semantics. Any other exception bubbles uncaught so genuine server
+ // faults surface as 500s instead of being masked as auth errors.
+ //
+ // The ignore below matches the same `return.type` issue already captured
+ // in the PHPStan baseline for the other two WP_Error returns in this
+ // method (broken `@return` docblock — unqualified WP class names resolve
+ // in the current namespace). Localized here so the baseline doesn't grow.
+ // @phpstan-ignore-next-line return.type -- see rationale above.
+ return new \WP_Error(
+ 'woocommerce_note_action_forbidden',
+ $e->getMessage(),
+ array( 'status' => rest_authorization_required_code() )
+ );
+ }
$data = $triggered_note->get_data();
$data = $this->prepare_item_for_response( $data, $request );
diff --git a/plugins/woocommerce/src/Internal/Admin/Notes/InstallJPAndWCSPlugins.php b/plugins/woocommerce/src/Internal/Admin/Notes/InstallJPAndWCSPlugins.php
index 5a43c0474f5..433499be710 100644
--- a/plugins/woocommerce/src/Internal/Admin/Notes/InstallJPAndWCSPlugins.php
+++ b/plugins/woocommerce/src/Internal/Admin/Notes/InstallJPAndWCSPlugins.php
@@ -99,12 +99,32 @@ class InstallJPAndWCSPlugins {
* being clicked in the admin note.
*
* @param Note $note The note being actioned.
+ *
+ * @throws NoteActionForbiddenException When the current user lacks the `install_plugins` capability.
*/
public function install_jp_and_wcs_plugins( $note ) {
if ( self::NOTE_NAME !== $note->get_name() ) {
return;
}
+ // The route-level permission check on `POST /wc-analytics/admin/notes/.../action/...`
+ // only requires `manage_woocommerce`, which `shop_manager` satisfies. Plugin install
+ // requires the dedicated `install_plugins` capability — gate per-handler.
+ //
+ // Throwing (rather than returning silently) prevents `Notes::trigger_note_action()`
+ // from persisting the action's `E_WC_ADMIN_NOTE_ACTIONED` status: the exception
+ // aborts the dispatch before the status-saving `$note->save()` inside
+ // `Notes::trigger_note_action()` runs, so the note stays actionable in the inbox.
+ // (The REST controller has already saved `is_read = true` at this point — that
+ // earlier save is intentional and unaffected by the cap check.)
+ // `NoteActions::trigger_note_action()` catches the typed exception and maps it
+ // to a 403 REST response.
+ if ( ! current_user_can( 'install_plugins' ) ) {
+ throw new NoteActionForbiddenException(
+ esc_html__( 'You do not have permissions to manage plugins. Please contact your site administrator.', 'woocommerce' )
+ );
+ }
+
$this->install_and_activate_plugin( 'jetpack' );
$this->install_and_activate_plugin( 'woocommerce-services' );
}
diff --git a/plugins/woocommerce/src/Internal/Admin/Notes/NoteActionForbiddenException.php b/plugins/woocommerce/src/Internal/Admin/Notes/NoteActionForbiddenException.php
new file mode 100644
index 00000000000..af7a1ddff95
--- /dev/null
+++ b/plugins/woocommerce/src/Internal/Admin/Notes/NoteActionForbiddenException.php
@@ -0,0 +1,36 @@
+<?php
+/**
+ * Note Action Forbidden Exception.
+ */
+
+declare( strict_types=1 );
+
+namespace Automattic\WooCommerce\Internal\Admin\Notes;
+
+defined( 'ABSPATH' ) || exit;
+
+/**
+ * Thrown by internal WooCommerce handlers on the `woocommerce_note_action[_*]`
+ * hooks to signal that the current user lacks the per-action capability the
+ * handler enforces.
+ *
+ * `Automattic\WooCommerce\Admin\API\NoteActions::trigger_note_action()` catches
+ * this exception and converts it to a 403 REST response. Any other exception
+ * type bubbles uncaught so genuine server faults are not masked as auth errors.
+ *
+ * Hook abort behavior: when a handler throws, `WP_Hook::do_action()` does not
+ * catch the exception, so any lower-priority callbacks registered on the same
+ * `woocommerce_note_action_<name>` hook are silently skipped, and
+ * `Notes::trigger_note_action()` does not run its post-hook
+ * `$note->set_status()`/`$note->save()` step — keeping the note actionable in
+ * the inbox.
+ *
+ * This is an internal contract used by first-party note-action handlers in the
+ * `Internal\\Admin\\Notes\\` namespace. Third-party extensions hooking into
+ * `woocommerce_note_action[_*]` should not rely on this class — its API and
+ * existence may change.
+ *
+ * @internal
+ * @since 10.9.0
+ */
+class NoteActionForbiddenException extends \Exception {}
diff --git a/plugins/woocommerce/tests/php/src/Internal/Admin/Notes/InstallJPAndWCSPluginsTest.php b/plugins/woocommerce/tests/php/src/Internal/Admin/Notes/InstallJPAndWCSPluginsTest.php
new file mode 100644
index 00000000000..facd05eaf67
--- /dev/null
+++ b/plugins/woocommerce/tests/php/src/Internal/Admin/Notes/InstallJPAndWCSPluginsTest.php
@@ -0,0 +1,118 @@
+<?php
+/**
+ * Tests for InstallJPAndWCSPlugins class.
+ */
+
+declare( strict_types=1 );
+
+namespace Automattic\WooCommerce\Tests\Internal\Admin\Notes;
+
+use Automattic\WooCommerce\Admin\Notes\Note;
+use Automattic\WooCommerce\Internal\Admin\Notes\InstallJPAndWCSPlugins;
+use Automattic\WooCommerce\Internal\Admin\Notes\NoteActionForbiddenException;
+use WC_Unit_Test_Case;
+
+/**
+ * Class InstallJPAndWCSPluginsTest
+ */
+class InstallJPAndWCSPluginsTest extends WC_Unit_Test_Case {
+
+ /**
+ * The System Under Test.
+ *
+ * @var InstallJPAndWCSPlugins
+ */
+ private $sut;
+
+ /**
+ * Set up test fixtures.
+ */
+ public function setUp(): void {
+ parent::setUp();
+ $this->sut = new InstallJPAndWCSPlugins();
+ }
+
+ /**
+ * Tear down test fixtures.
+ */
+ public function tearDown(): void {
+ wp_set_current_user( 0 );
+ parent::tearDown();
+ }
+
+ /**
+ * @testdox Should throw when triggered by a user without the install_plugins capability (e.g. shop_manager).
+ */
+ public function test_install_jp_and_wcs_plugins_throws_when_user_lacks_install_plugins_cap(): void {
+ $shop_manager_id = self::factory()->user->create( array( 'role' => 'shop_manager' ) );
+ wp_set_current_user( $shop_manager_id );
+
+ $this->assertFalse(
+ current_user_can( 'install_plugins' ),
+ 'shop_manager should not have install_plugins capability'
+ );
+
+ $note = new Note();
+ $note->set_name( InstallJPAndWCSPlugins::NOTE_NAME );
+
+ // The handler must throw the typed exception — a silent return would let
+ // Notes::trigger_note_action() continue and persist E_WC_ADMIN_NOTE_ACTIONED on
+ // the note despite no install running. Asserting on the specific class is what
+ // lets NoteActions::trigger_note_action() map this and only this to a 403.
+ // The exception class is the behavioral guarantee; a substring match on the
+ // message keeps the test from breaking on legitimate copy rewords.
+ $this->expectException( NoteActionForbiddenException::class );
+ $this->expectExceptionMessageMatches( '/permissions to manage plugins/' );
+
+ $this->sut->install_jp_and_wcs_plugins( $note );
+ }
+
+ /**
+ * @testdox Should not throw NoteActionForbiddenException when triggered by a user with the install_plugins capability (e.g. administrator).
+ */
+ public function test_install_jp_and_wcs_plugins_does_not_throw_for_admin_with_cap(): void {
+ $admin_id = self::factory()->user->create( array( 'role' => 'administrator' ) );
+ wp_set_current_user( $admin_id );
+
+ $this->assertTrue(
+ current_user_can( 'install_plugins' ),
+ 'administrator should have install_plugins capability'
+ );
+
+ $note = new Note();
+ $note->set_name( InstallJPAndWCSPlugins::NOTE_NAME );
+
+ // We assert only on the cap gate: the handler must NOT throw
+ // NoteActionForbiddenException for a user that has the capability. The handler
+ // continues into install_and_activate_plugin() which calls the real
+ // OnboardingPlugins installer and may fail for environmental reasons
+ // (network, missing dependencies in the unit-test runtime). Those failures
+ // are not under test here; only the cap-check direction matters. The
+ // assertTrue() precondition above keeps PHPUnit from marking the test risky.
+ try {
+ $this->sut->install_jp_and_wcs_plugins( $note );
+ } catch ( NoteActionForbiddenException $e ) {
+ $this->fail( 'Cap check should pass for admin, but threw NoteActionForbiddenException: ' . $e->getMessage() );
+ } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- intentional: only the cap-check direction is under test, installer failures are out of scope.
+ // Installer failures in the unit-test environment are expected and ignored.
+ }
+ }
+
+ /**
+ * @testdox Should ignore notes whose name does not match the handler's note name.
+ */
+ public function test_install_jp_and_wcs_plugins_ignores_wrong_note(): void {
+ $admin_id = self::factory()->user->create( array( 'role' => 'administrator' ) );
+ wp_set_current_user( $admin_id );
+
+ $other_note = new Note();
+ $other_note->set_name( 'some-other-note' );
+
+ // Wrong-note guard must short-circuit before any cap check or install attempt.
+ // expectNotToPerformAssertions() declares the intent in the Arrange phase:
+ // reaching the next line without an exception is the entire success condition.
+ $this->expectNotToPerformAssertions();
+
+ $this->sut->install_jp_and_wcs_plugins( $other_note );
+ }
+}