diff --git a/includes/class-alert-manager.php b/includes/class-alert-manager.php index df733a48a4..f780688c10 100644 --- a/includes/class-alert-manager.php +++ b/includes/class-alert-manager.php @@ -96,10 +96,85 @@ public static function init() { add_action( 'newspack_sync_retry_exhausted', [ __CLASS__, 'handle_sync_retry_exhausted' ] ); add_action( 'newspack_data_event_retry_exhausted', [ __CLASS__, 'handle_data_event_retry_exhausted' ] ); add_action( 'newspack_integration_health_check_failed', [ __CLASS__, 'handle_health_check_failed' ] ); + add_action( 'newspack_alert', [ __CLASS__, 'forward_alert_to_log' ] ); add_action( self::PATTERN_SCAN_HOOK, [ __CLASS__, 'scan_failure_patterns' ] ); add_action( 'init', [ __CLASS__, 'schedule_pattern_scan' ] ); } + /** + * Forward a `newspack_alert` to the `newspack_log` action so Newspack + * Manager's Logger routes it. Severity drives the destination: + * + * - severity = 'error' or 'critical' → type 'error', log_level 3 + * (Alert — Slack) + * - anything else (incl. 'warning', unknown, or missing severity) → + * type 'debug', log_level 2 (Watch — logstash only) + * + * Only known error severities escalate to Slack so an unanticipated + * alert shape (e.g. a third-party `newspack_alert` with no severity) + * lands in Watch rather than paging on-call. + * + * Only the human-readable `message` is forwarded as free text. Any + * contact email carried in the alert `context` is passed through + * Logger's first-class `user_email` param — a structured field that is + * not part of the Slack message body — instead of being interpolated + * into `message`. The rest of the `context` is intentionally dropped to + * avoid leaking source payloads into downstream logs. + * + * When Newspack Manager isn't active, `newspack_log` is a no-op. + * + * @param mixed $alert The alert payload fired by this class. + */ + public static function forward_alert_to_log( $alert ) { + if ( ! is_array( $alert ) || ! isset( $alert['message'] ) || ! is_scalar( $alert['message'] ) || '' === (string) $alert['message'] ) { + return; + } + + $code = is_scalar( $alert['type'] ?? null ) && '' !== (string) $alert['type'] + ? (string) $alert['type'] + : 'newspack_alert'; + + $severity = is_scalar( $alert['severity'] ?? null ) ? (string) $alert['severity'] : ''; + $is_error = in_array( $severity, [ 'error', 'critical' ], true ); + + $params = [ + 'type' => $is_error ? 'error' : 'debug', + 'log_level' => $is_error ? 3 : 2, + ]; + + $user_email = self::get_alert_user_email( $alert ); + if ( '' !== $user_email ) { + $params['user_email'] = $user_email; + } + + do_action( 'newspack_log', $code, (string) $alert['message'], $params ); + } + + /** + * Extract the contact email (if any) carried in an alert's `context` so + * it can be forwarded via Logger's structured `user_email` param rather + * than interpolated into the human-readable message. + * + * @param array $alert The alert payload. + * + * @return string The contact email, or '' when none is present. + */ + private static function get_alert_user_email( $alert ) { + $context = is_array( $alert['context'] ?? null ) ? $alert['context'] : []; + + // Failure-pattern alerts grouped by contact email carry it as the group value. + if ( 'contact_email' === ( $context['group_by'] ?? '' ) && is_scalar( $context['group_value'] ?? null ) ) { + return (string) $context['group_value']; + } + + // Sync/handler exhaustion payloads carry the contact under `contact.email`. + if ( is_array( $context['contact'] ?? null ) && is_scalar( $context['contact']['email'] ?? null ) ) { + return (string) $context['contact']['email']; + } + + return ''; + } + /** * Schedule the recurring pattern scan via WP-Cron. */ @@ -159,11 +234,13 @@ public static function record_failure( $payload ) { * @param array $payload Alert data from Contact_Sync. */ public static function handle_sync_retry_exhausted( $payload ) { + // The contact email is intentionally left out of the message; it is + // forwarded to the log via Logger's structured `user_email` param + // (see forward_alert_to_log) and remains available in `context`. $message = sprintf( - 'Max retries (%d) reached for integration "%s" sync of %s. Last error: %s', + 'Max retries (%d) reached for integration "%s" contact sync. Last error: %s', $payload['retry_count'] ?? 0, $payload['integration_id'] ?? 'unknown', - $payload['contact']['email'] ?? 'unknown', $payload['reason'] ?? 'unknown' ); @@ -287,11 +364,15 @@ function ( $entry ) use ( $global_cutoff ) { continue; } - $message = sprintf( - 'Pattern detected: %d failures with %s "%s" in the last %s.', + // When grouping by contact email, keep the email out of the + // message; it is forwarded via Logger's `user_email` param + // (see forward_alert_to_log) and stays in `context`. + $is_email_group = 'contact_email' === $rule['group_by']; + $message = sprintf( + 'Pattern detected: %d failures with %s%s in the last %s.', count( $entries ), $rule['label'], - $group_value, + $is_email_group ? '' : sprintf( ' "%s"', $group_value ), self::format_interval( $rule['interval'] ) ); diff --git a/tests/unit-tests/alert-manager.php b/tests/unit-tests/alert-manager.php index 71c099bbc1..268a034300 100644 --- a/tests/unit-tests/alert-manager.php +++ b/tests/unit-tests/alert-manager.php @@ -18,6 +18,7 @@ class Newspack_Test_Alert_Manager extends WP_UnitTestCase { public function tear_down() { parent::tear_down(); remove_all_actions( 'newspack_alert' ); + remove_all_actions( 'newspack_log' ); remove_all_filters( 'newspack_alert_pattern_rules' ); remove_all_filters( 'newspack_alert_failure_record' ); delete_option( Alert_Manager::FAILURE_LOG_OPTION ); @@ -357,6 +358,292 @@ function ( $data ) use ( &$fire_count ) { $this->assertEquals( 1, $fire_count, 'Second scan should be deduplicated.' ); } + /** + * Test that firing `newspack_alert` reaches `newspack_log` via the + * registered listener with log_level 3 (Alert → Slack) for error severity. + */ + public function test_newspack_alert_emits_newspack_log_via_listener() { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $captured = null; + add_action( + 'newspack_log', + function ( $code, $message, $params ) use ( &$captured ) { + $captured = compact( 'code', 'message', 'params' ); + }, + 10, + 3 + ); + + do_action( + 'newspack_alert', + [ + 'type' => 'sync_retry_exhausted', + 'severity' => 'error', + 'message' => 'Boom', + 'context' => [ 'integration_id' => 'mailchimp' ], + 'timestamp' => time(), + ] + ); + + $this->assertNotNull( $captured, 'newspack_log should fire via the newspack_alert listener.' ); + $this->assertSame( 'sync_retry_exhausted', $captured['code'] ); + $this->assertSame( 'Boom', $captured['message'] ); + $this->assertSame( 'error', $captured['params']['type'] ); + $this->assertSame( 3, $captured['params']['log_level'] ); + $this->assertArrayNotHasKey( 'data', $captured['params'], 'Context should not be forwarded as data.' ); + } + + /** + * Test severity-to-destination routing. Only known error severities + * escalate to Slack (log_level 3); everything else — including + * 'warning', unknown values, and a missing severity — lands in Watch + * (log_level 2) so unanticipated alert shapes do not page on-call. + * + * @dataProvider data_severity_routing + * + * @param array $alert Alert payload to forward. + * @param string $expected_type Expected forwarded log `type`. + * @param int $expected_lvl Expected forwarded `log_level`. + */ + public function test_severity_routing( $alert, $expected_type, $expected_lvl ) { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $captured = null; + add_action( + 'newspack_log', + function ( $code, $message, $params ) use ( &$captured ) { + $captured = compact( 'code', 'message', 'params' ); + }, + 10, + 3 + ); + + do_action( 'newspack_alert', $alert ); + + $this->assertNotNull( $captured ); + $this->assertSame( $expected_type, $captured['params']['type'] ); + $this->assertSame( $expected_lvl, $captured['params']['log_level'] ); + } + + /** + * Severity routing scenarios. + */ + public function data_severity_routing() { + return [ + 'error → Alert/Slack' => [ + [ + 'severity' => 'error', + 'message' => 'x', + ], + 'error', + 3, + ], + 'critical → Alert/Slack' => [ + [ + 'severity' => 'critical', + 'message' => 'x', + ], + 'error', + 3, + ], + 'warning → Watch' => [ + [ + 'severity' => 'warning', + 'message' => 'x', + ], + 'debug', + 2, + ], + 'info → Watch' => [ + [ + 'severity' => 'info', + 'message' => 'x', + ], + 'debug', + 2, + ], + 'empty severity → Watch' => [ + [ + 'severity' => '', + 'message' => 'x', + ], + 'debug', + 2, + ], + 'missing severity → Watch' => [ [ 'message' => 'x' ], 'debug', 2 ], + ]; + } + + /** + * Test that an alert without a `type` falls back to the default + * `newspack_alert` log code. + */ + public function test_alert_without_type_uses_default_code() { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $captured = null; + add_action( + 'newspack_log', + function ( $code, $message, $params ) use ( &$captured ) { + $captured = compact( 'code', 'message', 'params' ); + }, + 10, + 3 + ); + + do_action( + 'newspack_alert', + [ + 'severity' => 'error', + 'message' => 'No type here', + ] + ); + + $this->assertNotNull( $captured ); + $this->assertSame( 'newspack_alert', $captured['code'] ); + } + + /** + * Test that a numeric-zero message is still forwarded (it casts to the + * non-empty string '0'), unlike (bool) false which casts to ''. + */ + public function test_numeric_zero_message_is_forwarded() { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $captured = null; + add_action( + 'newspack_log', + function ( $code, $message, $params ) use ( &$captured ) { + $captured = compact( 'code', 'message', 'params' ); + }, + 10, + 3 + ); + + do_action( + 'newspack_alert', + [ + 'severity' => 'error', + 'message' => 0, + ] + ); + + $this->assertNotNull( $captured, 'A numeric 0 message should still be forwarded.' ); + $this->assertSame( '0', $captured['message'] ); + } + + /** + * Test that a contact email in the alert context is forwarded via + * Logger's structured `user_email` param and is NOT interpolated into + * the human-readable message that reaches Slack. + */ + public function test_contact_email_forwarded_via_user_email_param() { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $captured = null; + add_action( + 'newspack_log', + function ( $code, $message, $params ) use ( &$captured ) { + $captured = compact( 'code', 'message', 'params' ); + }, + 10, + 3 + ); + + // Sync/handler exhaustion payload: contact under context.contact.email. + do_action( + 'newspack_sync_retry_exhausted', + [ + 'integration_id' => 'mailchimp', + 'contact' => [ 'email' => 'reader@example.com' ], + 'retry_count' => 5, + 'reason' => 'Invalid API key', + ] + ); + + $this->assertNotNull( $captured ); + $this->assertSame( 'reader@example.com', $captured['params']['user_email'] ); + $this->assertStringNotContainsString( 'reader@example.com', $captured['message'], 'Email must not leak into the message.' ); + } + + /** + * Test that a `same_user` failure pattern (grouped by contact email) + * forwards the email via `user_email` and keeps it out of the message. + */ + public function test_same_user_pattern_email_forwarded_via_user_email_param() { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $captured = null; + add_action( + 'newspack_log', + function ( $code, $message, $params ) use ( &$captured ) { + if ( 'failure_pattern' === $code || str_contains( (string) $message, 'Pattern detected' ) ) { + $captured = compact( 'code', 'message', 'params' ); + } + }, + 10, + 3 + ); + + // Five failures for the same contact email (same_user threshold is 5). + $log = []; + for ( $i = 0; $i < 5; $i++ ) { + $log[] = [ + 'timestamp' => time() - 60, + 'integration_id' => "esp{$i}", + 'contact_email' => 'reader@example.com', + 'action_name' => "action_{$i}", + 'reason' => "reason {$i}", + ]; + } + update_option( Alert_Manager::FAILURE_LOG_OPTION, $log, false ); + + Alert_Manager::scan_failure_patterns(); + + $this->assertNotNull( $captured, 'A same_user pattern alert should be forwarded.' ); + $this->assertSame( 'reader@example.com', $captured['params']['user_email'] ); + $this->assertStringNotContainsString( 'reader@example.com', $captured['message'], 'Email must not leak into the message.' ); + } + + /** + * Test that malformed alerts (non-array, missing or non-scalar message) + * do not fire `newspack_log`. + * + * @dataProvider data_malformed_alerts + * + * @param mixed $alert The alert payload to forward. + */ + public function test_malformed_alert_does_not_emit_log( $alert ) { + add_action( 'newspack_alert', [ Alert_Manager::class, 'forward_alert_to_log' ] ); + + $fired = false; + add_action( + 'newspack_log', + function () use ( &$fired ) { + $fired = true; + } + ); + + do_action( 'newspack_alert', $alert ); + + $this->assertFalse( $fired, 'newspack_log should not fire for malformed alerts.' ); + } + + /** + * Malformed alert payloads. + */ + public function data_malformed_alerts() { + return [ + 'non-array' => [ 'string' ], + 'missing message' => [ [ 'type' => 'x' ] ], + 'non-scalar message' => [ [ 'message' => [ 'not', 'a', 'string' ] ] ], + 'empty string message' => [ [ 'message' => '' ] ], + // (bool) false casts to '' so it is skipped like an empty string. + 'false message' => [ [ 'message' => false ] ], + ]; + } + /** * Test that the pattern scan cron event is scheduled. */