From 8ccf41529861856b0a9bec475e8b6a447d9c3e46 Mon Sep 17 00:00:00 2001 From: Griffith Chen Date: Tue, 5 May 2026 20:38:28 +0800 Subject: [PATCH 1/3] Wpcomsh fatal-error: log recovery-mode entry from screen click Routes the fatal-error screen's "Enter recovery mode" link through a first-party signed redirect endpoint that emits `wpcomsh_fatal_recovery` to logstash before forwarding to a freshly-generated core recovery URL. Email-originated entries continue to use the bare core URL and don't log, so the new event measures only screen-originated recovery-link uptake. Refactors out shared helpers used by both this endpoint and the deactivator: HMAC sign/verify (`wpcomsh_fatal_sign_payload` / `wpcomsh_fatal_verify_payload`), per-key dedup with fail-open semantics (`wpcomsh_fatal_dedup_acquire`), and the WPCOMSH_Log dispatch funnel (`wpcomsh_fatal_emit_logstash_event`). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../add-wpcomsh-fatal-error-recovery-log | 4 + .../wpcomsh/wpcom-fatal-error/README.md | 1 + .../wpcom-fatal-error/fatal-error-helpers.php | 314 ++++++++++++++---- .../wpcom-fatal-error/fatal-error-screen.php | 21 +- .../fatal-plugin-deactivator.php | 48 +-- .../fatal-recovery-redirect.php | 89 +++++ .../wpcomsh/wpcom-fatal-error/load.php | 4 + 7 files changed, 366 insertions(+), 115 deletions(-) create mode 100644 projects/plugins/wpcomsh/changelog/add-wpcomsh-fatal-error-recovery-log create mode 100644 projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php diff --git a/projects/plugins/wpcomsh/changelog/add-wpcomsh-fatal-error-recovery-log b/projects/plugins/wpcomsh/changelog/add-wpcomsh-fatal-error-recovery-log new file mode 100644 index 000000000000..acedda89f852 --- /dev/null +++ b/projects/plugins/wpcomsh/changelog/add-wpcomsh-fatal-error-recovery-log @@ -0,0 +1,4 @@ +Significance: patch +Type: added + +Wpcomsh fatal-error: route the screen's "Enter recovery mode" link through a first-party redirect endpoint that logs a `wpcomsh_fatal_recovery` event before forwarding to a freshly-generated core recovery URL, so we can measure screen-originated recovery clicks alongside the existing signature and deactivate events without conflating them with email-originated entries. diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/README.md b/projects/plugins/wpcomsh/wpcom-fatal-error/README.md index dd3cd197ce39..424a3c2061ad 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/README.md +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/README.md @@ -31,6 +31,7 @@ path to deactivate the offending plugin. | `fatal-error-helpers.php` | Pure helpers: viewer detection, plugin identification, signed-form/recovery URL builders. Testable in isolation. | | `fatal-error-screen.css` | Styles, inlined into the page at render time. | | `fatal-plugin-deactivator.php` | Early-running endpoint that validates the signed deactivation POST, persists the change, and redirects. | +| `fatal-recovery-redirect.php` | Early-running endpoint behind the screen's "Enter recovery mode" link: logs `wpcomsh_fatal_recovery` and 302s to a fresh core recovery URL. | ## Architecture notes diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php index c5e30bc43bc4..ef4d31098eec 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php @@ -134,19 +134,16 @@ function wpcomsh_fatal_log_deactivate( $plugin_basename ) { } /** - * Emit a fatal-error event via WPCOMSH_Log::unsafe_direct_log_logstash(), - * alongside the decoded signature parts so consumers don't have to decode - * the signature themselves. + * Emit a fatal-error event for an identified extension via + * `wpcomsh_fatal_emit_logstash_event()`, alongside the decoded signature + * parts so consumers don't have to decode the signature themselves. * * Callable from both the fatal-screen render path and the deactivator * endpoint at mu-plugin load time. The deactivator path runs before - * constants.php, so we resolve the wpcomsh root via `dirname(__DIR__)` - * rather than WPCOMSH__PLUGIN_DIR_PATH. + * constants.php, so the helper resolves the wpcomsh root via + * `dirname(__DIR__)` rather than WPCOMSH__PLUGIN_DIR_PATH. * - * Manual require with `class_exists( …, false )` skips the autoloader during - * fatal handling, where its filesystem reads could compound a bad state. - * - * Best-effort: silently no-ops if either dependency is unreachable. A logging + * Best-effort: silently no-ops if dependencies are unreachable. A logging * failure must never escalate into a second fatal. * * @param array|null $plugin Extension metadata from wpcomsh_fatal_identify_plugin(). @@ -158,10 +155,8 @@ function wpcomsh_fatal_log_event( $plugin, $message ) { return; } - $wpcomsh_root = dirname( __DIR__ ); - if ( ! function_exists( 'wpcom_build_fatal_error_signature' ) ) { - $helper = $wpcomsh_root . '/jetpack_vendor/automattic/jetpack-mu-wpcom/src/common/fatal-error-signature.php'; + $helper = dirname( __DIR__ ) . '/jetpack_vendor/automattic/jetpack-mu-wpcom/src/common/fatal-error-signature.php'; if ( is_readable( $helper ) ) { require_once $helper; } @@ -185,17 +180,51 @@ function wpcomsh_fatal_log_event( $plugin, $message ) { // emit one log row + one outbound HTTP per visitor. $message is part of // the key so a deactivate event isn't suppressed by a recent signature // event for the same extension. - $cache_key = 'wpcomsh_fatal_event:' . hash( 'sha256', $message . '|' . $signature ); - try { - if ( ! wp_cache_add( $cache_key, 1, 'wpcomsh', HOUR_IN_SECONDS ) ) { - return; + if ( ! wpcomsh_fatal_dedup_acquire( 'wpcomsh_fatal_event:' . hash( 'sha256', $message . '|' . $signature ), HOUR_IN_SECONDS ) ) { + return; + } + + $properties = array( 'signature' => $signature ); + + // Round-trip through the decoder so the logged parts always agree + // with the signature. + if ( function_exists( 'wpcom_decode_fatal_error_signature' ) ) { + $parts = wpcom_decode_fatal_error_signature( $signature ); + if ( is_array( $parts ) ) { + $properties['kind'] = $parts['kind']; + $properties['slug'] = $parts['slug']; + $properties['extension_version'] = $parts['version']; + $properties['wp_version'] = $parts['wp']; + $properties['php_version'] = $parts['php']; } - } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- fail open: a cache failure should not block telemetry. - // Fall through and log. } + wpcomsh_fatal_emit_logstash_event( $message, $properties ); +} + +/** + * Dispatch a fatal-flow event to the `atomic_extension_conflict` logstash + * bucket, merging in shared site identifiers (`site_url`, `atomic_site_id`) + * and loading WPCOMSH_Log on demand. + * + * Manual require with `class_exists( …, false )` skips the autoloader during + * fatal handling, where its filesystem reads could compound a bad state. + * + * Both the per-extension event path (`wpcomsh_fatal_log_event`) and the + * recovery-mode entry path (`wpcomsh_fatal_log_recovery`) funnel through + * here so the shared bucket / severity / site identifiers stay in one place. + * Caller-supplied properties win on key collision. + * + * Best-effort: silently no-ops if WPCOMSH_Log is unreachable, and never lets + * a dispatch failure bubble out. + * + * @param string $message Event message slug. + * @param array $properties Event-specific properties merged ahead of site identifiers. + * @return void + */ +function wpcomsh_fatal_emit_logstash_event( $message, $properties = array() ) { if ( ! class_exists( 'WPCOMSH_Log', false ) ) { - $log_file = $wpcomsh_root . '/class-wpcomsh-log.php'; + $log_file = dirname( __DIR__ ) . '/class-wpcomsh-log.php'; if ( is_readable( $log_file ) ) { require_once $log_file; } @@ -204,15 +233,15 @@ function wpcomsh_fatal_log_event( $plugin, $message ) { return; } - $properties = array( 'signature' => $signature ); - // `get_site_url()` runs the `site_url` / `option_siteurl` filters, so a - // misbehaving filter could throw — keep the lookup in its own guard so - // the rest of the signature still makes it to logstash. - try { - $properties['site_url'] = get_site_url(); - } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- best-effort; omit site_url if a filter misbehaves. - // Fall through. + // misbehaving filter could throw — guard so the rest of the event still + // makes it to logstash. + if ( ! isset( $properties['site_url'] ) ) { + try { + $properties['site_url'] = get_site_url(); + } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- best-effort; omit site_url if a filter misbehaves. + // Fall through. + } } // Prefer the constant: it's set on Atomic and avoids the apply_filters() @@ -220,28 +249,17 @@ function wpcomsh_fatal_log_event( $plugin, $message ) { // only when the constant isn't defined; guard the call because the helper // runs the `wpcomsh_get_atomic_site_id` filter, and a misbehaving callback // must not bubble out of this best-effort logger. - $atomic_site_id = defined( 'ATOMIC_SITE_ID' ) ? (int) ATOMIC_SITE_ID : 0; - if ( 0 === $atomic_site_id && function_exists( 'wpcomsh_get_atomic_site_id' ) ) { - try { - $atomic_site_id = (int) wpcomsh_get_atomic_site_id(); - } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- best-effort; omit atomic_site_id if a filter misbehaves. - // Fall through. + if ( ! isset( $properties['atomic_site_id'] ) ) { + $atomic_site_id = defined( 'ATOMIC_SITE_ID' ) ? (int) ATOMIC_SITE_ID : 0; + if ( 0 === $atomic_site_id && function_exists( 'wpcomsh_get_atomic_site_id' ) ) { + try { + $atomic_site_id = (int) wpcomsh_get_atomic_site_id(); + } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- best-effort; omit atomic_site_id if a filter misbehaves. + // Fall through. + } } - } - if ( $atomic_site_id > 0 ) { - $properties['atomic_site_id'] = $atomic_site_id; - } - - // Round-trip through the decoder so the logged parts always agree - // with the signature. - if ( function_exists( 'wpcom_decode_fatal_error_signature' ) ) { - $parts = wpcom_decode_fatal_error_signature( $signature ); - if ( is_array( $parts ) ) { - $properties['kind'] = $parts['kind']; - $properties['slug'] = $parts['slug']; - $properties['extension_version'] = $parts['version']; - $properties['wp_version'] = $parts['wp']; - $properties['php_version'] = $parts['php']; + if ( $atomic_site_id > 0 ) { + $properties['atomic_site_id'] = $atomic_site_id; } } @@ -299,20 +317,31 @@ function wpcomsh_fatal_classify_plugin_path( $abs_file ) { } /** - * Build the form action + signed fields the fatal-plugin-deactivator endpoint - * validates, without relying on pluggable functions or WP nonces. + * Mint a `(exp, sig)` payload that one of the fatal-error endpoints + * (deactivator, recovery-redirect) will verify before acting. * - * Signature: HMAC over (plugin, expiry, logged_in cookie) using AUTH_SALT. - * Binding to the cookie ensures the request can't be replayed by another - * user or after the admin logs out, and can't be forged without AUTH_SALT. + * Signature: HMAC over (`$payload_prefix`, expiry, logged_in cookie) using + * AUTH_SALT. Binding to the cookie means the payload is tied to the admin's + * active session — it can't be replayed after logout, can't be replayed by + * another user, and can't be forged without AUTH_SALT. Nonces aren't usable + * here because the verifying endpoints run before pluggable.php loads. * - * Returned as form fields (rather than a signed URL) so the screen can submit - * via POST — destructive actions should not live in a GET-able link. + * `$payload_prefix` is a domain separator so a signature minted for one + * action (e.g. plugin deactivate, where the prefix is the basename) can't + * be replayed against another (e.g. recovery redirect, which uses + * `'recover'`). * - * @param string $plugin_basename Plugin file relative to WP_PLUGIN_DIR (e.g. "akismet/akismet.php"). - * @return array{action:string,fields:array}|null Form data, or null when prerequisites are missing. + * Default TTL is short (five minutes) because the deactivator action is + * destructive and we want fresh intent. Callers minting links for + * non-destructive actions should pass a longer `$ttl` so the URL doesn't + * silently expire while the admin is still on the same fatal screen — see + * `wpcomsh_fatal_build_recovery_redirect_url()` for the recovery case. + * + * @param string $payload_prefix Action-specific prefix mixed into the HMAC. + * @param int $ttl Validity window in seconds. + * @return array{exp:int,sig:string}|null Signed payload, or null when prerequisites are missing. */ -function wpcomsh_fatal_build_deactivate_form( $plugin_basename ) { +function wpcomsh_fatal_sign_payload( $payload_prefix, $ttl = 5 * MINUTE_IN_SECONDS ) { if ( ! defined( 'AUTH_SALT' ) || ! defined( 'LOGGED_IN_COOKIE' ) ) { return null; } @@ -320,25 +349,178 @@ function wpcomsh_fatal_build_deactivate_form( $plugin_basename ) { return null; } // The cookie is used only as a per-session secret inside an HMAC we - // never output, so sanitization is irrelevant here — we need its exact + // never output, so sanitization is irrelevant — we need its exact // byte-for-byte value to match at verification time. $cookie_value = (string) wp_unslash( $_COOKIE[ LOGGED_IN_COOKIE ] ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - $exp = time() + 5 * MINUTE_IN_SECONDS; - $sig = hash_hmac( - 'sha256', - $plugin_basename . '|' . $exp . '|' . $cookie_value, - (string) AUTH_SALT + $exp = time() + $ttl; + return array( + 'exp' => $exp, + 'sig' => hash_hmac( 'sha256', $payload_prefix . '|' . $exp . '|' . $cookie_value, (string) AUTH_SALT ), ); +} + +/** + * Counterpart to `wpcomsh_fatal_sign_payload()`: validate that the supplied + * `$exp` / `$sig` came from this site (HMAC over the same payload prefix + * keyed on AUTH_SALT) and the same authenticated session (logged_in cookie + * mixed into the HMAC), and that the payload hasn't expired. + * + * Bootstraps cookie constants on demand so the helper is callable from + * mu-plugin-time endpoints, before wp-settings.php's cookie_constants() + * runs. Returns false on any prerequisite or comparison failure — callers + * treat that as "reject the request." + * + * @param string $payload_prefix Action-specific prefix mixed into the HMAC. + * @param int $exp Expiry timestamp from the signed payload. + * @param string $sig Signature presented by the request. + * @return bool + */ +function wpcomsh_fatal_verify_payload( $payload_prefix, $exp, $sig ) { + if ( ! defined( 'AUTH_SALT' ) ) { + return false; + } + // Cookie constants (LOGGED_IN_COOKIE etc.) are defined later in + // wp-settings.php — between muplugins_loaded and active_plugins + // iteration. At mu-plugin load time they don't exist yet, but + // wp_cookie_constants() is already available. + if ( ! defined( 'LOGGED_IN_COOKIE' ) && function_exists( 'wp_cookie_constants' ) ) { + wp_cookie_constants(); + } + if ( ! defined( 'LOGGED_IN_COOKIE' ) ) { + return false; + } + if ( $exp < time() ) { + return false; + } + // Cookie is the per-session secret; sanitization would destroy the + // byte-for-byte match against the value present at sign time. + $cookie_value = isset( $_COOKIE[ LOGGED_IN_COOKIE ] ) + ? (string) wp_unslash( $_COOKIE[ LOGGED_IN_COOKIE ] ) // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + : ''; + if ( '' === $cookie_value ) { + return false; + } + $expected = hash_hmac( 'sha256', $payload_prefix . '|' . $exp . '|' . $cookie_value, (string) AUTH_SALT ); + return hash_equals( $expected, (string) $sig ); +} + +/** + * Try to claim a 5-minute (by default) cache lock so the same event / + * action proceeds at most once per TTL window. Wraps `wp_cache_add()` in + * a try/catch because object-cache backends can throw on unreachable + * upstreams; a cache outage MUST fail open (return true) so telemetry and + * recovery actions don't silently disappear during an incident. + * + * Caller is responsible for namespacing the key. Returns true when the + * caller acquired the lock — proceed; false when another caller already + * did within the window — short-circuit. + * + * @param string $key Cache key. + * @param int $ttl Lock TTL in seconds. + * @return bool + */ +function wpcomsh_fatal_dedup_acquire( $key, $ttl = 5 * MINUTE_IN_SECONDS ) { + try { + return (bool) wp_cache_add( $key, 1, 'wpcomsh', $ttl ); + } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- fail open: a cache outage shouldn't silence telemetry or block real actions. + return true; + } +} + +/** + * Build the form action + signed fields the fatal-plugin-deactivator endpoint + * validates, without relying on pluggable functions or WP nonces. + * + * Returned as form fields (rather than a signed URL) so the screen can submit + * via POST — destructive actions should not live in a GET-able link. + * + * @param string $plugin_basename Plugin file relative to WP_PLUGIN_DIR (e.g. "akismet/akismet.php"). + * @return array{action:string,fields:array}|null Form data, or null when prerequisites are missing. + */ +function wpcomsh_fatal_build_deactivate_form( $plugin_basename ) { + $signed = wpcomsh_fatal_sign_payload( $plugin_basename ); + if ( null === $signed ) { + return null; + } return array( 'action' => home_url( '/' ), 'fields' => array( 'wpcomsh_deactivate' => $plugin_basename, - 'wpcomsh_exp' => (string) $exp, - 'wpcomsh_sig' => $sig, + 'wpcomsh_exp' => (string) $signed['exp'], + 'wpcomsh_sig' => $signed['sig'], ), ); } +/** + * Build the signed redirect URL the fatal-error screen renders behind its + * "Enter recovery mode" link. The endpoint at fatal-recovery-redirect.php + * verifies the signature, then 302s to a freshly-generated core recovery + * URL — so the recovery key store doesn't pick up a row per fatal-screen + * pageview, only per actual click. + * + * Returned as a URL (not a form payload like the deactivator) because the + * link is non-destructive — it only mints a recovery key and redirects — + * and rendering as `` keeps the "What you can try next" copy + * readable. + * + * Validity window mirrors core's `recovery_mode_email_link_ttl` filter + * (default `DAY_IN_SECONDS`) so a host customizing recovery-link TTL + * customizes ours in lockstep — and so the screen link doesn't silently + * expire while the admin is still looking at the fatal page. The + * deactivator's tighter five-minute window is deliberate there because + * deactivation is destructive. + * + * @return string Signed URL, or '' when prerequisites are missing. + */ +function wpcomsh_fatal_build_recovery_redirect_url() { + if ( is_multisite() ) { + return ''; + } + // Mirror `WP_Recovery_Mode::get_link_ttl()`: rate_limit floor → link_ttl + // starts from that floor → final TTL is max of both. Each filter is + // guarded because callbacks may belong to the fataling extension; a + // throw must not turn the recovery-link render into a second fatal — + // fall back to the value we passed in. + $apply_filter_safe = static function ( $hook, $fallback ) { + try { + return (int) apply_filters( $hook, $fallback ); + } catch ( \Throwable $e ) { + return $fallback; + } + }; + /** This filter is documented in wp-includes/class-wp-recovery-mode.php */ + $rate_limit = $apply_filter_safe( 'recovery_mode_email_rate_limit', DAY_IN_SECONDS ); + /** This filter is documented in wp-includes/class-wp-recovery-mode.php */ + $valid_for = $apply_filter_safe( 'recovery_mode_email_link_ttl', $rate_limit ); + $signed = wpcomsh_fatal_sign_payload( 'recover', max( $valid_for, $rate_limit ) ); + if ( null === $signed ) { + return ''; + } + // Build on `site_url()` rather than `home_url()`: verification at the + // endpoint requires LOGGED_IN_COOKIE, which on host-only cookie setups + // (no COOKIE_DOMAIN) is bound to the host where wp-login.php ran — i.e. + // `site_url()`'s host. A wrapper URL on `home_url()` would arrive + // without the cookie on installs where the two hosts differ, and verify + // would silently reject the click. + // + // `site_url()` runs the `site_url` / `option_siteurl` filters; a + // callback belonging to the fataling extension could throw and re-fatal + // the screen — drop the link rather than escalate. + try { + return add_query_arg( + array( + 'wpcomsh_recover' => '1', + 'wpcomsh_exp' => (string) $signed['exp'], + 'wpcomsh_sig' => $signed['sig'], + ), + site_url( '/' ) + ); + } catch ( \Throwable $e ) { + return ''; + } +} + /** * Generate a recovery-mode URL using core's link service. * diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php index b1cce73f9126..27a135f8c1d3 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php @@ -43,15 +43,7 @@ function wpcomsh_customize_fatal_error_message( $message, $error = array() ) { / wpcomsh_fatal_log_event( $plugin, 'wpcomsh_fatal_signature' ); } elseif ( ! empty( $error['file'] ) ) { $coarse_key = 'wpcomsh_fatal_file:' . hash( 'sha256', (string) $error['file'] ); - $do_log = true; - - try { - $do_log = wp_cache_add( $coarse_key, 1, 'wpcomsh', HOUR_IN_SECONDS ); - } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- fail open: a cache failure should not silence telemetry. - // Fail open. - } - - if ( $do_log ) { + if ( wpcomsh_fatal_dedup_acquire( $coarse_key, HOUR_IN_SECONDS ) ) { wpcomsh_fatal_log_event( wpcomsh_fatal_identify_plugin( $error ), 'wpcomsh_fatal_signature' ); } } @@ -117,7 +109,16 @@ function wpcomsh_fatal_build_render_context( $error, $plugin = null, $user_id = 'plugin' => $plugin, 'error_message' => $is_admin ? (string) ( $error['message'] ?? '' ) : '', 'deactivate_form' => $can_deactivate ? wpcomsh_fatal_build_deactivate_form( $plugin['basename'] ) : null, - 'recovery_url' => $can_recover ? wpcomsh_fatal_build_recovery_url() : '', + // Point at our signed redirect endpoint (fatal-recovery-redirect.php) + // rather than the bare core URL, so a successful click is, by + // construction, a screen click — the email always carries the core + // URL. The endpoint mints a fresh recovery URL only after verifying + // the signature, which keeps the recovery_keys option from + // accumulating a row per fatal-screen pageview and prevents a + // CSRF-style navigation from triggering key generation. The helper + // also gates multisite, since core's recovery flow doesn't + // initialize there. + 'recovery_url' => $can_recover ? wpcomsh_fatal_build_recovery_redirect_url() : '', 'support_url' => 'https://wordpress.com/help/contact', 'environment' => $is_admin ? wpcomsh_fatal_get_environment_lines() : array(), ); diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-plugin-deactivator.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-plugin-deactivator.php index d0979903ea35..00a410da3826 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-plugin-deactivator.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-plugin-deactivator.php @@ -30,54 +30,24 @@ * @return void */ function wpcomsh_fatal_maybe_deactivate_plugin() { - // Nonces aren't usable in this endpoint (pluggable.php hasn't loaded yet); - // we validate an HMAC signature below instead. The early-return check only - // reads the parameter *presence* — actual values are validated after. - // phpcs:ignore WordPress.Security.NonceVerification.Missing + // Nonces aren't usable here (pluggable.php hasn't loaded yet); the HMAC + // check below is the actual auth gate. The presence check + regex below + // only constrain the inputs before they're trusted. + // phpcs:disable WordPress.Security.NonceVerification.Missing if ( empty( $_POST['wpcomsh_deactivate'] ) || empty( $_POST['wpcomsh_sig'] ) || empty( $_POST['wpcomsh_exp'] ) ) { return; } - if ( ! defined( 'AUTH_SALT' ) ) { - return; - } - // Cookie constants (LOGGED_IN_COOKIE etc.) are defined later in wp-settings.php - // — between muplugins_loaded and active_plugins iteration. At mu-plugin load - // time they don't exist yet, but wp_cookie_constants() is already available. - if ( ! defined( 'LOGGED_IN_COOKIE' ) && function_exists( 'wp_cookie_constants' ) ) { - wp_cookie_constants(); - } - if ( ! defined( 'LOGGED_IN_COOKIE' ) ) { - return; - } - - // Nonces aren't usable here because pluggable.php hasn't loaded yet — we - // validate an HMAC signature below instead. Inputs are constrained by - // regex / cast to int before being trusted. - // phpcs:disable WordPress.Security.NonceVerification.Missing - $plugin = isset( $_POST['wpcomsh_deactivate'] ) ? sanitize_text_field( wp_unslash( $_POST['wpcomsh_deactivate'] ) ) : ''; - $sig = isset( $_POST['wpcomsh_sig'] ) ? sanitize_text_field( wp_unslash( $_POST['wpcomsh_sig'] ) ) : ''; - $exp = isset( $_POST['wpcomsh_exp'] ) ? (int) $_POST['wpcomsh_exp'] : 0; + $plugin = sanitize_text_field( wp_unslash( $_POST['wpcomsh_deactivate'] ) ); + $sig = sanitize_text_field( wp_unslash( $_POST['wpcomsh_sig'] ) ); + $exp = (int) $_POST['wpcomsh_exp']; // phpcs:enable WordPress.Security.NonceVerification.Missing - // Reject expired or malformed plugin paths (no traversal; slug/file.php only). - if ( $exp < time() ) { - return; - } + // Reject malformed plugin paths (no traversal; slug/file.php only). if ( ! preg_match( '#^[a-zA-Z0-9][a-zA-Z0-9_.-]*/[a-zA-Z0-9][a-zA-Z0-9_.-]*\.php$#', $plugin ) ) { return; } - // The cookie is used only as a per-session secret inside an HMAC we - // never output; sanitization would destroy the byte-for-byte match. - $cookie_value = isset( $_COOKIE[ LOGGED_IN_COOKIE ] ) - ? (string) wp_unslash( $_COOKIE[ LOGGED_IN_COOKIE ] ) // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - : ''; - if ( '' === $cookie_value ) { - return; - } - - $expected = hash_hmac( 'sha256', $plugin . '|' . $exp . '|' . $cookie_value, (string) AUTH_SALT ); - if ( ! hash_equals( $expected, $sig ) ) { + if ( ! wpcomsh_fatal_verify_payload( $plugin, $exp, $sig ) ) { return; } diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php new file mode 100644 index 000000000000..3a7f3d39b21d --- /dev/null +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php @@ -0,0 +1,89 @@ + Date: Wed, 6 May 2026 18:13:52 +0800 Subject: [PATCH 2/3] Wpcomsh fatal-error: harden recovery-redirect endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the custom HMAC on the recovery click endpoint with a WP nonce (`wpcomsh_recover` action). The recovery endpoint runs after pluggable.php loads via `wpcomsh_fatal_current_user_id`, so `wp_verify_nonce` is callable there; the deactivator endpoint keeps its HMAC because it has to verify before pluggable.php is available. Pin `nonce_life` for the recovery action via a `PHP_INT_MAX`-priority filter registered lazily at the mint and verify call sites, so the tick used at mint time (screen render, plugins loaded) and verify time (mu-plugin file load, plugins not yet loaded) always agree. Mint under the cookie-resolved user via `wp_set_current_user` so an ambient `$current_user` mismatch can't sign for the wrong session. Failure paths now 302 to a path on the WP install root with the recovery query args stripped, instead of letting WP render a normal page response under `?wpcomsh_recover=1&…`. The redirect base comes from `wp_parse_url( site_url('/'), PHP_URL_PATH )`, not `$_SERVER['REQUEST_URI']`, so a crafted `//attacker.com/path` request URI can't produce a cross-origin `Location:` header. Subdirectory installs (e.g. example.com/wp/) bounce back into the install root. Unrelated query params survive via `remove_query_arg` over `$_SERVER['QUERY_STRING']`. The header is sent raw rather than via `wp_redirect` so early bails (empty nonce, multisite) don't pull pluggable.php just to bounce. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../wpcom-fatal-error/fatal-error-helpers.php | 145 +++++++++------- .../wpcom-fatal-error/fatal-error-screen.php | 15 +- .../fatal-recovery-redirect.php | 160 +++++++++++++----- 3 files changed, 204 insertions(+), 116 deletions(-) diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php index ef4d31098eec..e0cc6d6ba52a 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-helpers.php @@ -211,9 +211,9 @@ function wpcomsh_fatal_log_event( $plugin, $message ) { * fatal handling, where its filesystem reads could compound a bad state. * * Both the per-extension event path (`wpcomsh_fatal_log_event`) and the - * recovery-mode entry path (`wpcomsh_fatal_log_recovery`) funnel through - * here so the shared bucket / severity / site identifiers stay in one place. - * Caller-supplied properties win on key collision. + * recovery-redirect endpoint funnel through here so the shared bucket / + * severity / site identifiers stay in one place. Caller-supplied + * properties win on key collision. * * Best-effort: silently no-ops if WPCOMSH_Log is unreachable, and never lets * a dispatch failure bubble out. @@ -317,25 +317,20 @@ function wpcomsh_fatal_classify_plugin_path( $abs_file ) { } /** - * Mint a `(exp, sig)` payload that one of the fatal-error endpoints - * (deactivator, recovery-redirect) will verify before acting. + * Mint a `(exp, sig)` payload that the deactivator endpoint verifies + * before acting. * * Signature: HMAC over (`$payload_prefix`, expiry, logged_in cookie) using * AUTH_SALT. Binding to the cookie means the payload is tied to the admin's * active session — it can't be replayed after logout, can't be replayed by * another user, and can't be forged without AUTH_SALT. Nonces aren't usable - * here because the verifying endpoints run before pluggable.php loads. + * here because the verifying endpoint runs before pluggable.php loads. * * `$payload_prefix` is a domain separator so a signature minted for one - * action (e.g. plugin deactivate, where the prefix is the basename) can't - * be replayed against another (e.g. recovery redirect, which uses - * `'recover'`). + * action can't be replayed against another. * * Default TTL is short (five minutes) because the deactivator action is - * destructive and we want fresh intent. Callers minting links for - * non-destructive actions should pass a longer `$ttl` so the URL doesn't - * silently expire while the admin is still on the same fatal screen — see - * `wpcomsh_fatal_build_recovery_redirect_url()` for the recovery case. + * destructive and we want fresh intent. * * @param string $payload_prefix Action-specific prefix mixed into the HMAC. * @param int $ttl Validity window in seconds. @@ -453,66 +448,51 @@ function wpcomsh_fatal_build_deactivate_form( $plugin_basename ) { } /** - * Build the signed redirect URL the fatal-error screen renders behind its - * "Enter recovery mode" link. The endpoint at fatal-recovery-redirect.php - * verifies the signature, then 302s to a freshly-generated core recovery - * URL — so the recovery key store doesn't pick up a row per fatal-screen - * pageview, only per actual click. - * - * Returned as a URL (not a form payload like the deactivator) because the - * link is non-destructive — it only mints a recovery key and redirects — - * and rendering as `` keeps the "What you can try next" copy - * readable. - * - * Validity window mirrors core's `recovery_mode_email_link_ttl` filter - * (default `DAY_IN_SECONDS`) so a host customizing recovery-link TTL - * customizes ours in lockstep — and so the screen link doesn't silently - * expire while the admin is still looking at the fatal page. The - * deactivator's tighter five-minute window is deliberate there because - * deactivation is destructive. - * - * @return string Signed URL, or '' when prerequisites are missing. + * Build the URL the fatal-error screen renders behind its "Enter + * recovery mode" link. The endpoint at fatal-recovery-redirect.php + * verifies the nonce + capability, then 302s to a freshly-generated + * core recovery URL — so the recovery key store doesn't pick up a row + * per fatal-screen pageview, only per actual click. + * + * The nonce binds the URL to (action, user_id, session_token, tick) + * via `wp_create_nonce()`, so a CSRF-style navigation can't reach the + * endpoint without first extracting the nonce from the rendered + * screen. The deactivator endpoint keeps its custom HMAC because it + * runs at mu-plugin file-load time, before pluggable.php loads and + * `wp_create_nonce` / `wp_verify_nonce` are callable. + * + * @param int $user_id Cookie-resolved user the link is being rendered for. Pinning + * `$current_user` to this id before minting the nonce makes + * sure verification (which sets the same id) sees a + * matching signature — without it, an ambient `$current_user` + * that differs from the cookie-bound admin (e.g. a request + * that called `wp_set_current_user( 0 )` earlier) would mint + * a nonce for the wrong user and the click would silently + * no-op. + * @return string Redirect URL, or '' when prerequisites are missing. */ -function wpcomsh_fatal_build_recovery_redirect_url() { - if ( is_multisite() ) { +function wpcomsh_fatal_build_recovery_redirect_url( $user_id ) { + if ( is_multisite() || ! $user_id ) { return ''; } - // Mirror `WP_Recovery_Mode::get_link_ttl()`: rate_limit floor → link_ttl - // starts from that floor → final TTL is max of both. Each filter is - // guarded because callbacks may belong to the fataling extension; a - // throw must not turn the recovery-link render into a second fatal — - // fall back to the value we passed in. - $apply_filter_safe = static function ( $hook, $fallback ) { - try { - return (int) apply_filters( $hook, $fallback ); - } catch ( \Throwable $e ) { - return $fallback; - } - }; - /** This filter is documented in wp-includes/class-wp-recovery-mode.php */ - $rate_limit = $apply_filter_safe( 'recovery_mode_email_rate_limit', DAY_IN_SECONDS ); - /** This filter is documented in wp-includes/class-wp-recovery-mode.php */ - $valid_for = $apply_filter_safe( 'recovery_mode_email_link_ttl', $rate_limit ); - $signed = wpcomsh_fatal_sign_payload( 'recover', max( $valid_for, $rate_limit ) ); - if ( null === $signed ) { - return ''; - } - // Build on `site_url()` rather than `home_url()`: verification at the - // endpoint requires LOGGED_IN_COOKIE, which on host-only cookie setups - // (no COOKIE_DOMAIN) is bound to the host where wp-login.php ran — i.e. + // Build on `site_url()` rather than `home_url()`: the recovery flow + // requires LOGGED_IN_COOKIE, which on host-only cookie setups (no + // COOKIE_DOMAIN) is bound to the host where wp-login.php ran — i.e. // `site_url()`'s host. A wrapper URL on `home_url()` would arrive - // without the cookie on installs where the two hosts differ, and verify - // would silently reject the click. + // without the cookie on installs where the two hosts differ, and the + // endpoint would silently reject the click. // - // `site_url()` runs the `site_url` / `option_siteurl` filters; a - // callback belonging to the fataling extension could throw and re-fatal - // the screen — drop the link rather than escalate. + // `site_url()` runs the `site_url` / `option_siteurl` filters and + // `wp_set_current_user()` fires the `set_current_user` action; a + // callback belonging to the fataling extension could throw and + // re-fatal the screen — drop the link rather than escalate. try { + wp_set_current_user( $user_id ); + wpcomsh_fatal_pin_recover_nonce_life(); return add_query_arg( array( 'wpcomsh_recover' => '1', - 'wpcomsh_exp' => (string) $signed['exp'], - 'wpcomsh_sig' => $signed['sig'], + '_wpnonce' => wp_create_nonce( 'wpcomsh_recover' ), ), site_url( '/' ) ); @@ -521,6 +501,43 @@ function wpcomsh_fatal_build_recovery_redirect_url() { } } +/** + * Register a `nonce_life` filter at `PHP_INT_MAX` that pins `DAY_IN_SECONDS` + * for the `wpcomsh_recover` action only, so the tick used at mint time + * (screen render, after plugins load) and verify time (mu-plugin file load, + * before plugins iterate) always agrees. Without this, a regular plugin + * that unconditionally filters `nonce_life` at default priority would + * shift the mint-side tick but not the verify-side tick (its callback + * isn't registered yet at mu-plugin time), and the click would silently + * fail verification. + * + * Idempotent (named callback) so call sites can invoke this lazily, just + * before `wp_create_nonce` / `wp_verify_nonce` — keeps the filter out of + * `nonce_life` dispatch on requests that don't touch the recovery flow. + * + * @return void + */ +function wpcomsh_fatal_pin_recover_nonce_life() { + add_filter( 'nonce_life', '_wpcomsh_fatal_recover_nonce_life', PHP_INT_MAX, 2 ); +} + +/** + * Filter callback for `wpcomsh_fatal_pin_recover_nonce_life()`. Returns + * `DAY_IN_SECONDS` for the `wpcomsh_recover` action and the upstream + * value for everything else. + * + * `$action = null` so a one-arg `apply_filters( 'nonce_life', $life )` from + * plugin code (allowed by the public filter contract) doesn't trip an + * ArgumentCountError under PHP 8+. + * + * @param int $life Upstream nonce lifetime. + * @param string|null $action Action passed to `wp_nonce_tick()`. + * @return int + */ +function _wpcomsh_fatal_recover_nonce_life( $life, $action = null ) { + return 'wpcomsh_recover' === $action ? DAY_IN_SECONDS : $life; +} + /** * Generate a recovery-mode URL using core's link service. * diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php index 27a135f8c1d3..0c8ff3f4214e 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-error-screen.php @@ -109,16 +109,11 @@ function wpcomsh_fatal_build_render_context( $error, $plugin = null, $user_id = 'plugin' => $plugin, 'error_message' => $is_admin ? (string) ( $error['message'] ?? '' ) : '', 'deactivate_form' => $can_deactivate ? wpcomsh_fatal_build_deactivate_form( $plugin['basename'] ) : null, - // Point at our signed redirect endpoint (fatal-recovery-redirect.php) - // rather than the bare core URL, so a successful click is, by - // construction, a screen click — the email always carries the core - // URL. The endpoint mints a fresh recovery URL only after verifying - // the signature, which keeps the recovery_keys option from - // accumulating a row per fatal-screen pageview and prevents a - // CSRF-style navigation from triggering key generation. The helper - // also gates multisite, since core's recovery flow doesn't - // initialize there. - 'recovery_url' => $can_recover ? wpcomsh_fatal_build_recovery_redirect_url() : '', + // Endpoint-mediated link so the recovery key is minted on click + // (one row in the `recovery_keys` option per click, not per + // render) and we can log the click. Helper gates multisite. See + // fatal-recovery-redirect.php for the auth model. + 'recovery_url' => $can_recover ? wpcomsh_fatal_build_recovery_redirect_url( $user_id ) : '', 'support_url' => 'https://wordpress.com/help/contact', 'environment' => $is_admin ? wpcomsh_fatal_get_environment_lines() : array(), ); diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php index 3a7f3d39b21d..50b80c68645e 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php @@ -3,74 +3,150 @@ * One-shot redirect endpoint behind the fatal-error screen's * "Enter recovery mode" link. * - * The screen renders an HMAC-signed URL pointing here instead of core's - * recovery-mode URL directly, so a successful click is, by construction, a - * click from the screen we rendered to the same authenticated session — - * the recovery-mode email always carries the bare core URL, and a CSRF- - * style navigation can't forge the signature without AUTH_SALT. Minting - * the recovery key here (instead of at fatal-screen render time) also - * keeps the recovery_keys option from accumulating a row per fatal-screen - * pageview. + * The screen points its recovery link here + * (`?wpcomsh_recover=1&_wpnonce=…` on `site_url()`) rather than at the + * bare core recovery URL so we can: + * - log a `wpcomsh_fatal_recovery` event keyed on the actual click, + * not on every fatal-screen pageview; + * - mint the core recovery key on click rather than at render time, + * keeping the `recovery_keys` option from accumulating a row per + * pageview. * - * Security model and load-order argument mirror fatal-plugin-deactivator.php. + * Auth: WP nonce + cookie-resolved current user + `resume_plugins` / + * `resume_themes` capability. The nonce binds the URL to the admin's + * session, so a CSRF-style navigation can't reach the endpoint + * without the rendered nonce. The deactivator endpoint keeps its + * custom HMAC because it runs before pluggable.php loads, where + * `wp_verify_nonce` isn't available. + * + * Failure paths 302 back to the same URL with the recovery query args + * stripped, instead of letting WP serve a normal page response under + * `?wpcomsh_recover=1&…`. Two reasons: the user-visible URL stops + * carrying the suspicious params (so a refresh or back-nav doesn't + * re-trigger the endpoint), and any side-effecting bootstraps below + * (the pluggable.php load via `wpcomsh_fatal_current_user_id`) land on + * a header-only 302 response — never a rendered page where a regular + * plugin's pluggable override would silently no-op because pluggable + * was already loaded at mu-plugin time. + * + * Load-order argument mirrors fatal-plugin-deactivator.php. * * @package wpcomsh */ /** - * Verify the signed URL, dedup, mint a fresh core recovery URL, log the - * click, and 302 to it. Best-effort: silently no-ops if anything is off. + * Validate the click, dedup the log, mint a fresh core recovery URL, + * and 302 to it. On any failure after we recognize this as a recovery + * click, 302 to the same URL minus the recovery query args. * * @return void */ function wpcomsh_fatal_maybe_handle_recovery_click() { - // Nonces aren't usable here (pluggable.php hasn't loaded yet); the HMAC - // check below is the actual auth gate. - // phpcs:disable WordPress.Security.NonceVerification.Recommended - if ( empty( $_GET['wpcomsh_recover'] ) || empty( $_GET['wpcomsh_sig'] ) || empty( $_GET['wpcomsh_exp'] ) ) { - return; + // phpcs:disable WordPress.Security.NonceVerification.Recommended -- nonce verified by wp_verify_nonce() below. + if ( empty( $_GET['wpcomsh_recover'] ) ) { + return; // Not our request — let WP serve normally. } - // The screen never emits this URL on multisite, so any matching request - // here is bogus. Bail before the verifier so an invalid GET can't cause - // `wp_cookie_constants()` to run ahead of `ms_cookie_constants()`. - if ( is_multisite() ) { - return; + + // Raw header() rather than wp_redirect(): the early bails (empty + // nonce, multisite) run before pluggable.php is loaded, and pulling + // pluggable in just to bounce defeats the point of containing the + // preload damage. + // + // Derive the redirect base from `site_url('/')` rather than echoing + // $_SERVER['REQUEST_URI']: a crafted protocol-relative request URI + // like `//attacker.com/path` would otherwise produce a `Location: + // //attacker.com/path` header that browsers follow cross-origin (open + // redirect). Routing through site_url also lands the user at the WP + // install root on subdirectory installs (e.g. example.com/wp/) + // instead of the domain root. Try/catch covers a misbehaving + // `site_url` filter from an earlier mu-plugin throwing. + // + // Only the query string is carried over from the request — read from + // $_SERVER['QUERY_STRING'] (no host can be smuggled in there), with + // the recovery args stripped via `remove_query_arg` (which accepts a + // query-only string and re-encodes via WP's standard `build_query`). + $bail_clean = static function () { + $base_path = '/'; + try { + $path = wp_parse_url( site_url( '/' ), PHP_URL_PATH ); + if ( is_string( $path ) && '' !== $path ) { + $base_path = $path; + } + } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- fall back to '/' on filter throw. + // Fall through. + } + + // `remove_query_arg` accepts a query-only string and re-encodes the + // remaining args through WP's `build_query`, so the raw bytes from + // QUERY_STRING are safe to pass through — text-field sanitization + // would mangle valid query syntax (`+`, etc.) for no benefit. + $query = remove_query_arg( + array( 'wpcomsh_recover', '_wpnonce' ), + (string) wp_unslash( $_SERVER['QUERY_STRING'] ?? '' ) // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- see comment above; remove_query_arg parses + re-encodes. + ); + $location = $base_path; + if ( '' !== $query ) { + $location .= '?' . $query; + } + header( 'Location: ' . $location, true, 302 ); + exit; + }; + + if ( empty( $_GET['_wpnonce'] ) ) { + $bail_clean(); } - $sig = sanitize_text_field( wp_unslash( $_GET['wpcomsh_sig'] ) ); - $exp = (int) $_GET['wpcomsh_exp']; + $nonce = sanitize_text_field( wp_unslash( $_GET['_wpnonce'] ) ); // phpcs:enable WordPress.Security.NonceVerification.Recommended - if ( ! wpcomsh_fatal_verify_payload( 'recover', $exp, $sig ) ) { - return; + // The screen never emits this URL on multisite, so any matching request + // here is bogus. + if ( is_multisite() ) { + $bail_clean(); } - // HMAC binding only proves the request carries the same logged-in - // cookie *bytes* as when the URL was minted — a stale cookie (server- - // side session invalidation, password reset, account demotion) still - // byte-matches. With day-long TTLs, that gap matters: validate the - // cookie against current user state and re-check the recovery capability - // before minting a core recovery URL. wpcomsh_fatal_current_user_id() - // also loads pluggable.php as a side effect, which both - // wpcomsh_fatal_build_recovery_url() (via wp_generate_password) and - // wp_safe_redirect() below rely on. + // wpcomsh_fatal_current_user_id() bootstraps pluggable.php (needed by + // wp_verify_nonce / wp_redirect / the recovery-link service below) + // and validates the auth cookie without setting `$current_user`. We + // then call wp_set_current_user ourselves so wp_verify_nonce uses + // our resolved id instead of re-validating the cookie, and pin the + // `nonce_life` filter so the verify-time tick agrees with the + // mint-time tick across load phases. The resolve+verify cluster is + // wrapped in try/catch because the `set_current_user` action can be + // hooked by another mu-plugin loaded earlier in this pass — a throw + // must not turn the recovery-link click into a second fatal. $user_id = wpcomsh_fatal_current_user_id(); if ( ! $user_id ) { - return; + $bail_clean(); } - if ( ! user_can( $user_id, 'resume_plugins' ) && ! user_can( $user_id, 'resume_themes' ) ) { - return; + try { + wp_set_current_user( $user_id ); + wpcomsh_fatal_pin_recover_nonce_life(); + if ( ! wp_verify_nonce( $nonce, 'wpcomsh_recover' ) ) { + $bail_clean(); + } + } catch ( \Throwable $e ) { + $bail_clean(); + } + + // `current_user_can` rather than `user_can( $user_id, ... )`: we just + // called `wp_set_current_user`, so `$current_user` is populated and + // the cap check skips a redundant `get_userdata()` lookup. + if ( ! current_user_can( 'resume_plugins' ) && ! current_user_can( 'resume_themes' ) ) { + $bail_clean(); } $recovery_url = wpcomsh_fatal_build_recovery_url(); if ( '' === $recovery_url ) { - return; + $bail_clean(); } // Dedup gates the *log only*, not the redirect: a refresh / back-nav - // of the same signed URL shouldn't flood log rows, but the user must - // still reach recovery mode — otherwise the link silently looks broken. - // (Core recovery keys are single-use, so each click mints a fresh one.) - if ( wpcomsh_fatal_dedup_acquire( 'wpcomsh_fatal_event:recovery:' . $sig ) ) { + // shouldn't flood log rows, but the user must still reach recovery + // mode — otherwise the link silently looks broken. (Core recovery + // keys are single-use, so each click mints a fresh one.) Per-user + // keying is enough because the cap check above already constrains + // callers to admins on this site. + if ( wpcomsh_fatal_dedup_acquire( 'wpcomsh_fatal_event:recovery:' . $user_id ) ) { wpcomsh_fatal_emit_logstash_event( 'wpcomsh_fatal_recovery' ); } From 02760716da3cd4d1fe1b02ba8db3c152ede8464a Mon Sep 17 00:00:00 2001 From: Griffith Chen Date: Wed, 6 May 2026 18:22:09 +0800 Subject: [PATCH 3/3] Wpcomsh fatal-error: declare bail_clean closure as ': never' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phan's PhanPluginNeverReturnFunction caught the closure: it always exits and has no return path. wpcomsh requires PHP 8.1+, so the native `never` return type is available — keeps the closure's exit-only contract visible to the type checker rather than relying on inferred void. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php index 50b80c68645e..abc6dbc6e31b 100644 --- a/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php +++ b/projects/plugins/wpcomsh/wpcom-fatal-error/fatal-recovery-redirect.php @@ -65,7 +65,7 @@ function wpcomsh_fatal_maybe_handle_recovery_click() { // $_SERVER['QUERY_STRING'] (no host can be smuggled in there), with // the recovery args stripped via `remove_query_arg` (which accepts a // query-only string and re-encodes via WP's standard `build_query`). - $bail_clean = static function () { + $bail_clean = static function (): never { $base_path = '/'; try { $path = wp_parse_url( site_url( '/' ), PHP_URL_PATH );