diff --git a/modules/notifications/notifications.php b/modules/notifications/notifications.php index f27d11d6..3c3acfe0 100644 --- a/modules/notifications/notifications.php +++ b/modules/notifications/notifications.php @@ -554,8 +554,9 @@ function ( $user_id ) use ( $post_id ) { * @since 0.8 */ public function handle_user_post_subscription() { + // Require a valid nonce for this AJAX request. // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Nonce value passed directly to wp_verify_nonce(). - if ( ! empty( $_GET['_wpnonce'] ) && ! wp_verify_nonce( $_GET['_wpnonce'], 'ef_notifications_user_post_subscription' ) ) { + if ( ! isset( $_GET['_wpnonce'] ) || ! wp_verify_nonce( $_GET['_wpnonce'], 'ef_notifications_user_post_subscription' ) ) { $this->print_ajax_response( 'error', $this->module->messages['nonce-failed'] ); } @@ -599,21 +600,30 @@ public function save_post_subscriptions( $new_status, $old_status, $post ) { return; } + // Only process if Edit Flow's followers form was submitted. + if ( ! isset( $_POST['ef-save_followers'] ) ) { + return; + } + + // Check capability. + if ( ! current_user_can( $this->edit_post_subscriptions_cap ) ) { + return; + } + + // Verify Edit Flow's own nonce. Return early if missing or invalid - don't die, + // as this hook fires on all post transitions including non-admin contexts. // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Nonce value passed directly to wp_verify_nonce(). - if ( ! empty( $_POST['_wpnonce'] ) && ! wp_verify_nonce( $_POST['_wpnonce'], 'update-post_' . $post->ID ) ) { - $this->print_ajax_response( 'error', $this->module->messages['nonce-failed'] ); + if ( ! isset( $_POST['ef_notifications_nonce'] ) || ! wp_verify_nonce( $_POST['ef_notifications_nonce'], 'save_user_usergroups' ) ) { + return; } - // Only if has edit_post_subscriptions cap. - if ( isset( $_POST['ef-save_followers'] ) && current_user_can( $this->edit_post_subscriptions_cap ) ) { - // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Values are sanitized when saved. - $users = isset( $_POST['ef-selected-users'] ) ? $_POST['ef-selected-users'] : []; - // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Values are sanitized when saved. - $usergroups = isset( $_POST['following_usergroups'] ) ? $_POST['following_usergroups'] : []; - $this->save_post_following_users( $post, $users ); - if ( $this->module_enabled( 'user_groups' ) && in_array( $this->get_current_post_type(), $this->get_post_types_for_module( $edit_flow->user_groups->module ) ) ) { - $this->save_post_following_usergroups( $post, $usergroups ); - } + // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Values are sanitized when saved. + $users = isset( $_POST['ef-selected-users'] ) ? $_POST['ef-selected-users'] : []; + // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Values are sanitized when saved. + $usergroups = isset( $_POST['following_usergroups'] ) ? $_POST['following_usergroups'] : []; + $this->save_post_following_users( $post, $users ); + if ( $this->module_enabled( 'user_groups' ) && in_array( $this->get_current_post_type(), $this->get_post_types_for_module( $edit_flow->user_groups->module ) ) ) { + $this->save_post_following_usergroups( $post, $usergroups ); } } diff --git a/tests/Integration/NotificationsAjaxTest.php b/tests/Integration/NotificationsAjaxTest.php index bb36d24b..121f036f 100644 --- a/tests/Integration/NotificationsAjaxTest.php +++ b/tests/Integration/NotificationsAjaxTest.php @@ -270,4 +270,109 @@ public function test_ajax_save_persists_usergroup_subscriptions(): void { $edit_flow->user_groups->delete_usergroup( $group1->term_id ); $edit_flow->user_groups->delete_usergroup( $group2->term_id ); } + + /** + * Test: handle_user_post_subscription requires a nonce. + * + * This tests the fix for the nonce check logic. Previously the check was: + * `if ( ! empty( $_GET['_wpnonce'] ) && ! wp_verify_nonce(...) )` + * which allowed requests without any nonce to pass through. + * + * The fix changes it to: + * `if ( ! isset( $_GET['_wpnonce'] ) || ! wp_verify_nonce(...) )` + * which requires a valid nonce. + * + * @ticket https://github.com/Automattic/edit-flow/issues/882 + * @covers EF_Notifications::handle_user_post_subscription + */ + public function test_handle_user_post_subscription_requires_nonce(): void { + wp_set_current_user( self::$admin_user_id ); + + $post_id = $this->factory->post->create(); + + // Make request WITHOUT a nonce - this should fail now. + $_GET['post_id'] = $post_id; + $_GET['method'] = 'follow'; + // Intentionally NOT setting $_GET['_wpnonce'] + + // print_ajax_response outputs JSON before dying, so we get WPAjaxDieContinueException. + try { + $this->_handleAjax( 'ef_notifications_user_post_subscription' ); + } catch ( WPAjaxDieContinueException $e ) { + unset( $e ); + } + + // Verify error response. + $response = json_decode( $this->_last_response, true ); + $this->assertIsArray( $response, 'Response should be valid JSON' ); + $this->assertEquals( 'error', $response['status'], 'Response should indicate error when nonce is missing' ); + } + + /** + * Test: handle_user_post_subscription fails with invalid nonce. + * + * @ticket https://github.com/Automattic/edit-flow/issues/882 + * @covers EF_Notifications::handle_user_post_subscription + */ + public function test_handle_user_post_subscription_fails_with_invalid_nonce(): void { + wp_set_current_user( self::$admin_user_id ); + + $post_id = $this->factory->post->create(); + + $_GET['_wpnonce'] = 'invalid_nonce'; + $_GET['post_id'] = $post_id; + $_GET['method'] = 'follow'; + + // print_ajax_response outputs JSON before dying, so we get WPAjaxDieContinueException. + try { + $this->_handleAjax( 'ef_notifications_user_post_subscription' ); + } catch ( WPAjaxDieContinueException $e ) { + unset( $e ); + } + + // Verify error response. + $response = json_decode( $this->_last_response, true ); + $this->assertIsArray( $response, 'Response should be valid JSON' ); + $this->assertEquals( 'error', $response['status'], 'Response should indicate error when nonce is invalid' ); + } + + /** + * Test: handle_user_post_subscription succeeds with valid nonce. + * + * @ticket https://github.com/Automattic/edit-flow/issues/882 + * @covers EF_Notifications::handle_user_post_subscription + */ + public function test_handle_user_post_subscription_succeeds_with_valid_nonce(): void { + global $edit_flow; + + wp_set_current_user( self::$admin_user_id ); + + $post_id = $this->factory->post->create( + array( + 'post_author' => self::$admin_user_id, + 'post_status' => 'draft', + ) + ); + + $_GET['_wpnonce'] = wp_create_nonce( 'ef_notifications_user_post_subscription' ); + $_GET['post_id'] = $post_id; + $_GET['method'] = 'follow'; + + try { + $this->_handleAjax( 'ef_notifications_user_post_subscription' ); + } catch ( WPAjaxDieContinueException $e ) { + unset( $e ); + } + + // Verify we got a success response. + $this->assertNotEmpty( $this->_last_response, 'AJAX should return a response' ); + + $response = json_decode( $this->_last_response, true ); + $this->assertIsArray( $response, 'Response should be valid JSON' ); + $this->assertEquals( 'success', $response['status'], 'Response should indicate success' ); + + // Verify the user is now following the post. + $following_users = $edit_flow->notifications->get_following_users( $post_id, 'id' ); + $this->assertContains( self::$admin_user_id, $following_users, 'User should be following the post' ); + } } diff --git a/tests/Integration/NotificationsClassicEditorTest.php b/tests/Integration/NotificationsClassicEditorTest.php index d25e403c..f63ec1ca 100644 --- a/tests/Integration/NotificationsClassicEditorTest.php +++ b/tests/Integration/NotificationsClassicEditorTest.php @@ -1,8 +1,9 @@ notifications->save_post_subscriptions( 'draft', 'draft', $post ); } catch ( \Exception $e ) { $exception_thrown = true; } - // The function should NOT call wp_die when given a valid Classic Editor nonce. - // If this assertion fails, it means the nonce check is incorrectly rejecting - // valid Classic Editor nonces (the bug reported in 0.10.0). $this->assertFalse( $wp_die_called, - 'save_post_subscriptions should accept valid Classic Editor nonce (update-post_{$post_id}), but it called wp_die instead. This indicates the nonce action string is incorrect.' + 'save_post_subscriptions should not call wp_die when given a valid Edit Flow nonce.' ); $this->assertFalse( $exception_thrown, - 'save_post_subscriptions threw an exception due to wp_die being called with a valid nonce.' + 'save_post_subscriptions should not throw an exception with a valid Edit Flow nonce.' ); } /** - * Test that saving a post via wp_update_post does not fail due to revision nonce mismatch. + * Test that save_post_subscriptions ignores requests without Edit Flow form data. * - * When WordPress saves a post, it creates a revision which triggers transition_post_status - * with a different post ID (the revision ID). The nonce was created for the original post, - * so if save_post_subscriptions doesn't skip revisions, the nonce check will fail. + * When ef-save_followers is not set, the function should return early + * without processing or dying. * - * @ticket https://wordpress.org/support/topic/upgrading-to-0-10-0-breaks-funtionality-for-editor-role/ + * @covers EF_Notifications::save_post_subscriptions */ - public function test_save_post_with_revision_does_not_fail_nonce_check() { - // Create a post. + public function test_save_post_subscriptions_ignores_non_edit_flow_requests() { + global $edit_flow; + + $post_id = self::factory()->post->create( + array( + 'post_author' => self::$editor_user_id, + 'post_status' => 'draft', + ) + ); + + $post = get_post( $post_id ); + + // Simulate a request without Edit Flow form data (e.g., REST API, other forms). + $_POST = array(); + + $wp_die_called = false; + + add_filter( + 'wp_die_handler', + function () use ( &$wp_die_called ) { + return function ( $message ) use ( &$wp_die_called ) { + $wp_die_called = true; + throw new \Exception( 'wp_die called: ' . $message ); + }; + } + ); + + $exception_thrown = false; + try { + $edit_flow->notifications->save_post_subscriptions( 'draft', 'draft', $post ); + } catch ( \Exception $e ) { + $exception_thrown = true; + } + + $this->assertFalse( + $wp_die_called, + 'save_post_subscriptions should return early without dying when ef-save_followers is not set.' + ); + $this->assertFalse( + $exception_thrown, + 'save_post_subscriptions should not throw when ef-save_followers is not set.' + ); + } + + /** + * Test that save_post_subscriptions doesn't die when another form's nonce is present. + * + * This is the core fix for #882: when a contact form (or any other form) triggers + * a post status transition with its own _wpnonce field, Edit Flow should not + * kill the request by calling wp_die(). + * + * @ticket https://github.com/Automattic/edit-flow/issues/882 + * @covers EF_Notifications::save_post_subscriptions + */ + public function test_save_post_subscriptions_does_not_die_with_unrelated_nonce() { + global $edit_flow; + + $post_id = self::factory()->post->create( + array( + 'post_author' => self::$editor_user_id, + 'post_status' => 'draft', + ) + ); + + $post = get_post( $post_id ); + + // Simulate a contact form submission that happens to trigger post status change. + // The form has its own _wpnonce for a different action (e.g., speaker_submission). + $_POST['_wpnonce'] = wp_create_nonce( 'speaker_submission' ); + // Note: ef-save_followers is NOT set because this is not an Edit Flow form. + + $wp_die_called = false; + + add_filter( + 'wp_die_handler', + function () use ( &$wp_die_called ) { + return function ( $message ) use ( &$wp_die_called ) { + $wp_die_called = true; + throw new \Exception( 'wp_die called: ' . $message ); + }; + } + ); + + $exception_thrown = false; + try { + $edit_flow->notifications->save_post_subscriptions( 'draft', 'publish', $post ); + } catch ( \Exception $e ) { + $exception_thrown = true; + } + + $this->assertFalse( + $wp_die_called, + 'save_post_subscriptions should NOT die when an unrelated form triggers post transition. ' . + 'This was the bug in #882 where contact form submissions failed because Edit Flow ' . + 'was checking the generic _wpnonce field.' + ); + $this->assertFalse( + $exception_thrown, + 'save_post_subscriptions should not throw when an unrelated form triggers post transition.' + ); + } + + /** + * Test that save_post_subscriptions returns early with invalid Edit Flow nonce. + * + * When Edit Flow's form is submitted but the nonce is invalid, the function + * should return early without processing (and importantly, without dying). + * + * @covers EF_Notifications::save_post_subscriptions + */ + public function test_save_post_subscriptions_returns_with_invalid_nonce() { + global $edit_flow; + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_user_id, @@ -139,12 +243,13 @@ public function test_save_post_with_revision_does_not_fail_nonce_check() { ) ); - // Simulate Classic Editor POST request with a valid nonce for the original post. - $_POST['_wpnonce'] = wp_create_nonce( 'update-post_' . $post_id ); - $_POST['ef-save_followers'] = '1'; - $_POST['ef-selected-users'] = array( self::$editor_user_id ); + $post = get_post( $post_id ); + + // Simulate Edit Flow form submission with an invalid nonce. + $_POST['ef_notifications_nonce'] = 'invalid_nonce_value'; + $_POST['ef-save_followers'] = '1'; + $_POST['ef-selected-users'] = array( self::$editor_user_id ); - // Track if wp_die was called. $wp_die_called = false; add_filter( @@ -157,9 +262,60 @@ function () use ( &$wp_die_called ) { } ); - // Update the post which triggers the full save lifecycle including revision creation. - // This will fire transition_post_status for both the post AND the revision. - // The revision has a different ID, so without the revision skip fix, nonce check fails. + $exception_thrown = false; + try { + $edit_flow->notifications->save_post_subscriptions( 'draft', 'draft', $post ); + } catch ( \Exception $e ) { + $exception_thrown = true; + } + + $this->assertFalse( + $wp_die_called, + 'save_post_subscriptions should return early, not die, when nonce is invalid. ' . + 'Dying in a transition_post_status hook kills unrelated functionality.' + ); + $this->assertFalse( + $exception_thrown, + 'save_post_subscriptions should not throw when nonce is invalid.' + ); + } + + /** + * Test that save_post_subscriptions skips revisions. + * + * Revisions have different post IDs which would cause nonce verification to fail + * if not skipped. This test ensures revisions are properly handled. + * + * @covers EF_Notifications::save_post_subscriptions + */ + public function test_save_post_subscriptions_skips_revisions() { + global $edit_flow; + + $post_id = self::factory()->post->create( + array( + 'post_author' => self::$editor_user_id, + 'post_status' => 'draft', + ) + ); + + // Simulate Edit Flow form submission. + $_POST['ef_notifications_nonce'] = wp_create_nonce( 'save_user_usergroups' ); + $_POST['ef-save_followers'] = '1'; + $_POST['ef-selected-users'] = array( self::$editor_user_id ); + + $wp_die_called = false; + + add_filter( + 'wp_die_handler', + function () use ( &$wp_die_called ) { + return function ( $message ) use ( &$wp_die_called ) { + $wp_die_called = true; + throw new \Exception( 'wp_die called: ' . $message ); + }; + } + ); + + // Update the post which triggers revision creation and transition_post_status hooks. $exception_thrown = false; try { wp_update_post( @@ -174,37 +330,47 @@ function () use ( &$wp_die_called ) { $this->assertFalse( $wp_die_called, - 'wp_update_post should not trigger wp_die - save_post_subscriptions must skip revisions to avoid nonce mismatch' + 'save_post_subscriptions should skip revisions to avoid nonce mismatch issues.' ); $this->assertFalse( $exception_thrown, - 'wp_update_post threw an exception due to wp_die being called during revision save' + 'wp_update_post should not throw due to revision handling.' ); } /** - * Test that the nonce verification uses the correct action string. + * Test that subscriptions are saved when valid Edit Flow nonce is provided. * - * WordPress Classic Editor uses 'update-post_{$post_id}' for the edit form nonce. - * This test verifies that a nonce created with this action is considered valid. + * @covers EF_Notifications::save_post_subscriptions */ - public function test_classic_editor_nonce_action_is_update_post() { - $post_id = self::factory()->post->create(); - - // Create a nonce using the Classic Editor action. - $nonce = wp_create_nonce( 'update-post_' . $post_id ); + public function test_save_post_subscriptions_saves_data_with_valid_nonce() { + global $edit_flow; - // Verify the nonce is valid for the correct action. - $this->assertNotFalse( - wp_verify_nonce( $nonce, 'update-post_' . $post_id ), - 'Nonce should be valid for update-post_{$post_id} action' + $post_id = self::factory()->post->create( + array( + 'post_author' => self::$editor_user_id, + 'post_status' => 'draft', + ) ); - // The buggy code uses 'editpost' which is NOT a valid WordPress nonce action. - // This should fail to verify. - $this->assertFalse( - wp_verify_nonce( $nonce, 'editpost' ), - 'Nonce created for update-post_{$post_id} should NOT verify against editpost action' + $post = get_post( $post_id ); + + // Simulate Edit Flow form submission with valid nonce. + $_POST['ef_notifications_nonce'] = wp_create_nonce( 'save_user_usergroups' ); + $_POST['ef-save_followers'] = '1'; + $_POST['ef-selected-users'] = array( self::$editor_user_id ); + + // Call the function. + $edit_flow->notifications->save_post_subscriptions( 'draft', 'draft', $post ); + + // Verify the subscription was saved. + // Note: get_following_users with 'id' returns IDs as integers. + $following_users = $edit_flow->notifications->get_following_users( $post_id, 'id' ); + + $this->assertContains( + self::$editor_user_id, + $following_users, + 'User should be saved as a follower when valid Edit Flow nonce is provided.' ); } }