From 2a4622f11450ab52d837498eaf7ecc8928412ce9 Mon Sep 17 00:00:00 2001 From: Katie Wilkerson Rethman Date: Mon, 1 Jun 2026 16:34:18 -0500 Subject: [PATCH 1/5] feat(emails): allow test-send for inactive emails (NPPD-1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A publisher couldn't test-send a Newspack-managed transactional email when the email was in draft/inactive status. The cause was an incidental conflation: `Emails::send_email()` AND'd a `'publish' === post_status` guard that was designed for the auto-send code path (event-triggered RAS verification, payment receipts, etc.) but also blocked the test-send REST handler that went through the same function. Git blame on the guard (c121e5c3ce, d2937017d1) confirms no test-send-specific design intent — the gate was shared incidentally. Recon also confirmed the generous-scope fix is safe: NO Newspack-managed email config registers its own per-email test-send guard. All eight reader-activation emails + the three reader-revenue emails + group-subscription-invite + card-expiry- warning route through `Emails::send_email()` with no custom gating. WC-managed emails have a separate test-send path entirely (via WC's settings UI) — unaffected by this change. REFACTOR — semantic split - Extracted `validate_send_prerequisites( $post_id, $to )`: shared post-id-path prereqs (Newspack_Newsletters active, recipient non-empty, post resolves, HTML payload saved via EMAIL_HTML_META). Returns the resolved config + config name, or a WP_Error identifying which prereq failed. Future shared guards (rate limiting, etc.) land here. - Extracted `dispatch_email( $email_config, $config_name, $to, $placeholders )`: shared wp_mail() dispatch — locale switching, header construction, payload substitution, logging. Both send_email() and send_test_email() route through it so the wp_mail surface stays identical between auto-send and test-send. - Refactored `send_email()`: string path keeps its inline checks (used by every production auto-trigger); post-id branch now routes through validate_send_prerequisites + adds its own status check on top + dispatches via dispatch_email. - Added `send_test_email( $post_id, $to )`: new public entry point. Calls validate_send_prerequisites, intentionally skips the status check, dispatches via dispatch_email. Returns true|WP_Error so callers can surface specific failure causes. - Refactored `api_send_test_email()`: calls send_test_email instead of send_email. Returns the WP_Error from send_test_email directly when prereqs fail — replaces the previous generic "Test email was not sent." message with specific error codes (newspack_emails_unsupported, newspack_emails_empty_recipient, newspack_emails_post_not_resolvable, newspack_test_email_dispatch_failed). RECIPIENT VALIDATION Added `is_email( $recipient )` validation at api_send_test_email() before entering the dispatch path. Returns 400 + newspack_invalid_test_recipient on failure. Same anti-pattern as NPPD-1566's sanitize_email-vs-is_email validation gap: the args' sanitize_text_field callback strips tags but doesn't validate email format, so a typo like 'not-an-email' would silently reach wp_mail() and fail with no clue why. Cheap to add while we're here. OBSERVABLE BEHAVIOR - Auto-send (every production caller): unchanged. send_email() with a string config name still requires post_status='publish'. - Test-send (admin Send button on the email-edit screen): now works for inactive emails. Returns specific WP_Error codes on prereq failures. - Existing 4 emails tests still pass — the test_emails_status test that asserts auto-send blocks drafts is unchanged in behavior (it goes through send_email's post-id branch which retains the status check via the helper-then-status flow). DEVIATION NOTE User offered a fallback to the bool-parameter approach if the helper-extraction proved harder than expected. The extraction went cleanly because the post-id branch of send_email is exclusively used by the test-send path (only one production non-test caller passes a post_id — and that's api_send_test_email itself). Helper approach shipped as planned. Tests for the new behavior land in commit 2. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../includes/emails/class-emails.php | 184 ++++++++++++++++-- 1 file changed, 166 insertions(+), 18 deletions(-) diff --git a/plugins/newspack-plugin/includes/emails/class-emails.php b/plugins/newspack-plugin/includes/emails/class-emails.php index 217a29bf50..c76440e8fd 100644 --- a/plugins/newspack-plugin/includes/emails/class-emails.php +++ b/plugins/newspack-plugin/includes/emails/class-emails.php @@ -251,9 +251,23 @@ public static function get_email_payload( $config_name, $placeholders = [] ) { /** * Send an HTML email. * - * @param string $config_name Email config name. - * @param string $to Recipient's email addesss. - * @param array $placeholders Dynamic content substitutions. + * AUTO-SEND path — triggered by site events (registration, payment, + * subscription renewal, etc.). Requires the email's underlying + * post to be in `publish` status: an inactive/draft email must + * not fire on triggered events. For the deliberate admin + * preview operation, see {@see self::send_test_email()} — + * NPPD-1547 split the two entry points because the shared + * `'publish' === status` guard incidentally blocked test-send + * for inactive emails, conflating auto-trigger gating with + * admin-deliberate-action semantics. + * + * @param string|int $config_name Email config name (string), or + * Email post ID (int) for the + * post-id path shared with + * send_test_email. + * @param string $to Recipient's email address. + * @param array $placeholders Dynamic content substitutions. + * @return bool wp_mail() result, or false if any prerequisite failed. */ public static function send_email( $config_name, $to, $placeholders = [] ) { if ( ! self::supports_emails() ) { @@ -289,19 +303,141 @@ public static function send_email( $config_name, $to, $placeholders = [] ) { } } - $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); - if ( 'string' === gettype( $config_name ) ) { + // String path: looked up by type name. Used by every + // production auto-trigger (RAS verification, magic + // links, payment receipts, etc.). No HTML-payload check + // — the string path uses template files, not stored + // post content. $email_config = self::get_email_config_by_type( $config_name ); + if ( ! $to || ! $email_config || 'publish' !== $email_config['status'] ) { + return false; + } } elseif ( 'integer' === gettype( $config_name ) ) { - $email_config = self::serialize_email( null, $config_name ); - $config_name = \get_post_meta( $config_name, self::EMAIL_CONFIG_NAME_META, true ); + // Post-id path: shared with send_test_email() via + // validate_send_prerequisites(). Auto-send additionally + // requires 'publish' status; test-send does not. + $resolved = self::validate_send_prerequisites( $config_name, $to ); + if ( is_wp_error( $resolved ) ) { + return false; + } + if ( 'publish' !== $resolved['config']['status'] ) { + return false; + } + $email_config = $resolved['config']; + $config_name = $resolved['name']; } else { return false; } - if ( ! $to || ! $email_config || 'publish' !== $email_config['status'] ) { - return false; + + return self::dispatch_email( $email_config, $config_name, $to, $placeholders ); + } + + /** + * Send a test email for the given post. + * + * TEST-SEND path — the deliberate admin operation of previewing + * an email's actual rendered output. Distinct from the auto-send + * path ({@see self::send_email()}) which fires on triggered + * events and requires the email to be in `publish` status. + * + * NPPD-1547: before this split, test-send went through send_email() + * and inherited its `'publish' === status` guard, blocking + * test-send for inactive emails. That conflation was incidental + * — git blame shows the guard was introduced for the auto-send + * path with no test-send-specific design intent. Splitting the + * entry points makes the distinction explicit and means the + * test-send code path skips ONLY the status check, retaining + * every other prerequisite (Newspack_Newsletters present, + * recipient non-empty, post resolves, HTML payload saved). + * + * @param int $post_id Email post id. + * @param string $to Recipient email address. + * @return true|WP_Error True on successful dispatch; WP_Error + * with appropriate status code on failure. + */ + public static function send_test_email( $post_id, $to ) { + $resolved = self::validate_send_prerequisites( $post_id, $to ); + if ( is_wp_error( $resolved ) ) { + return $resolved; + } + $sent = self::dispatch_email( $resolved['config'], $resolved['name'], $to, [] ); + if ( ! $sent ) { + return new \WP_Error( + 'newspack_test_email_dispatch_failed', + esc_html__( 'Test email could not be dispatched. Check your mail configuration.', 'newspack-plugin' ), + [ 'status' => 500 ] + ); + } + return true; + } + + /** + * Validate the shared prerequisites for sending an email via the + * post-id path. Used by both send_email()'s post-id branch and + * send_test_email() so future shared guards (rate limiting, + * recipient format validation, etc.) need to land in one place. + * + * DOES NOT check post status — the caller layers its own status + * check on top when appropriate. send_email() requires + * `'publish'`; send_test_email() does not. + * + * @param int $post_id Email post id. + * @param string $to Recipient email address. + * @return array|WP_Error On success, [ 'config' => array, + * 'name' => string ]. On failure, WP_Error + * identifying which prerequisite failed. + */ + private static function validate_send_prerequisites( $post_id, $to ) { + if ( ! self::supports_emails() ) { + return new \WP_Error( + 'newspack_emails_unsupported', + esc_html__( 'Email sending requires the Newspack Newsletters plugin to be active.', 'newspack-plugin' ), + [ 'status' => 500 ] + ); + } + if ( empty( $to ) ) { + return new \WP_Error( + 'newspack_emails_empty_recipient', + esc_html__( 'A recipient is required.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); + } + // `serialize_email()` returns false when the post doesn't + // exist OR when its EMAIL_HTML_META is missing/empty — i.e. + // the email editor has never saved content for this post. + // Both are legitimate "can't dispatch this" cases. + $email_config = self::serialize_email( null, $post_id ); + if ( ! $email_config ) { + return new \WP_Error( + 'newspack_emails_post_not_resolvable', + esc_html__( 'Email cannot be resolved from the provided post ID, or has no saved content.', 'newspack-plugin' ), + [ 'status' => 404 ] + ); } + return [ + 'config' => $email_config, + 'name' => (string) \get_post_meta( $post_id, self::EMAIL_CONFIG_NAME_META, true ), + ]; + } + + /** + * Dispatch an email via wp_mail() — locale-switching, header + * construction, payload substitution, logging. Shared between + * send_email() and send_test_email() so the wp_mail() call + * surface is identical between auto-send and test-send. + * + * Callers are responsible for prerequisite validation before + * reaching here. + * + * @param array $email_config Serialized email config. + * @param string $config_name Resolved config name (logger context). + * @param string $to Recipient. + * @param array $placeholders Dynamic content substitutions. + * @return bool wp_mail() result. + */ + private static function dispatch_email( $email_config, $config_name, $to, $placeholders ) { + $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); $email_content_type = function() { return 'text/html'; @@ -581,18 +717,30 @@ public static function get_emails( $config_names = [], $get_full_configs = true * @param WP_REST_Request $request Request. */ public static function api_send_test_email( $request ) { - $was_sent = self::send_email( - $request->get_param( 'post_id' ), - $request->get_param( 'recipient' ) - ); - if ( $was_sent ) { - return \rest_ensure_response( [] ); - } else { + $recipient = $request->get_param( 'recipient' ); + $post_id = $request->get_param( 'post_id' ); + + // Validate recipient format BEFORE entering the dispatch + // path. `sanitize_text_field` (the args' sanitize_callback) + // strips tags + trims whitespace but doesn't validate email + // format. Without `is_email()` here, a typo like + // 'not-an-email' reaches `wp_mail()` and silently fails — the + // publisher sees the generic dispatch error with no clue + // why. NPPD-1547 adds this guard alongside the test-send + // split so the failure mode is specific. + if ( ! is_email( $recipient ) ) { return new \WP_Error( - 'newspack_test_email_not_sent', - __( 'Test email was not sent.', 'newspack-plugin' ) + 'newspack_invalid_test_recipient', + esc_html__( 'Recipient must be a valid email address.', 'newspack-plugin' ), + [ 'status' => 400 ] ); } + + $result = self::send_test_email( $post_id, $recipient ); + if ( is_wp_error( $result ) ) { + return $result; + } + return \rest_ensure_response( [] ); } /** From 5562cff4dd743448e000949fd369c5ae3cd3baff Mon Sep 17 00:00:00 2001 From: Katie Wilkerson Rethman Date: Mon, 1 Jun 2026 16:45:51 -0500 Subject: [PATCH 2/5] test(emails): coverage for test-send-for-inactive contract (NPPD-1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrites `test_emails_status` to lock the auto-send-vs-test-send distinction explicitly (both paths asserted in one test). Adds 8 new tests covering the test-send entry point, recipient validation, prerequisite failures, permission gating, and a regression lock against accidentally relaxing the auto-send status gate during future refactors. NEW TESTS 1. test_test_send_works_for_draft — headline contract. Sets the email post to draft, calls send_test_email, asserts true return. Verifies the mailer received the recipient (guards against short-circuit before dispatch_email runs). 2. test_test_send_rejects_non_email_recipient — typo recipient ('not-an-email') routed through api_send_test_email returns 400 + newspack_invalid_test_recipient, NOT silently reaching wp_mail and failing with the generic dispatch error. Locks the is_email() validation added in commit 1. 3. test_test_send_blocked_when_no_newspack_newsletters — SKIPPED with explicit reason. supports_emails() uses class_exists(), which can't be made false in the test bootstrap without runkit/uopz or a production-code change (adding a filter to supports_emails) — coverage gap documented rather than silently omitted. 4. test_test_send_blocked_when_missing_html_payload — fresh throwaway post (not the shared test-email-config — avoids leaking the missing-meta state into later tests in the same class, since this file deliberately doesn't call parent::set_up). Asserts WP_Error newspack_emails_post_not_resolvable with 404 status. 5. test_test_send_blocked_when_recipient_empty — empty recipient passed directly to send_test_email surfaces newspack_emails_empty_recipient with 400. Tests the helper's guard at a level below the api handler's is_email() check. 6. test_test_send_blocked_when_non_admin — subscriber user hits api_permissions_check, asserts 403 + newspack_rest_forbidden. Uses wp_insert_user directly (not $this->factory) because this class deliberately doesn't call parent::set_up() — the factory dependency would force the parent call, which breaks the existing test_emails_send_by_id test (empirically verified). 7. test_test_send_blocked_when_post_missing — post_id=9999999 surfaces newspack_emails_post_not_resolvable with 404. Same error code as missing-HTML-payload but distinct test name communicates the scenario. 8. test_auto_send_still_blocked_for_draft — REGRESSION LOCK. Sets the post to draft, calls send_email() via BOTH the string-name branch AND the post-id branch, asserts both return false. Without this lock, a future refactor that accidentally routes the post-id branch through send_test_email's status-less path would silently start dispatching draft emails on triggered events — a much worse failure mode than the original bug (silent over-firing vs. silent under-firing). REWRITTEN test_emails_status The original test asserted only that can_send_email() and send_email() return false for draft. The rewrite keeps those assertions AND adds the inverse for the test-send path: send_test_email() returns true for the same draft post. Both contracts in one test makes the distinction explicit — a future refactor that re-conflates the two paths fails both assertions simultaneously, producing a clear "the split has been re-broken" signal rather than mysterious behavior changes. VERIFICATION - PHPCS exit 0 (full output verified) - Emails class tests in isolation: 12 / 34 / 1 skipped - Full PHP suite: 1278 / 3850 / 2 skipped — delta from this commit: +8 tests / +20 assertions / +1 skipped (the intentional one). No other test counts shifted; no WC shim interactions introduced. NOTES ON STATE ISOLATION This test class deliberately doesn't call parent::set_up() — empirically, adding parent::set_up breaks test_emails_send_by_id (the parent's transaction lifecycle interacts with the file's state-mutation expectations). Without the parent's transaction rollback, DB changes survive across tests in this file. The new tests avoid leaking state by: - test #4 (missing-html-payload): uses a fresh throwaway post + wp_delete_post in the same test - test #6 (non-admin): uses wp_insert_user + wp_delete_user - other new tests: read-only or use ephemeral post IDs Filed forward as a potential cleanup follow-up: investigate why parent::set_up breaks the existing test, and migrate to standard WP_UnitTestCase transaction isolation if feasible. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../tests/unit-tests/emails.php | 258 +++++++++++++++++- 1 file changed, 252 insertions(+), 6 deletions(-) diff --git a/plugins/newspack-plugin/tests/unit-tests/emails.php b/plugins/newspack-plugin/tests/unit-tests/emails.php index fcc758dc28..84e660e11b 100644 --- a/plugins/newspack-plugin/tests/unit-tests/emails.php +++ b/plugins/newspack-plugin/tests/unit-tests/emails.php @@ -14,6 +14,13 @@ class Newspack_Test_Emails extends WP_UnitTestCase { /** * Setup. + * + * Intentionally does NOT call `parent::set_up()` — the existing + * tests in this class depend on the absence of the parent's + * transaction lifecycle (tested empirically: adding parent::set_up + * breaks `test_emails_send_by_id`). Tests that need a user + * factory use the static accessor on `WP_UnitTestCase` rather + * than `$this->factory`. */ public function set_up() { reset_phpmailer_instance(); @@ -147,7 +154,23 @@ public function test_emails_send_by_id() { } /** - * Email post status handling. + * Email post status — auto-send vs. test-send distinction (NPPD-1547). + * + * Locks the contract that splits the two entry points: + * + * - AUTO-SEND (Emails::send_email + Emails::can_send_email) is + * blocked for draft emails. Event-triggered sends (RAS, receipts, + * renewal reminders, etc.) require the email to be in 'publish' + * status — an inactive email must not fire on triggered events. + * - TEST-SEND (Emails::send_test_email) IS allowed for draft + * emails. The admin's deliberate "Send test" operation is + * semantically distinct from auto-trigger gating; before + * NPPD-1547 the shared 'publish' === status guard incidentally + * blocked test-send for inactive emails. + * + * Both assertions in one test makes the distinction explicit and + * locked: a future refactor that re-conflates the two paths + * fails this test against both contracts simultaneously. */ public function test_emails_status() { $test_email = self::get_test_email( 'test-email-config' ); @@ -158,12 +181,235 @@ public function test_emails_status() { ] ); - self::assertFalse( Emails::can_send_email( 'test-email-config' ), 'Email can\'t be sent – it\'s not published.' ); - $send_result = Emails::send_email( - 'test-email-config', - 'someone@example.com' + // AUTO-SEND: draft is blocked. Unchanged from pre-NPPD-1547. + self::assertFalse( + Emails::can_send_email( 'test-email-config' ), + 'can_send_email() must return false for draft (auto-send gate).' + ); + $auto_send_result = Emails::send_email( 'test-email-config', 'someone@example.com' ); + self::assertFalse( $auto_send_result, 'send_email() must not dispatch for draft (auto-send gate).' ); + + // TEST-SEND: draft is allowed. The headline contract of NPPD-1547. + $test_send_result = Emails::send_test_email( $test_email['post_id'], 'someone@example.com' ); + self::assertTrue( + $test_send_result, + 'send_test_email() must return true for draft — the admin "Send test" operation is decoupled from the auto-send status gate.' + ); + } + + /* + * ------------------------------------------------------------------ + * NPPD-1547 — Test-send entry point + recipient validation + * ------------------------------------------------------------------ + * The split between auto-send and test-send is verified at the + * status-gate level by `test_emails_status` above. The tests + * below cover the rest of the test-send contract: prerequisite + * failures surface specific WP_Error codes (not the previous + * generic "Test email was not sent." string), and the + * api_send_test_email handler validates recipient format before + * the dispatch path. + */ + + /** + * Headline contract: test-send dispatches for a draft email. + * Mirrors the second half of test_emails_status but isolates + * the assertion so failure pinpoints the test-send path. + */ + public function test_test_send_works_for_draft() { + $test_email = self::get_test_email( 'test-email-config' ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'draft', + ] + ); + + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'send_test_email() must return true (not WP_Error) for a draft email with all other prereqs satisfied.' ); + + // Verify the email actually reached the (mocked) mailer with + // the correct recipient — guards against accidentally + // short-circuiting before dispatch_email is called. + $mailer = tests_retrieve_phpmailer_instance(); + self::assertContains( 'tester@example.com', $mailer->get_sent()->to[0] ); + } + + /** + * Recipient format validation lives at the api_send_test_email + * handler (not at send_test_email itself), so the test routes + * through the handler with a WP_REST_Request. Typo input like + * 'not-an-email' must return 400 + newspack_invalid_test_recipient, + * NOT silently reach wp_mail() and fail with the generic dispatch + * error. + */ + public function test_test_send_rejects_non_email_recipient() { + $test_email = self::get_test_email( 'test-email-config' ); + + $request = new WP_REST_Request( 'POST' ); + $request->set_param( 'post_id', $test_email['post_id'] ); + $request->set_param( 'recipient', 'not-an-email' ); + + $response = Emails::api_send_test_email( $request ); + + self::assertInstanceOf( WP_Error::class, $response ); + self::assertSame( 'newspack_invalid_test_recipient', $response->get_error_code() ); + self::assertSame( 400, $response->get_error_data()['status'] ); + } + + /** + * Supports_emails() returning false means Newspack_Newsletters is + * not active. send_test_email must return WP_Error with + * newspack_emails_unsupported. + * + * STRUCTURAL LIMITATION: supports_emails() uses class_exists() on + * Newspack_Newsletters; in the test bootstrap that class IS + * loaded, and PHP doesn't allow un-loading a class within a + * process. The false branch is not exercisable without either + * (a) adding a filter to supports_emails() for test seam (a + * production change with no callers outside tests), or (b) + * runkit/uopz extensions (not standard test infra here). + * + * Skipped with explicit acknowledgement of the gap. The branch + * is a one-line class_exists check with no other logic — value + * of testing the false case is low relative to the production- + * code-change cost of opening it up for mocking. Filed forward + * as a potential testability follow-up. + */ + public function test_test_send_blocked_when_no_newspack_newsletters() { + $this->markTestSkipped( 'supports_emails() false branch is not exercisable without modifying production code (no filter on supports_emails) or runkit/uopz extensions.' ); + } + + /** + * A post WITHOUT EMAIL_HTML_META cannot be dispatched. + * serialize_email() guards this case by returning false when + * the meta is missing/empty; validate_send_prerequisites surfaces + * it as newspack_emails_post_not_resolvable. + * + * Uses its own throwaway post rather than the shared + * `test-email-config` so the missing-meta state doesn't leak + * into later tests in the file (this class deliberately doesn't + * call parent::set_up(), so WP_UnitTestCase's transaction + * rollback isn't in effect — DB changes survive across tests). + */ + public function test_test_send_blocked_when_missing_html_payload() { + // Create a fresh email post with NO EMAIL_HTML_META — that's + // the state under test. + $post_id = wp_insert_post( + [ + 'post_type' => Emails::POST_TYPE, + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 missing-html test', + 'meta_input' => [ + Emails::EMAIL_CONFIG_NAME_META => 'test-email-config', + // Intentionally no EMAIL_HTML_META. + ], + ] + ); + + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + // Clean up before any assertion can fail and short-circuit + // the function — guarantees no leak even on assertion failure. + wp_delete_post( $post_id, true ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_post_not_resolvable', $result->get_error_code() ); + self::assertSame( 404, $result->get_error_data()['status'] ); + } + + /** + * Empty recipient is blocked at the helper level (independent of + * the api handler's is_email validation). Tests the + * send_test_email entry point directly so the assertion isolates + * the helper's behavior. + */ + public function test_test_send_blocked_when_recipient_empty() { + $test_email = self::get_test_email( 'test-email-config' ); + + $result = Emails::send_test_email( $test_email['post_id'], '' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_empty_recipient', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } + + /** + * Non-admin users cannot call the test-send endpoint. The + * permission check is on the REST route's permission_callback + * (api_permissions_check), which requires manage_options. Test + * the callback directly with a subscriber-level user logged in + * — calling api_send_test_email via PHP doesn't pass through + * the route's permission_callback, but the callback itself is + * what the REST framework runs. + */ + public function test_test_send_blocked_when_non_admin() { + // Don't use `$this->factory` — this class doesn't call + // parent::set_up(), so factory isn't initialized. Direct + // wp_insert_user avoids the dependency. + $subscriber_id = wp_insert_user( + [ + 'user_login' => 'nppd1547_subscriber_' . uniqid(), + 'user_pass' => 'test-password', + 'user_email' => 'sub-' . uniqid() . '@example.test', + 'role' => 'subscriber', + ] + ); + $prev_user = get_current_user_id(); + wp_set_current_user( $subscriber_id ); + + $result = Emails::api_permissions_check( null ); + + wp_set_current_user( $prev_user ); + wp_delete_user( $subscriber_id ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_rest_forbidden', $result->get_error_code() ); + self::assertSame( 403, $result->get_error_data()['status'] ); + } + + /** + * A post_id that doesn't exist surfaces as + * newspack_emails_post_not_resolvable (same error code as + * missing-HTML-payload, since both go through serialize_email's + * false return). The test name distinguishes the SCENARIO even + * though the error code is shared. + */ + public function test_test_send_blocked_when_post_missing() { + $result = Emails::send_test_email( 9999999, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_post_not_resolvable', $result->get_error_code() ); + self::assertSame( 404, $result->get_error_data()['status'] ); + } + + /** + * REGRESSION LOCK: NPPD-1547's refactor extracted the send-path + * helper but MUST preserve auto-send's status gate. Without this + * lock-in, a future refactor that accidentally routes + * send_email's post-id branch through send_test_email's + * status-less path would silently start dispatching draft + * emails on triggered events — a much worse failure mode than + * the original bug (silent over-firing vs. silent under-firing). + */ + public function test_auto_send_still_blocked_for_draft() { + $test_email = self::get_test_email( 'test-email-config' ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'draft', + ] ); - self::assertFalse( $send_result, 'Email has not been sent.' ); + // Post-id path of send_email — the branch that shares + // validate_send_prerequisites with send_test_email. Auto-send + // must still apply the status check on top. + $post_id_result = Emails::send_email( $test_email['post_id'], 'tester@example.com' ); + self::assertFalse( $post_id_result, 'send_email() post-id branch must remain status-gated.' ); + + // String path of send_email — the dominant auto-trigger + // surface. Status check is inline (not via the helper). + $string_result = Emails::send_email( 'test-email-config', 'tester@example.com' ); + self::assertFalse( $string_result, 'send_email() string-name branch must remain status-gated.' ); } } From 1218ca6cbaf01119e32344d272471b1b8f746859 Mon Sep 17 00:00:00 2001 From: Katie Wilkerson Rethman Date: Mon, 1 Jun 2026 19:44:10 -0500 Subject: [PATCH 3/5] fix(emails): address code review findings (NPPD-1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 13 of 15 findings from the code review on PR #187. Two deferred as follow-ups (#1 dispatch_email re-fetching via get_email_payload — pre-existing, touches auto-send hot path; #13 string-branch bypasses helper — larger refactor, helper is post-id-only by design). CORRECTNESS - **Empty EMAIL_CONFIG_NAME_META blank-body** (#2). Without a guard, send_test_email on a draft with HTML payload but no config-name meta dispatched a blank-bodied email via get_email_payload('') → get_email_config_by_type('') returning false → array-access on false. Added newspack_emails_config_name_missing (422) guard in validate_send_prerequisites. Reachable post-NPPD-1547 because the publish gate no longer blocks drafts with incomplete meta. - **Frontend swallows new error codes** (#3). The sendTestEmail handler in src/other-scripts/emails/index.js previously caught ALL errors as 'Test email was not sent.' — collapsing every specific WP_Error the PR added back to a single generic string. Now surfaces error.message when present, falling back to the generic only for unstructured errors (network/CORS failures). - **send_test_email public + no permission check** (#4). Defense- in-depth: added current_user_can( 'manage_options' ) at the entry point. Only safe before because the sole caller was behind the REST permission_callback; future internal callers (CLI commands, plugin hooks) would have bypassed. New test test_test_send_blocked_when_non_admin_via_direct_php locks the contract. - **is_email() validation moved into helper** (#5). Previously lived at api_send_test_email — direct PHP callers of send_test_email skipped the format check. Now in validate_send_prerequisites alongside the empty-recipient check; REST and direct entry surface identical error codes for identical inputs. BEHAVIORAL ALIGNMENT (preserve pre-PR semantics) - **RAS-ACC migration moved to a helper** (#6) called by both send_email AND send_test_email. Pre-PR test-send went through send_email and triggered the migration; the original NPPD-1547 refactor dropped that side effect for test-send. Now reinstated. - **switch_to_locale moved back around the FULL send operation** (#7), not just dispatch_email. Pre-PR the locale switch wrapped config resolution, so get_email_config_by_type's lazy-create path used the admin's locale for load_email_template's __() calls. The original NPPD-1547 refactor put the switch inside dispatch_email only, meaning first-time post creation persisted HTML in site default locale instead. Wrapped both send_email and send_test_email with try/finally for locale restore. - **Trash exclusion** (#8) in send_test_email. The publish-gate skip was for the "draft / inactive" case; trashed posts are intentionally removed and shouldn't be sendable. New code newspack_emails_post_trashed (409) + test test_test_send_blocked_for_trashed_post. TEST INFRASTRUCTURE - **Test state restoration** (#9). Every test that mutates the shared test-email-config post now uses try/finally to restore the original status. Previously these tests left the post in draft, silently failing any future test that depends on 'publish' status. - Added login_as_admin() / logout_admin() helpers — every test that calls send_test_email now logs in as admin (required by the new entry-point cap check) and restores the previously- current user in a finally block. ERROR CODE SPLITS + NAMING - **Split conflated error codes** (#10). newspack_emails_post_not_resolvable (404 for all three of post-id-shape, post-missing, HTML-missing) is now three distinct codes: - newspack_emails_invalid_post_id (400) — post_id is 0, negative, or non-numeric (the absint(missing-param) case) - newspack_emails_post_missing (404) — post_id is valid but no post exists - newspack_emails_html_payload_missing (422) — post exists but EMAIL_HTML_META is missing/empty This lets the UI route by failure mode ("save your draft first" vs "the post is gone" vs "fix the form"). - **Validation order reordered** (#11). Cheapest/most-common failure modes first: post_id shape → recipient empty → recipient format → infrastructure prereq → post lookup → HTML payload → config name. - **Eliminated duplicate empty-recipient codes** (#12). The old api handler's newspack_invalid_test_recipient + helper's newspack_emails_empty_recipient surfaced two codes for the same state depending on entry point. Now a single code path through the helper: empty → newspack_emails_empty_recipient (400); invalid format → newspack_emails_invalid_recipient (400). - **Naming consistency** (#14). All emails-related error codes now use the newspack_emails_* prefix uniformly. Consumers can group/filter on the namespace without knowing both old conventions. Renamed: - newspack_invalid_test_recipient → newspack_emails_invalid_recipient - newspack_test_email_dispatch_failed → newspack_emails_test_dispatch_failed - **supports_emails() returns 412 instead of 500**. Configuration prereq not satisfied is not a server fault — 412 (Precondition Failed) more accurately reflects the state and reduces noise in uptime monitoring. LOGGER FIDELITY - **Logger::log fires conditionally** on dispatch outcome (#15). Pre-fix: "Sending X to Y" fired unconditionally after wp_mail regardless of return value, producing misleading success-shaped log lines for actual failures. Post-fix: separate "Sent" / "Failed to send" lines that match the WP_Error contract callers observe. TEST DELTA PHP suite: 1278 → 1283 tests / 3850 → 3865 assertions / 2 → 2 skipped. +5 tests / +15 assertions / 0 skipped change. No regressions in non-emails tests. JS suite: 185/185 unchanged (no JS test coverage for the sidebar handler exists; the frontend update is in plumbing code that has no test file). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../includes/emails/class-emails.php | 331 ++++++++---- .../src/other-scripts/emails/index.js | 12 +- .../tests/unit-tests/emails.php | 469 +++++++++++++----- 3 files changed, 583 insertions(+), 229 deletions(-) diff --git a/plugins/newspack-plugin/includes/emails/class-emails.php b/plugins/newspack-plugin/includes/emails/class-emails.php index c76440e8fd..3462b3ac58 100644 --- a/plugins/newspack-plugin/includes/emails/class-emails.php +++ b/plugins/newspack-plugin/includes/emails/class-emails.php @@ -274,63 +274,93 @@ public static function send_email( $config_name, $to, $placeholders = [] ) { return false; } - // Migrate to RAS-ACC email templates if migration option is not set AND there have been no manual updates to the templates. - if ( get_option( 'newspack_email_templates_migrated', '' ) !== 'v1' ) { - $migrated = true; - $templates = get_posts( - [ - 'post_type' => self::POST_TYPE, - 'posts_per_page' => -1, - 'post_status' => 'publish', - ] - ); - - foreach ( $templates as $template ) { - $publish_date = get_the_date( 'Y-m-d H:i:s', $template->ID ); - $last_modified_date = get_the_modified_date( 'Y-m-d H:i:s', $template->ID ); - - // Template has not been modified, so trash the post so we can trigger a template update. - if ( $publish_date === $last_modified_date ) { - if ( ! wp_trash_post( $template->ID ) ) { - // Flag the migration as failed so we can trigger another attempt later. - $migrated = false; - } + self::maybe_run_ras_acc_template_migration(); + + // Switch locale around the FULL send operation, not just + // dispatch. get_email_config_by_type's lazy-create path + // requires a template PHP file with __() calls; the locale + // active at that moment determines what gets persisted to + // the email post. Restoring symmetry via try/finally + // guarantees the locale is restored even if a wp_mail filter + // throws. + $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); + try { + if ( 'string' === gettype( $config_name ) ) { + // String path: looked up by type name. Used by every + // production auto-trigger (RAS verification, magic + // links, payment receipts, etc.). No HTML-payload check + // — the string path uses template files, not stored + // post content. + $email_config = self::get_email_config_by_type( $config_name ); + if ( ! $to || ! $email_config || 'publish' !== $email_config['status'] ) { + return false; + } + } elseif ( 'integer' === gettype( $config_name ) ) { + // Post-id path: shared with send_test_email() via + // validate_send_prerequisites(). Auto-send additionally + // requires 'publish' status; test-send does not. + $resolved = self::validate_send_prerequisites( $config_name, $to ); + if ( is_wp_error( $resolved ) ) { + return false; } + if ( 'publish' !== $resolved['config']['status'] ) { + return false; + } + $email_config = $resolved['config']; + $config_name = $resolved['name']; + } else { + return false; } - if ( $migrated ) { - update_option( 'newspack_email_templates_migrated', 'v1' ); + return self::dispatch_email( $email_config, $config_name, $to, $placeholders ); + } finally { + if ( $switched_locale ) { + \restore_previous_locale(); } } + } - if ( 'string' === gettype( $config_name ) ) { - // String path: looked up by type name. Used by every - // production auto-trigger (RAS verification, magic - // links, payment receipts, etc.). No HTML-payload check - // — the string path uses template files, not stored - // post content. - $email_config = self::get_email_config_by_type( $config_name ); - if ( ! $to || ! $email_config || 'publish' !== $email_config['status'] ) { - return false; - } - } elseif ( 'integer' === gettype( $config_name ) ) { - // Post-id path: shared with send_test_email() via - // validate_send_prerequisites(). Auto-send additionally - // requires 'publish' status; test-send does not. - $resolved = self::validate_send_prerequisites( $config_name, $to ); - if ( is_wp_error( $resolved ) ) { - return false; - } - if ( 'publish' !== $resolved['config']['status'] ) { - return false; + /** + * Run the v1 RAS-ACC email-templates migration once per request if + * it hasn't already completed. Extracted from `send_email()` so + * `send_test_email()` runs the same migration — both entry points + * are reachable from a fresh-install admin's first interaction + * with the email system, and the migration is harmless to run on + * any path (it's option-gated and idempotent). + * + * Trashing un-modified templates forces them to be regenerated + * from the current source on the next read, picking up RAS-ACC + * template updates without overwriting publisher edits. + */ + private static function maybe_run_ras_acc_template_migration() { + if ( 'v1' === get_option( 'newspack_email_templates_migrated', '' ) ) { + return; + } + $migrated = true; + $templates = get_posts( + [ + 'post_type' => self::POST_TYPE, + 'posts_per_page' => -1, + 'post_status' => 'publish', + ] + ); + + foreach ( $templates as $template ) { + $publish_date = get_the_date( 'Y-m-d H:i:s', $template->ID ); + $last_modified_date = get_the_modified_date( 'Y-m-d H:i:s', $template->ID ); + + // Template has not been modified, so trash the post so we can trigger a template update. + if ( $publish_date === $last_modified_date ) { + if ( ! wp_trash_post( $template->ID ) ) { + // Flag the migration as failed so we can trigger another attempt later. + $migrated = false; + } } - $email_config = $resolved['config']; - $config_name = $resolved['name']; - } else { - return false; } - return self::dispatch_email( $email_config, $config_name, $to, $placeholders ); + if ( $migrated ) { + update_option( 'newspack_email_templates_migrated', 'v1' ); + } } /** @@ -347,9 +377,17 @@ public static function send_email( $config_name, $to, $placeholders = [] ) { * — git blame shows the guard was introduced for the auto-send * path with no test-send-specific design intent. Splitting the * entry points makes the distinction explicit and means the - * test-send code path skips ONLY the status check, retaining - * every other prerequisite (Newspack_Newsletters present, - * recipient non-empty, post resolves, HTML payload saved). + * test-send code path skips ONLY the draft/publish gate, + * retaining every other prerequisite plus an explicit trash + * exclusion (a trashed post is intentionally removed, not + * "inactive"). + * + * Includes its own `current_user_can( 'manage_options' )` check + * even though the only current caller (api_send_test_email) is + * already behind a REST permission_callback. send_test_email is + * `public static` and the surface invites future internal + * callers; locking the cap check inside the entry point + * guarantees no path bypasses it. * * @param int $post_id Email post id. * @param string $to Recipient email address. @@ -357,43 +395,88 @@ public static function send_email( $config_name, $to, $placeholders = [] ) { * with appropriate status code on failure. */ public static function send_test_email( $post_id, $to ) { - $resolved = self::validate_send_prerequisites( $post_id, $to ); - if ( is_wp_error( $resolved ) ) { - return $resolved; - } - $sent = self::dispatch_email( $resolved['config'], $resolved['name'], $to, [] ); - if ( ! $sent ) { + if ( ! current_user_can( 'manage_options' ) ) { return new \WP_Error( - 'newspack_test_email_dispatch_failed', - esc_html__( 'Test email could not be dispatched. Check your mail configuration.', 'newspack-plugin' ), - [ 'status' => 500 ] + 'newspack_emails_forbidden', + esc_html__( 'You cannot send test emails.', 'newspack-plugin' ), + [ 'status' => 403 ] ); } - return true; + + self::maybe_run_ras_acc_template_migration(); + + $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); + try { + $resolved = self::validate_send_prerequisites( $post_id, $to ); + if ( is_wp_error( $resolved ) ) { + return $resolved; + } + + // Trash exclusion. The status-gate skip is for draft/pending + // (the "inactive but still being edited" case); a trashed + // post is intentionally removed from publication and + // shouldn't be sendable even by an admin's deliberate + // action. Restoring from trash is a one-click admin + // operation if the publisher really wants to test-send + // a trashed post. + if ( 'trash' === ( $resolved['config']['status'] ?? '' ) ) { + return new \WP_Error( + 'newspack_emails_post_trashed', + esc_html__( 'Cannot test-send a trashed email. Restore the email first.', 'newspack-plugin' ), + [ 'status' => 409 ] + ); + } + + $sent = self::dispatch_email( $resolved['config'], $resolved['name'], $to, [] ); + if ( ! $sent ) { + return new \WP_Error( + 'newspack_emails_test_dispatch_failed', + esc_html__( 'Test email could not be dispatched. Check your mail configuration.', 'newspack-plugin' ), + [ 'status' => 500 ] + ); + } + return true; + } finally { + if ( $switched_locale ) { + \restore_previous_locale(); + } + } } /** * Validate the shared prerequisites for sending an email via the * post-id path. Used by both send_email()'s post-id branch and - * send_test_email() so future shared guards (rate limiting, - * recipient format validation, etc.) need to land in one place. + * send_test_email() so future shared guards (rate limiting, etc.) + * land in one place. + * + * Checks (in order, cheapest first): + * - $post_id is a positive integer + * - $to is non-empty AND a valid email per is_email() + * - Newspack Newsletters plugin is active + * - Post resolves to a config with HTML payload + * - Resolved config_name (EMAIL_CONFIG_NAME_META) is non-empty * * DOES NOT check post status — the caller layers its own status * check on top when appropriate. send_email() requires - * `'publish'`; send_test_email() does not. + * `'publish'`; send_test_email() excludes only `'trash'`. + * + * Error codes use the `newspack_emails_*` prefix consistently so + * downstream consumers can group/filter on the namespace. * * @param int $post_id Email post id. * @param string $to Recipient email address. * @return array|WP_Error On success, [ 'config' => array, * 'name' => string ]. On failure, WP_Error - * identifying which prerequisite failed. + * with `status` data field set to an + * appropriate HTTP code. */ private static function validate_send_prerequisites( $post_id, $to ) { - if ( ! self::supports_emails() ) { + // Caller-input checks first (cheap, common failure modes). + if ( empty( $post_id ) || ! is_numeric( $post_id ) || (int) $post_id <= 0 ) { return new \WP_Error( - 'newspack_emails_unsupported', - esc_html__( 'Email sending requires the Newspack Newsletters plugin to be active.', 'newspack-plugin' ), - [ 'status' => 500 ] + 'newspack_emails_invalid_post_id', + esc_html__( 'A valid email post ID is required.', 'newspack-plugin' ), + [ 'status' => 400 ] ); } if ( empty( $to ) ) { @@ -403,21 +486,61 @@ private static function validate_send_prerequisites( $post_id, $to ) { [ 'status' => 400 ] ); } - // `serialize_email()` returns false when the post doesn't - // exist OR when its EMAIL_HTML_META is missing/empty — i.e. - // the email editor has never saved content for this post. - // Both are legitimate "can't dispatch this" cases. + if ( ! is_email( $to ) ) { + return new \WP_Error( + 'newspack_emails_invalid_recipient', + esc_html__( 'Recipient must be a valid email address.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); + } + // Infrastructure prereq: Newspack Newsletters provides the + // email-editor post type + meta keys. 412 (Precondition + // Failed) more accurately reflects "configuration prereq + // not satisfied" than 500 (server fault) — monitoring that + // pages on 5xx no longer fires for known-config states. + if ( ! self::supports_emails() ) { + return new \WP_Error( + 'newspack_emails_unsupported', + esc_html__( 'Email sending requires the Newspack Newsletters plugin to be active.', 'newspack-plugin' ), + [ 'status' => 412 ] + ); + } + // `serialize_email( null, $post_id )` returns false when the + // post doesn't exist OR when its EMAIL_HTML_META is missing. + // Distinguish via get_post() so the error codes (and HTTP + // status) point the publisher at the actionable cause. + if ( ! get_post( $post_id ) ) { + return new \WP_Error( + 'newspack_emails_post_missing', + esc_html__( 'Email post does not exist.', 'newspack-plugin' ), + [ 'status' => 404 ] + ); + } $email_config = self::serialize_email( null, $post_id ); if ( ! $email_config ) { return new \WP_Error( - 'newspack_emails_post_not_resolvable', - esc_html__( 'Email cannot be resolved from the provided post ID, or has no saved content.', 'newspack-plugin' ), - [ 'status' => 404 ] + 'newspack_emails_html_payload_missing', + esc_html__( 'Email has no saved content. Open the email in the editor and save before sending.', 'newspack-plugin' ), + [ 'status' => 422 ] + ); + } + $config_name = (string) \get_post_meta( $post_id, self::EMAIL_CONFIG_NAME_META, true ); + // EMAIL_CONFIG_NAME_META missing/empty would propagate through + // dispatch_email → get_email_payload('') → get_email_config_by_type('') + // returning false, producing a blank-bodied email. Now reachable + // post-NPPD-1547 because send_test_email skips the publish gate + // (drafts are likelier to have incomplete meta). Fail fast + // instead of dispatching empty content. + if ( '' === $config_name ) { + return new \WP_Error( + 'newspack_emails_config_name_missing', + esc_html__( 'Email is not associated with a known config type. Cannot dispatch.', 'newspack-plugin' ), + [ 'status' => 422 ] ); } return [ 'config' => $email_config, - 'name' => (string) \get_post_meta( $post_id, self::EMAIL_CONFIG_NAME_META, true ), + 'name' => $config_name, ]; } @@ -437,7 +560,14 @@ private static function validate_send_prerequisites( $post_id, $to ) { * @return bool wp_mail() result. */ private static function dispatch_email( $email_config, $config_name, $to, $placeholders ) { - $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); + // Locale-switching is performed by the caller (send_email / + // send_test_email) around the FULL send operation, including + // config resolution — get_email_config_by_type's lazy-create + // path requires a template PHP file with __() calls, and the + // locale active at that moment determines what gets persisted + // to the email post. Moving the switch into dispatch_email + // would mean first-time-template-creation runs under site + // default locale instead of the admin's. $email_content_type = function() { return 'text/html'; @@ -459,12 +589,19 @@ private static function dispatch_email( $email_config, $config_name, $to, $place ); remove_filter( 'wp_mail_content_type', $email_content_type ); - if ( $switched_locale ) { - \restore_previous_locale(); + // Log dispatch outcome, not the attempt. The old + // unconditional "Sending..." log produced misleading + // success-shaped lines for wp_mail failures and routinely + // led incident investigation down the wrong path + // (concluding dispatch succeeded when wp_mail returned + // false). Distinguishing success/failure in the log + // matches the WP_Error contract callers see. + if ( $email_send_result ) { + Logger::log( 'Sent "' . $config_name . '" email to: ' . $to ); + } else { + Logger::log( 'Failed to send "' . $config_name . '" email to: ' . $to ); } - Logger::log( 'Sending "' . $config_name . '" email to: ' . $to ); - return $email_send_result; } @@ -717,26 +854,16 @@ public static function get_emails( $config_names = [], $get_full_configs = true * @param WP_REST_Request $request Request. */ public static function api_send_test_email( $request ) { - $recipient = $request->get_param( 'recipient' ); - $post_id = $request->get_param( 'post_id' ); - - // Validate recipient format BEFORE entering the dispatch - // path. `sanitize_text_field` (the args' sanitize_callback) - // strips tags + trims whitespace but doesn't validate email - // format. Without `is_email()` here, a typo like - // 'not-an-email' reaches `wp_mail()` and silently fails — the - // publisher sees the generic dispatch error with no clue - // why. NPPD-1547 adds this guard alongside the test-send - // split so the failure mode is specific. - if ( ! is_email( $recipient ) ) { - return new \WP_Error( - 'newspack_invalid_test_recipient', - esc_html__( 'Recipient must be a valid email address.', 'newspack-plugin' ), - [ 'status' => 400 ] - ); - } - - $result = self::send_test_email( $post_id, $recipient ); + // All validation (post_id, recipient empty/format, supports + // emails, post resolves, HTML payload present, config name + // present) lives in send_test_email → + // validate_send_prerequisites. The handler is a thin + // pass-through so the REST and direct-PHP entry points + // produce identical error codes for identical inputs. + $result = self::send_test_email( + $request->get_param( 'post_id' ), + $request->get_param( 'recipient' ) + ); if ( is_wp_error( $result ) ) { return $result; } diff --git a/plugins/newspack-plugin/src/other-scripts/emails/index.js b/plugins/newspack-plugin/src/other-scripts/emails/index.js index 9219133585..19c2131908 100644 --- a/plugins/newspack-plugin/src/other-scripts/emails/index.js +++ b/plugins/newspack-plugin/src/other-scripts/emails/index.js @@ -82,8 +82,16 @@ const ReaderRevenueEmailSidebar = compose( [ .then( () => { createNotice( 'success', __( 'Test email sent!', 'newspack-plugin' ) ); } ) - .catch( () => { - createNotice( 'error', __( 'Test email was not sent.', 'newspack-plugin' ) ); + .catch( error => { + // Surface the server's specific message when present + // (NPPD-1547 added structured error codes / messages + // for each prerequisite failure: invalid recipient, + // missing HTML payload, trashed post, etc.). The + // `__( 'Test email was not sent.' )` generic is the + // fallback for unstructured errors (network failures, + // CORS, etc.) — those don't have a `.message` field. + const message = ( error && error.message ) || __( 'Test email was not sent.', 'newspack-plugin' ); + createNotice( 'error', message ); } ) .finally( () => { setInFlight( false ); diff --git a/plugins/newspack-plugin/tests/unit-tests/emails.php b/plugins/newspack-plugin/tests/unit-tests/emails.php index 84e660e11b..c6691d2580 100644 --- a/plugins/newspack-plugin/tests/unit-tests/emails.php +++ b/plugins/newspack-plugin/tests/unit-tests/emails.php @@ -174,6 +174,7 @@ public function test_emails_send_by_id() { */ public function test_emails_status() { $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); wp_update_post( [ 'ID' => $test_email['post_id'], @@ -181,20 +182,39 @@ public function test_emails_status() { ] ); - // AUTO-SEND: draft is blocked. Unchanged from pre-NPPD-1547. - self::assertFalse( - Emails::can_send_email( 'test-email-config' ), - 'can_send_email() must return false for draft (auto-send gate).' - ); - $auto_send_result = Emails::send_email( 'test-email-config', 'someone@example.com' ); - self::assertFalse( $auto_send_result, 'send_email() must not dispatch for draft (auto-send gate).' ); - - // TEST-SEND: draft is allowed. The headline contract of NPPD-1547. - $test_send_result = Emails::send_test_email( $test_email['post_id'], 'someone@example.com' ); - self::assertTrue( - $test_send_result, - 'send_test_email() must return true for draft — the admin "Send test" operation is decoupled from the auto-send status gate.' - ); + try { + // AUTO-SEND: draft is blocked. Unchanged from pre-NPPD-1547. + self::assertFalse( + Emails::can_send_email( 'test-email-config' ), + 'can_send_email() must return false for draft (auto-send gate).' + ); + $auto_send_result = Emails::send_email( 'test-email-config', 'someone@example.com' ); + self::assertFalse( $auto_send_result, 'send_email() must not dispatch for draft (auto-send gate).' ); + + // TEST-SEND: draft is allowed. The headline contract of NPPD-1547. + // send_test_email requires manage_options at its entry point; + // log in as admin for the duration of the call. + $admin = self::login_as_admin(); + try { + $test_send_result = Emails::send_test_email( $test_email['post_id'], 'someone@example.com' ); + self::assertTrue( + $test_send_result, + 'send_test_email() must return true for draft — the admin "Send test" operation is decoupled from the auto-send status gate.' + ); + } finally { + self::logout_admin( $admin ); + } + } finally { + // Restore the shared post so later tests see 'publish' + // status (this class skips parent::set_up(), so DB + // mutations leak across tests without explicit restore). + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } } /* @@ -204,19 +224,19 @@ public function test_emails_status() { * The split between auto-send and test-send is verified at the * status-gate level by `test_emails_status` above. The tests * below cover the rest of the test-send contract: prerequisite - * failures surface specific WP_Error codes (not the previous - * generic "Test email was not sent." string), and the - * api_send_test_email handler validates recipient format before - * the dispatch path. + * failures surface specific newspack_emails_* WP_Error codes + * (not the previous generic "Test email was not sent." string), + * and capability + trash + recipient-format guards apply + * uniformly whether send_test_email is reached via the REST + * handler or via a direct PHP call. */ /** * Headline contract: test-send dispatches for a draft email. - * Mirrors the second half of test_emails_status but isolates - * the assertion so failure pinpoints the test-send path. */ public function test_test_send_works_for_draft() { - $test_email = self::get_test_email( 'test-email-config' ); + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); wp_update_post( [ 'ID' => $test_email['post_id'], @@ -224,129 +244,245 @@ public function test_test_send_works_for_draft() { ] ); - $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'send_test_email() must return true (not WP_Error) for a draft email with all other prereqs satisfied.' ); + + // Verify the email actually reached the (mocked) mailer + // with the correct recipient — guards against accidentally + // short-circuiting before dispatch_email is called. + $mailer = tests_retrieve_phpmailer_instance(); + self::assertContains( 'tester@example.com', $mailer->get_sent()->to[0] ); + } finally { + self::logout_admin( $admin ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } - self::assertTrue( $result, 'send_test_email() must return true (not WP_Error) for a draft email with all other prereqs satisfied.' ); + /** + * Recipient format validation now lives in + * validate_send_prerequisites (the helper), not at the REST + * handler. Both the api_send_test_email REST entry AND direct + * send_test_email PHP calls reject typo input with the same + * code (`newspack_emails_invalid_recipient`) and 400 status. + */ + public function test_test_send_rejects_non_email_recipient_via_rest() { + $test_email = self::get_test_email( 'test-email-config' ); - // Verify the email actually reached the (mocked) mailer with - // the correct recipient — guards against accidentally - // short-circuiting before dispatch_email is called. - $mailer = tests_retrieve_phpmailer_instance(); - self::assertContains( 'tester@example.com', $mailer->get_sent()->to[0] ); + $admin = self::login_as_admin(); + try { + $request = new WP_REST_Request( 'POST' ); + $request->set_param( 'post_id', $test_email['post_id'] ); + $request->set_param( 'recipient', 'not-an-email' ); + + $response = Emails::api_send_test_email( $request ); + + self::assertInstanceOf( WP_Error::class, $response ); + self::assertSame( 'newspack_emails_invalid_recipient', $response->get_error_code() ); + self::assertSame( 400, $response->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } } /** - * Recipient format validation lives at the api_send_test_email - * handler (not at send_test_email itself), so the test routes - * through the handler with a WP_REST_Request. Typo input like - * 'not-an-email' must return 400 + newspack_invalid_test_recipient, - * NOT silently reach wp_mail() and fail with the generic dispatch - * error. + * Direct PHP callers of send_test_email get the same recipient + * validation as REST callers — guards against the "helper does + * one thing, handler does another" gap that would let a future + * internal caller bypass is_email(). */ - public function test_test_send_rejects_non_email_recipient() { + public function test_test_send_rejects_non_email_recipient_via_direct_php() { $test_email = self::get_test_email( 'test-email-config' ); - $request = new WP_REST_Request( 'POST' ); - $request->set_param( 'post_id', $test_email['post_id'] ); - $request->set_param( 'recipient', 'not-an-email' ); - - $response = Emails::api_send_test_email( $request ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'not-an-email' ); - self::assertInstanceOf( WP_Error::class, $response ); - self::assertSame( 'newspack_invalid_test_recipient', $response->get_error_code() ); - self::assertSame( 400, $response->get_error_data()['status'] ); + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_invalid_recipient', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } } /** * Supports_emails() returning false means Newspack_Newsletters is - * not active. send_test_email must return WP_Error with - * newspack_emails_unsupported. + * not active. Acknowledged structural test-infra gap. * - * STRUCTURAL LIMITATION: supports_emails() uses class_exists() on - * Newspack_Newsletters; in the test bootstrap that class IS - * loaded, and PHP doesn't allow un-loading a class within a - * process. The false branch is not exercisable without either - * (a) adding a filter to supports_emails() for test seam (a - * production change with no callers outside tests), or (b) - * runkit/uopz extensions (not standard test infra here). - * - * Skipped with explicit acknowledgement of the gap. The branch - * is a one-line class_exists check with no other logic — value - * of testing the false case is low relative to the production- - * code-change cost of opening it up for mocking. Filed forward - * as a potential testability follow-up. + * The supports_emails() implementation uses class_exists() on Newspack_Newsletters; + * the test bootstrap loads that class and PHP doesn't allow + * un-loading. The false branch is not exercisable without either + * adding a filter to supports_emails() for test seam (a + * production change for testability) or runkit/uopz extensions. + * Accepting reduced coverage on this single guard rather than + * paying the production-code-change cost. Tracked separately as + * "make supports_emails() check mockable for testing" follow-up. */ public function test_test_send_blocked_when_no_newspack_newsletters() { - $this->markTestSkipped( 'supports_emails() false branch is not exercisable without modifying production code (no filter on supports_emails) or runkit/uopz extensions.' ); + $this->markTestSkipped( 'supports_emails() false branch is not exercisable without modifying production code or runkit/uopz extensions. Test infra investment we are choosing not to make in this PR; tracked as a follow-up.' ); } /** * A post WITHOUT EMAIL_HTML_META cannot be dispatched. - * serialize_email() guards this case by returning false when - * the meta is missing/empty; validate_send_prerequisites surfaces - * it as newspack_emails_post_not_resolvable. + * validate_send_prerequisites surfaces it as + * newspack_emails_html_payload_missing with 422 — distinct from + * "post doesn't exist" (now newspack_emails_post_missing/404) so + * the UI can prompt "save your draft first" vs. "the post is + * gone". * - * Uses its own throwaway post rather than the shared - * `test-email-config` so the missing-meta state doesn't leak - * into later tests in the file (this class deliberately doesn't - * call parent::set_up(), so WP_UnitTestCase's transaction - * rollback isn't in effect — DB changes survive across tests). + * Uses its own throwaway post (different config name) so the + * missing-meta state doesn't leak into other tests via the + * shared get_emails() lookup. */ public function test_test_send_blocked_when_missing_html_payload() { - // Create a fresh email post with NO EMAIL_HTML_META — that's - // the state under test. $post_id = wp_insert_post( [ 'post_type' => Emails::POST_TYPE, 'post_status' => 'draft', 'post_title' => 'NPPD-1547 missing-html test', 'meta_input' => [ - Emails::EMAIL_CONFIG_NAME_META => 'test-email-config', - // Intentionally no EMAIL_HTML_META. + Emails::EMAIL_CONFIG_NAME_META => 'nppd1547-missing-html', ], ] ); - $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_html_payload_missing', $result->get_error_code() ); + self::assertSame( 422, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } - // Clean up before any assertion can fail and short-circuit - // the function — guarantees no leak even on assertion failure. - wp_delete_post( $post_id, true ); + /** + * Post exists with HTML payload BUT no EMAIL_CONFIG_NAME_META. + * Surfaces as newspack_emails_config_name_missing with 422. + * Without this guard, dispatch_email would call + * get_email_payload('') which returns false → array-access on + * false → blank-bodied email sent to recipient. Now reachable + * because send_test_email skips the publish gate, so drafts + * with incomplete meta can route this far. + */ + public function test_test_send_blocked_when_config_name_missing() { + $post_id = wp_insert_post( + [ + 'post_type' => Emails::POST_TYPE, + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 missing-config-name test', + 'meta_input' => [ + // Intentionally no EMAIL_CONFIG_NAME_META. + \Newspack_Newsletters::EMAIL_HTML_META => 'nppd-1547 stub', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_config_name_missing', $result->get_error_code() ); + self::assertSame( 422, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * Trashed posts cannot be test-sent. The publish-status gate + * skip in NPPD-1547 is for the "draft / inactive but being + * edited" case; a trashed post is intentionally removed and + * shouldn't be sendable. Restoring from trash is a one-click + * admin operation if the publisher really wants to test the + * trashed content. + */ + public function test_test_send_blocked_for_trashed_post() { + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'trash', + ] + ); - self::assertInstanceOf( WP_Error::class, $result ); - self::assertSame( 'newspack_emails_post_not_resolvable', $result->get_error_code() ); - self::assertSame( 404, $result->get_error_data()['status'] ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_post_trashed', $result->get_error_code() ); + self::assertSame( 409, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } } /** - * Empty recipient is blocked at the helper level (independent of - * the api handler's is_email validation). Tests the - * send_test_email entry point directly so the assertion isolates - * the helper's behavior. + * Empty recipient surfaces newspack_emails_empty_recipient with + * 400 — same code regardless of REST vs direct entry (no more + * duplicate codes for the same state). */ public function test_test_send_blocked_when_recipient_empty() { $test_email = self::get_test_email( 'test-email-config' ); - $result = Emails::send_test_email( $test_email['post_id'], '' ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], '' ); - self::assertInstanceOf( WP_Error::class, $result ); - self::assertSame( 'newspack_emails_empty_recipient', $result->get_error_code() ); - self::assertSame( 400, $result->get_error_data()['status'] ); + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_empty_recipient', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } } /** - * Non-admin users cannot call the test-send endpoint. The - * permission check is on the REST route's permission_callback - * (api_permissions_check), which requires manage_options. Test - * the callback directly with a subscriber-level user logged in - * — calling api_send_test_email via PHP doesn't pass through - * the route's permission_callback, but the callback itself is - * what the REST framework runs. + * Invalid post_id (0, negative, non-numeric) surfaces + * newspack_emails_invalid_post_id with 400 — distinct from + * "post doesn't exist" (404). 0 is the common case via + * `absint(missing_param)`; this guard returns a structurally + * correct 400 rather than the previous mis-classified 404. */ - public function test_test_send_blocked_when_non_admin() { - // Don't use `$this->factory` — this class doesn't call - // parent::set_up(), so factory isn't initialized. Direct - // wp_insert_user avoids the dependency. + public function test_test_send_blocked_when_post_id_invalid() { + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( 0, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_invalid_post_id', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Subscriber-level user calling the REST route is blocked by + * api_permissions_check (the route's permission_callback). + */ + public function test_test_send_blocked_when_non_admin_via_rest() { $subscriber_id = wp_insert_user( [ 'user_login' => 'nppd1547_subscriber_' . uniqid(), @@ -358,29 +494,69 @@ public function test_test_send_blocked_when_non_admin() { $prev_user = get_current_user_id(); wp_set_current_user( $subscriber_id ); - $result = Emails::api_permissions_check( null ); + try { + $result = Emails::api_permissions_check( null ); - wp_set_current_user( $prev_user ); - wp_delete_user( $subscriber_id ); + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_rest_forbidden', $result->get_error_code() ); + self::assertSame( 403, $result->get_error_data()['status'] ); + } finally { + wp_set_current_user( $prev_user ); + wp_delete_user( $subscriber_id ); + } + } + + /** + * Subscriber-level user calling send_test_email DIRECTLY (not + * via REST) is blocked by the cap check at the entry point of + * send_test_email itself. Without the entry-point check, a + * future PHP caller (CLI command, plugin hook) could bypass + * the REST route's permission_callback and dispatch draft + * emails from a low-privilege context. This test locks the + * defense-in-depth contract. + */ + public function test_test_send_blocked_when_non_admin_via_direct_php() { + $test_email = self::get_test_email( 'test-email-config' ); + + $subscriber_id = wp_insert_user( + [ + 'user_login' => 'nppd1547_subscriber_direct_' . uniqid(), + 'user_pass' => 'test-password', + 'user_email' => 'sub-direct-' . uniqid() . '@example.test', + 'role' => 'subscriber', + ] + ); + $prev_user = get_current_user_id(); + wp_set_current_user( $subscriber_id ); - self::assertInstanceOf( WP_Error::class, $result ); - self::assertSame( 'newspack_rest_forbidden', $result->get_error_code() ); - self::assertSame( 403, $result->get_error_data()['status'] ); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_forbidden', $result->get_error_code() ); + self::assertSame( 403, $result->get_error_data()['status'] ); + } finally { + wp_set_current_user( $prev_user ); + wp_delete_user( $subscriber_id ); + } } /** - * A post_id that doesn't exist surfaces as - * newspack_emails_post_not_resolvable (same error code as - * missing-HTML-payload, since both go through serialize_email's - * false return). The test name distinguishes the SCENARIO even - * though the error code is shared. + * Non-existent post_id surfaces newspack_emails_post_missing + * with 404 — distinct from invalid-id-shape (400) and missing- + * content (422). The split lets the UI route by failure mode. */ public function test_test_send_blocked_when_post_missing() { - $result = Emails::send_test_email( 9999999, 'tester@example.com' ); - - self::assertInstanceOf( WP_Error::class, $result ); - self::assertSame( 'newspack_emails_post_not_resolvable', $result->get_error_code() ); - self::assertSame( 404, $result->get_error_data()['status'] ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( 9999999, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_post_missing', $result->get_error_code() ); + self::assertSame( 404, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } } /** @@ -389,11 +565,11 @@ public function test_test_send_blocked_when_post_missing() { * lock-in, a future refactor that accidentally routes * send_email's post-id branch through send_test_email's * status-less path would silently start dispatching draft - * emails on triggered events — a much worse failure mode than - * the original bug (silent over-firing vs. silent under-firing). + * emails on triggered events. */ public function test_auto_send_still_blocked_for_draft() { - $test_email = self::get_test_email( 'test-email-config' ); + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); wp_update_post( [ 'ID' => $test_email['post_id'], @@ -401,15 +577,58 @@ public function test_auto_send_still_blocked_for_draft() { ] ); - // Post-id path of send_email — the branch that shares - // validate_send_prerequisites with send_test_email. Auto-send - // must still apply the status check on top. - $post_id_result = Emails::send_email( $test_email['post_id'], 'tester@example.com' ); - self::assertFalse( $post_id_result, 'send_email() post-id branch must remain status-gated.' ); + try { + // Post-id path of send_email — the branch that shares + // validate_send_prerequisites with send_test_email. Auto-send + // must still apply the status check on top. + $post_id_result = Emails::send_email( $test_email['post_id'], 'tester@example.com' ); + self::assertFalse( $post_id_result, 'send_email() post-id branch must remain status-gated.' ); + + // String path of send_email — the dominant auto-trigger + // surface. Status check is inline (not via the helper). + $string_result = Emails::send_email( 'test-email-config', 'tester@example.com' ); + self::assertFalse( $string_result, 'send_email() string-name branch must remain status-gated.' ); + } finally { + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + + /** + * Login helper: create a fresh administrator user, set them + * as current, return state needed for cleanup. Tests that + * call send_test_email need to pair this with logout_admin() + * in a try/finally block. + * + * @return array{0: int, 1: int} [ admin user id, previously-current user id ] + */ + private static function login_as_admin() { + $admin_id = wp_insert_user( + [ + 'user_login' => 'nppd1547_admin_' . uniqid(), + 'user_pass' => 'test-password', + 'user_email' => 'admin-' . uniqid() . '@example.test', + 'role' => 'administrator', + ] + ); + $prev_user = get_current_user_id(); + wp_set_current_user( $admin_id ); + return [ $admin_id, $prev_user ]; + } - // String path of send_email — the dominant auto-trigger - // surface. Status check is inline (not via the helper). - $string_result = Emails::send_email( 'test-email-config', 'tester@example.com' ); - self::assertFalse( $string_result, 'send_email() string-name branch must remain status-gated.' ); + /** + * Restore the previously-current user and delete the admin + * created by login_as_admin(). + * + * @param array{0: int, 1: int} $session State returned by login_as_admin. + */ + private static function logout_admin( $session ) { + [ $admin_id, $prev_user ] = $session; + wp_set_current_user( $prev_user ); + wp_delete_user( $admin_id ); } } From ddc831c0c5e037340b7f71f9c04ceaf7cb56bbce Mon Sep 17 00:00:00 2001 From: Katie Wilkerson Rethman Date: Tue, 2 Jun 2026 10:46:58 -0500 Subject: [PATCH 4/5] fix(emails): address Copilot review on send-path validation (NPPD-1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from Copilot's review of PR #187, all confirmed and fixed. COPILOT #1 — dispatch_email's wp_mail filter not in try/finally `add_filter( 'wp_mail_content_type', ... )` was paired with a bare `remove_filter()` AFTER the wp_mail() call. If wp_mail() (or any filter it triggers) threw, the html-content-type filter would persist for every subsequent wp_mail() call in the same request — silently converting plain-text mails (password resets, WP core notifications, etc.) to html. Wrapped the wp_mail() invocation in try/finally so remove_filter() always runs. COPILOT #2 — is_numeric() too loose for post_id validation validate_send_prerequisites used is_numeric() which accepts: - floats: '1.5' (int)('1.5') = 1 - scientific: '1e3' (int)('1e3') = 1000 - exponents: '1.7e308' (int)((float)'1.7e308') = PHP_INT_MAX Any of these would slip through the validate gate and silently resolve the wrong post (or trigger PHP_INT_MAX-shaped overflow on the subsequent query). Tightened the integer-shape check: is_int( $post_id ) || ( is_string( $post_id ) && '' !== $post_id && ctype_digit( $post_id ) ) ctype_digit accepts only digit-only strings (no '.', no 'e', no sign), then the value is cast to int once and the rest of the function operates on a well-typed positive integer. COPILOT #3 — gettype()-based branching can't see numeric strings send_email's `'string' === gettype($config_name)` vs `'integer' === gettype($config_name)` branching silently routed digit-only strings to the string-name branch (which then failed because get_email_config_by_type looks up type names, not post IDs). Pre-existing — production callers all pass either string type names or absint'd integers — but a future caller passing a numeric string from $_POST, post meta, or an untyped integration would silently fail. Normalized digit-only strings to int at the top of send_email so the post-id branch is reached correctly. NEW TESTS - test_test_send_rejects_non_integer_shaped_post_id loops nine values that is_numeric accepts but ctype_digit rejects ('1.5', '1e3', '1.7e308', '-5', 'abc', '12abc', ' 5 ', '', null) and asserts each is rejected with newspack_emails_invalid_post_id. - test_send_test_email_accepts_numeric_string_post_id locks in the hardening for the digit-only-string-survives-and-routes- correctly contract. Test delta: +2 tests / +19 assertions. PHP suite 1283 → 1285 / 3865 → 3884 / 2 skipped (unchanged). PHPCS exit 0. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../includes/emails/class-emails.php | 46 +++++++++-- .../tests/unit-tests/emails.php | 80 +++++++++++++++++++ 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/plugins/newspack-plugin/includes/emails/class-emails.php b/plugins/newspack-plugin/includes/emails/class-emails.php index 3462b3ac58..ea1f1fd64c 100644 --- a/plugins/newspack-plugin/includes/emails/class-emails.php +++ b/plugins/newspack-plugin/includes/emails/class-emails.php @@ -274,6 +274,18 @@ public static function send_email( $config_name, $to, $placeholders = [] ) { return false; } + // Normalize numeric-string post IDs to int BEFORE the + // gettype()-based branching below. Without this, a caller + // passing a digit-only string (e.g. from $_POST, post meta, + // or a cast-naive integration) would be routed to the + // string-name branch and silently fail because + // get_email_config_by_type() looks up type names, not IDs. + // Only digit-only strings are normalized — sender-name-style + // strings stay strings. + if ( is_string( $config_name ) && '' !== $config_name && ctype_digit( $config_name ) ) { + $config_name = (int) $config_name; + } + self::maybe_run_ras_acc_template_migration(); // Switch locale around the FULL send operation, not just @@ -472,13 +484,22 @@ public static function send_test_email( $post_id, $to ) { */ private static function validate_send_prerequisites( $post_id, $to ) { // Caller-input checks first (cheap, common failure modes). - if ( empty( $post_id ) || ! is_numeric( $post_id ) || (int) $post_id <= 0 ) { + // is_numeric() is too loose — accepts '1.5', '1e3', '1.7e308' + // which then (int)-cast to 1, 1000, or PHP_INT_MAX silently, + // potentially resolving the wrong post. Require an actual + // integer-shaped value (int type OR a digit-only string), + // then normalize to int so downstream calls operate on a + // well-typed positive integer. + $is_int_shaped = is_int( $post_id ) + || ( is_string( $post_id ) && '' !== $post_id && ctype_digit( $post_id ) ); + if ( ! $is_int_shaped || (int) $post_id <= 0 ) { return new \WP_Error( 'newspack_emails_invalid_post_id', esc_html__( 'A valid email post ID is required.', 'newspack-plugin' ), [ 'status' => 400 ] ); } + $post_id = (int) $post_id; if ( empty( $to ) ) { return new \WP_Error( 'newspack_emails_empty_recipient', @@ -580,14 +601,23 @@ private static function dispatch_email( $email_config, $config_name, $to, $place $headers[] = sprintf( 'Reply-To: %s <%s>', $email_config['from_name'], $email_config['reply_to_email'] ); } + // try/finally so the wp_mail_content_type filter is removed + // even if wp_mail() (or a filter it triggers) throws — + // otherwise the html-content-type filter persists for every + // subsequent wp_mail() call in the same request, silently + // converting plain-text mails (password resets, WP core + // notifications) to html. add_filter( 'wp_mail_content_type', $email_content_type ); - $email_send_result = wp_mail( // phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.wp_mail_wp_mail - $to, - $email_config['subject'], - self::get_email_payload( $config_name, $placeholders ), - $headers - ); - remove_filter( 'wp_mail_content_type', $email_content_type ); + try { + $email_send_result = wp_mail( // phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.wp_mail_wp_mail + $to, + $email_config['subject'], + self::get_email_payload( $config_name, $placeholders ), + $headers + ); + } finally { + remove_filter( 'wp_mail_content_type', $email_content_type ); + } // Log dispatch outcome, not the attempt. The old // unconditional "Sending..." log produced misleading diff --git a/plugins/newspack-plugin/tests/unit-tests/emails.php b/plugins/newspack-plugin/tests/unit-tests/emails.php index c6691d2580..ddf9b8080b 100644 --- a/plugins/newspack-plugin/tests/unit-tests/emails.php +++ b/plugins/newspack-plugin/tests/unit-tests/emails.php @@ -478,6 +478,86 @@ public function test_test_send_blocked_when_post_id_invalid() { } } + /** + * Tight post_id validation: is_numeric() is too loose — it + * accepts floats like '1.5' and scientific notation like '1e3' + * which then truncate-cast to ints (1, 1000) and resolve to + * the wrong post. Require integer-shaped input. + * + * Each value must be rejected with newspack_emails_invalid_post_id. + */ + public function test_test_send_rejects_non_integer_shaped_post_id() { + $admin = self::login_as_admin(); + try { + // Values is_numeric() accepts but ctype_digit rejects. + // Each must surface newspack_emails_invalid_post_id (400). + $non_integer_shapes = [ + '1.5', + '1e3', + '1.7e308', + '-5', + 'abc', + '12abc', + ' 5 ', + '', + null, + ]; + + foreach ( $non_integer_shapes as $bad_post_id ) { + // Friendly label for assertion-failure output. null + // and '' don't survive sprintf %s cleanly otherwise. + $label = null === $bad_post_id ? 'NULL' : ( '' === $bad_post_id ? "''" : "'{$bad_post_id}'" ); + $result = Emails::send_test_email( $bad_post_id, 'tester@example.com' ); + self::assertInstanceOf( + WP_Error::class, + $result, + "post_id {$label} must be rejected" + ); + self::assertSame( + 'newspack_emails_invalid_post_id', + $result->get_error_code(), + "post_id {$label} wrong error code" + ); + } + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Digit-only string post_id normalizes to int and routes to the + * post-id branch correctly. Locks in the send_email + * gettype()-branching hardening: numeric-string callers (from + * $_POST, post meta, cast-naive integrations) no longer silently + * fall through to the string-name branch. + */ + public function test_send_test_email_accepts_numeric_string_post_id() { + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'draft', + ] + ); + + $admin = self::login_as_admin(); + try { + // Cast post_id to a string to simulate what a $_POST + // or untyped REST param would deliver pre-absint. + $result = Emails::send_test_email( (string) $test_email['post_id'], 'tester@example.com' ); + self::assertTrue( $result, 'Numeric-string post_id must be accepted (normalized to int internally).' ); + } finally { + self::logout_admin( $admin ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + /** * Subscriber-level user calling the REST route is blocked by * api_permissions_check (the route's permission_callback). From 3ccbc4618d4aaef383cca3b7bc8e8991188035e8 Mon Sep 17 00:00:00 2001 From: Katie Wilkerson Rethman Date: Fri, 5 Jun 2026 22:22:55 -0500 Subject: [PATCH 5/5] fix(emails): guard test-send post type/config + locale/save robustness (NPPD-1547) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../includes/emails/class-emails.php | 23 +++ .../src/other-scripts/emails/index.js | 52 +++--- .../tests/unit-tests/emails.php | 148 ++++++++++++++++++ 3 files changed, 198 insertions(+), 25 deletions(-) diff --git a/plugins/newspack-plugin/includes/emails/class-emails.php b/plugins/newspack-plugin/includes/emails/class-emails.php index ea1f1fd64c..62ab51f3e4 100644 --- a/plugins/newspack-plugin/includes/emails/class-emails.php +++ b/plugins/newspack-plugin/includes/emails/class-emails.php @@ -537,6 +537,18 @@ private static function validate_send_prerequisites( $post_id, $to ) { [ 'status' => 404 ] ); } + // Require the Newspack email CPT explicitly. serialize_email() keys + // off post meta (EMAIL_CONFIG_NAME_META / EMAIL_HTML_META) that a + // non-email post could also carry, so gate on post_type — otherwise + // a matching-meta post of another type could reach the test-send + // dispatch path. + if ( self::POST_TYPE !== get_post_type( $post_id ) ) { + return new \WP_Error( + 'newspack_emails_wrong_post_type', + esc_html__( 'The provided post is not a Newspack email.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); + } $email_config = self::serialize_email( null, $post_id ); if ( ! $email_config ) { return new \WP_Error( @@ -559,6 +571,17 @@ private static function validate_send_prerequisites( $post_id, $to ) { [ 'status' => 422 ] ); } + // Beyond non-empty, require the config name to be a REGISTERED type. + // An unknown/stale name (e.g. a renamed provider type left on an old + // draft) would otherwise reach dispatch and re-resolve to nothing + // downstream, producing a blank-bodied email. Fail fast instead. + if ( ! isset( self::get_email_configs()[ $config_name ] ) ) { + return new \WP_Error( + 'newspack_emails_config_name_unknown', + esc_html__( 'Email is associated with an unregistered config type. Cannot dispatch.', 'newspack-plugin' ), + [ 'status' => 422 ] + ); + } return [ 'config' => $email_config, 'name' => $config_name, diff --git a/plugins/newspack-plugin/src/other-scripts/emails/index.js b/plugins/newspack-plugin/src/other-scripts/emails/index.js index 19c2131908..a826544132 100644 --- a/plugins/newspack-plugin/src/other-scripts/emails/index.js +++ b/plugins/newspack-plugin/src/other-scripts/emails/index.js @@ -70,32 +70,34 @@ const ReaderRevenueEmailSidebar = compose( [ const sendTestEmail = async () => { setInFlight( true ); - await savePost(); - apiFetch( { - path: `/newspack/v1/newspack-emails/test`, - method: 'POST', - data: { - recipient: settings.testRecipient, - post_id: postId, - }, - } ) - .then( () => { - createNotice( 'success', __( 'Test email sent!', 'newspack-plugin' ) ); - } ) - .catch( error => { - // Surface the server's specific message when present - // (NPPD-1547 added structured error codes / messages - // for each prerequisite failure: invalid recipient, - // missing HTML payload, trashed post, etc.). The - // `__( 'Test email was not sent.' )` generic is the - // fallback for unstructured errors (network failures, - // CORS, etc.) — those don't have a `.message` field. - const message = ( error && error.message ) || __( 'Test email was not sent.', 'newspack-plugin' ); - createNotice( 'error', message ); - } ) - .finally( () => { - setInFlight( false ); + // Single try/catch around BOTH savePost() and apiFetch() with a + // shared finally cleanup. Previously only the apiFetch().finally() + // cleared the in-flight state, so a rejected savePost() (a failed + // draft save) left the sidebar stuck in "Sending…" with no notice — + // the apiFetch chain never ran. Now either failure surfaces a notice + // and always resets inFlight. + try { + await savePost(); + await apiFetch( { + path: `/newspack/v1/newspack-emails/test`, + method: 'POST', + data: { + recipient: settings.testRecipient, + post_id: postId, + }, } ); + createNotice( 'success', __( 'Test email sent!', 'newspack-plugin' ) ); + } catch ( error ) { + // Surface the server's specific message when present (NPPD-1547 + // added structured error codes / messages for each prerequisite + // failure: invalid recipient, missing HTML payload, trashed post, + // etc.). The generic is the fallback for unstructured errors + // (a savePost failure, network failures, CORS) with no `.message`. + const message = ( error && error.message ) || __( 'Test email was not sent.', 'newspack-plugin' ); + createNotice( 'error', message ); + } finally { + setInFlight( false ); + } }; return ( <> diff --git a/plugins/newspack-plugin/tests/unit-tests/emails.php b/plugins/newspack-plugin/tests/unit-tests/emails.php index ddf9b8080b..693bfaaa27 100644 --- a/plugins/newspack-plugin/tests/unit-tests/emails.php +++ b/plugins/newspack-plugin/tests/unit-tests/emails.php @@ -402,6 +402,154 @@ public function test_test_send_blocked_when_config_name_missing() { } } + /** + * A non-email post that happens to carry the email meta keys must not + * reach the dispatch path. serialize_email() keys off post meta, so the + * post_type guard is what keeps a matching-meta post of another type out. + */ + public function test_test_send_blocked_for_wrong_post_type() { + $post_id = wp_insert_post( + [ + 'post_type' => 'post', + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 wrong-post-type test', + 'meta_input' => [ + Emails::EMAIL_CONFIG_NAME_META => 'test-email-config', + \Newspack_Newsletters::EMAIL_HTML_META => 'stub', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_wrong_post_type', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * A non-empty but UNREGISTERED config name (e.g. a renamed provider type + * left on an old draft) is rejected before dispatch — otherwise it would + * re-resolve to nothing downstream and send a blank-bodied email. + */ + public function test_test_send_blocked_when_config_name_unknown() { + $post_id = wp_insert_post( + [ + 'post_type' => Emails::POST_TYPE, + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 unknown-config-name test', + 'meta_input' => [ + Emails::EMAIL_CONFIG_NAME_META => 'this-config-does-not-exist', + \Newspack_Newsletters::EMAIL_HTML_META => 'stub', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_config_name_unknown', $result->get_error_code() ); + self::assertSame( 422, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * The `wp_mail_content_type` filter is request-scoped: dispatch_email + * must remove it after a SUCCESSFUL send, or subsequent plain-text mail + * in the same request silently becomes html. + */ + public function test_dispatch_removes_content_type_filter_on_success() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'Sanity: the test send should succeed.' ); + self::assertFalse( + has_filter( 'wp_mail_content_type' ), + 'wp_mail_content_type must be removed after a successful dispatch.' + ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * ...and removed after a FAILED send too — the try/finally in + * dispatch_email is what guarantees the filter never leaks past the + * wp_mail() call. wp_mail is forced to fail via `pre_wp_mail`. + */ + public function test_dispatch_removes_content_type_filter_on_failure() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + add_filter( 'pre_wp_mail', '__return_false' ); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_test_dispatch_failed', $result->get_error_code() ); + self::assertFalse( + has_filter( 'wp_mail_content_type' ), + 'wp_mail_content_type must be removed even when dispatch fails.' + ); + } finally { + remove_filter( 'pre_wp_mail', '__return_false' ); + self::logout_admin( $admin ); + } + } + + /** + * The locale switch wraps the whole send operation. After a SUCCESSFUL + * send the locale must be restored. The admin is given a distinct + * (`fr_FR`) user locale so a real switch happens and a leak would be + * observable. + */ + public function test_send_test_email_restores_locale_on_success() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + update_user_meta( $admin[0], 'locale', 'fr_FR' ); + $before = get_locale(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'Sanity: the test send should succeed.' ); + self::assertSame( $before, get_locale(), 'Locale must be restored after a successful test-send.' ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * ...and restored on an EARLY prerequisite failure too (here an invalid + * recipient, which returns before dispatch). The switch is the + * outermost operation, so every return path must unwind it. + */ + public function test_send_test_email_restores_locale_on_early_error() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + update_user_meta( $admin[0], 'locale', 'fr_FR' ); + $before = get_locale(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'not-an-email' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( $before, get_locale(), 'Locale must be restored even when an early prerequisite check fails.' ); + } finally { + self::logout_admin( $admin ); + } + } + /** * Trashed posts cannot be test-sent. The publish-status gate * skip in NPPD-1547 is for the "draft / inactive but being