From 12361f155ee3d35feef09e2390abddae8224b25d Mon Sep 17 00:00:00 2001 From: Chris Portscheller Date: Wed, 29 Jul 2026 21:39:59 -0500 Subject: [PATCH] fix: card-testing evidence inflation, the dead intel store, and the monitor counter (#61 #62 #63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #61 — one reused order could manufacture a card-testing verdict. track_attempt() records a row per checkout SUBMISSION, not per order: woocommerce_checkout_order_processed fires on every submission and WC_Checkout::create_order() reuses the order held in the order_awaiting_payment session. So a customer retrying a declined card produced several rows against one order_id and could trip Patterns 1, 2 and 4 alone. Those three now require evidence one order cannot fabricate — three distinct orders or more than one card. Pattern 3 already keys on distinct cards and is unchanged. Deliberately NOT fixed by deduping rows per order_id: a real carding bot cycling cards through one reused cart also produces a single order_id, so a dedupe would collapse the attack to one row and blind every pattern plus the velocity counter. #62 — the actor feed was a write-only dead store. record_intel() wrote an option and intel_for() had zero callers, so 2.3.2 removed the enforcement and replaced it with nothing, which deleted the feature rather than re-scoping it. intel_for() is now the primary source for the CRITICAL-moment notice, which was paying for a blocking HTTP lookup on a page render; the live lookup is the fallback when the local feed has nothing. Two notices also asserted things that stopped being true when the feed stopped blocking — "already blocked network-wide" with a success chip linking to a blocked-IPs page that no longer holds feed rows, and a Pro upsell claiming the actor "would have been blocked before its first request". Both now describe recognition rather than a block. Removed the block-era carcass: enforce_cap(), the blocker() accessor and its property, EXPIRY_HOURS. Corrected the class docblock, which still described mirroring the feed into the blocked-IPs table, and recorded that exclude_allowlisted() is exact-match only — acceptable while the store is advisory, not if a blocking consumer ever returns. #63 — the monitor-mode notice could read "no request has met the bar for enforcement yet" on a default install where rate limiting was the only thing being suppressed, because THROTTLE was excluded from the counter. Now counted, keyed on the rule name rather than the reason: a throttle reason embeds the request counter and would mint a fresh breakdown key per request. A throttled request already writes webdecoy_rate_limits and inserts a detections row, so this adds no new order of cost. Also corrected the docblock that said "autoloaded counter" when the call passes autoload=false. 80/80 tests, phpcs error count unchanged (14, all pre-existing in tests/bootstrap.php), phpstan unchanged. --- includes/class-webdecoy-actor-feed.php | 86 ++++++--------------- includes/class-webdecoy-critical-moment.php | 37 +++++++-- includes/class-webdecoy-woocommerce.php | 25 +++++- webdecoy.php | 26 ++++--- 4 files changed, 88 insertions(+), 86 deletions(-) diff --git a/includes/class-webdecoy-actor-feed.php b/includes/class-webdecoy-actor-feed.php index 7047242..e6e3245 100644 --- a/includes/class-webdecoy-actor-feed.php +++ b/includes/class-webdecoy-actor-feed.php @@ -2,11 +2,18 @@ /** * WebDecoy Actor Feed * - * Pre-emptive network blocking. When this site is connected to WebDecoy Cloud - * AND entitled to the actor feed (Pro+), an hourly cron pulls the shared - * "known attacker" IP feed and mirrors it into the existing blocked-IPs table - * with a rolling 48h expiry — so those attackers are denied before they ever - * reach the site. + * Cross-site attacker INTELLIGENCE. When this site is connected to WebDecoy Cloud + * AND entitled to the actor feed (Pro+), an hourly cron pulls the shared "known + * attacker" list and stores it locally, where {@see self::intel_for()} answers + * "has this address been seen attacking other sites recently?". + * + * It does NOT block. Until 2.3.2 it mirrored the feed into the blocked-IPs table + * with a rolling 48h expiry, which was measured net-negative: only 2 of 4,866 + * addresses were ever seen at more than one site, 93% of hostile addresses are gone + * within the hour, 82% of feed entries were already a week stale and none were still + * active — so blocking on it mostly hit whoever holds the address now. Treat a hit + * as a scoring input or a reason to rate-limit, never as grounds to block. + * Refs WebDecoy/app#476. * * Zero external calls unless connected AND entitled. The cron is only scheduled * while both hold ({@see self::maybe_reconcile_schedule()}), and the handler @@ -14,14 +21,17 @@ * ({@see self::sync()}). A `403 upgrade_required` from the feed also unschedules * quietly. * - * Coexistence with the user's own blocks — hard invariants enforced here: - * - network rows carry `created_by = 'webdecoy-network'`. Rows created by - * anyone else (a human admin, 'system', rate-limiter) are NEVER updated or - * deleted by this class. - * - an IP present in the user's ip_allowlist is NEVER inserted (exact match and - * CIDR range, via {@see WebDecoy_Blocker::is_allowlisted()}). - * - at most {@see self::MAX_NETWORK_ROWS} network rows are kept; the oldest - * (least-recently-seen) are evicted first. + * Invariants: + * - nothing here writes to the blocked-IPs table. {@see self::purge_feed_blocks()} + * removes the rows earlier versions wrote, and is the only statement this class + * still issues against that table. Rows created by anyone else — a human admin, + * 'system', the rate limiter — are never touched. + * - an address in the user's ip_allowlist is never stored. Exact match only: + * {@see self::exclude_allowlisted()} is a pure filter with no CIDR support. That + * is acceptable because the store is advisory — a stored address grants nothing + * and denies nothing — but do not reintroduce a blocking consumer without + * restoring a CIDR-aware allowlist gate. + * - at most {@see self::MAX_NETWORK_ROWS} addresses are kept per sync. * * @package WebDecoy */ @@ -61,8 +71,6 @@ class WebDecoy_Actor_Feed /** Entitlement feature key that gates the whole mechanism. */ private const FEATURE = 'actor_feed'; - /** Rolling block lifetime (hours). Refreshed every sync for live feed IPs. */ - private const EXPIRY_HOURS = 48; /** Hard cap on the number of network-owned rows. */ private const MAX_NETWORK_ROWS = 2000; @@ -73,9 +81,6 @@ class WebDecoy_Actor_Feed /** Seconds per hour — literal to keep the class loadable without WordPress. */ private const HOUR = 3600; - /** @var WebDecoy_Blocker|null Lazily created for allowlist checks. */ - private $blocker = null; - /** * Wire up the cron. The handler is registered unconditionally (wp-cron can * fire on any front-end request); the schedule itself is reconciled against @@ -322,44 +327,6 @@ public static function purge_feed_blocks(): int return $deleted; } - /** - * Trim network-owned rows back to the cap, evicting the oldest first. - * - * blocked_at proxies last_seen: it is refreshed to "now" for every IP still - * present in the feed each sync, so the smallest blocked_at is the row that - * has gone longest without appearing in the feed — the right one to drop. - */ - private function enforce_cap(): void - { - global $wpdb; - $table = $wpdb->prefix . 'webdecoy_blocked_ips'; - - $count = (int) $wpdb->get_var($wpdb->prepare( - "SELECT COUNT(*) FROM {$table} WHERE created_by = %s", - self::CREATED_BY - )); - if ($count <= self::MAX_NETWORK_ROWS) { - return; - } - - $excess = $count - self::MAX_NETWORK_ROWS; - $ids = $wpdb->get_col($wpdb->prepare( - "SELECT id FROM {$table} WHERE created_by = %s ORDER BY blocked_at ASC, id ASC LIMIT %d", - self::CREATED_BY, - $excess - )); - - if (!$ids) { - return; - } - - $placeholders = implode(',', array_fill(0, count($ids), '%d')); - $wpdb->query($wpdb->prepare( - "DELETE FROM {$table} WHERE id IN ({$placeholders})", - $ids - )); - } - /** * Decrypted API key from the plugin's in-request options. */ @@ -387,13 +354,6 @@ private function allowlist(): array return is_array($allow) ? $allow : []; } - private function blocker(): WebDecoy_Blocker - { - if ($this->blocker === null) { - $this->blocker = new WebDecoy_Blocker(); - } - return $this->blocker; - } // --------------------------------------------------------------------- // Pure helpers (no WordPress calls) — unit-tested in tests/ActorFeedTest.php. diff --git a/includes/class-webdecoy-critical-moment.php b/includes/class-webdecoy-critical-moment.php index 2d1ca8f..dd0253c 100644 --- a/includes/class-webdecoy-critical-moment.php +++ b/includes/class-webdecoy-critical-moment.php @@ -143,7 +143,22 @@ private function build_notice(string $ip): array $org_id = isset($options['organization_id']) ? (string) $options['organization_id'] : ''; $api_key = isset($options['api_key']) ? (string) $options['api_key'] : ''; - if ($api_key !== '' && class_exists('WebDecoy_Actor_Intel')) { + // Prefer the locally synced feed: the hourly actor-feed sync already holds + // this data, so consulting it costs an option read instead of a blocking + // HTTP round trip on a page render. This is also the consumer the feed was + // missing — before this it wrote an option nothing ever read (#62). + if ($has_actor_feed && class_exists('WebDecoy_Actor_Feed')) { + $local = (new WebDecoy_Actor_Feed())->intel_for($ip); + if ($local !== null && $local['last_seen'] > 0) { + $known = true; + $days = self::days_since($local['last_seen'], time()); + } + } + + // Fall back to the live lookup only when the local feed had nothing — + // either the org has no feed entitlement or the actor is newer than the + // last sync. + if (!$known && $api_key !== '' && class_exists('WebDecoy_Actor_Intel')) { $intel = new WebDecoy_Actor_Intel($api_key); $map = $intel->lookup([$ip]); $entry = isset($map[$ip]) && is_array($map[$ip]) ? $map[$ip] : null; @@ -160,9 +175,13 @@ private function build_notice(string $ip): array return [ 'message' => sprintf( /* translators: %d: number of days since first seen on the network */ + // Must not claim Pro would have BLOCKED this. Since 2.3.2 the + // cross-site feed is advisory and writes nothing to the block + // list, on any plan — see #476. Sell the history, not a block + // that does not happen. _n( - 'This attacker was first seen on the WebDecoy network %d day ago — on Pro it would have been blocked before its first request.', - 'This attacker was first seen on the WebDecoy network %d days ago — on Pro it would have been blocked before its first request.', + 'This attacker was first seen on the WebDecoy network %d day ago — Pro shows you the full cross-site history for this actor.', + 'This attacker was first seen on the WebDecoy network %d days ago — Pro shows you the full cross-site history for this actor.', $n, 'webdecoy' ), @@ -176,11 +195,15 @@ private function build_notice(string $ip): array ]; case 'connected_covered': + // Was "already blocked network-wide" with a success chip pointing at + // the blocked-IPs page. Since 2.3.2 the feed blocks nothing and that + // page is empty of feed entries, so the claim was false and the link + // went nowhere useful. Recognition is real; the block was not. #62. return [ - 'message' => __('This attacker is already blocked network-wide — your actor feed already covers threats like this before they reach your site.', 'webdecoy'), - 'cta_label' => __('View blocked IPs', 'webdecoy'), - 'cta_url' => admin_url('admin.php?page=webdecoy-blocked'), - 'type' => 'success', + 'message' => __('This attacker is already known across the WebDecoy network — the same actor has been seen attacking other sites.', 'webdecoy'), + 'cta_label' => __('View detections', 'webdecoy'), + 'cta_url' => admin_url('admin.php?page=webdecoy-detections'), + 'type' => 'warning', ]; case 'connected_unknown': diff --git a/includes/class-webdecoy-woocommerce.php b/includes/class-webdecoy-woocommerce.php index 9e90147..4d46136 100644 --- a/includes/class-webdecoy-woocommerce.php +++ b/includes/class-webdecoy-woocommerce.php @@ -156,12 +156,28 @@ private function detect_card_testing(string $ip): bool return false; } + // Evidence one order cannot manufacture. track_attempt() records one row per + // checkout SUBMISSION, not per order — woocommerce_checkout_order_processed + // fires on every submission and WC_Checkout::create_order() reuses the order + // held in the order_awaiting_payment session. So a customer retrying a declined + // card produces several rows against ONE order_id and could trip Patterns 1, 2 + // and 4 on their own. Those three now require either several distinct orders or + // more than one card. Refs #61. + // + // Deliberately NOT solved by deduping rows per order_id: a real carding bot + // cycling cards through one reused cart also produces a single order_id, so a + // dedupe would collapse the attack to one row and blind every pattern plus the + // velocity counter. + $distinct_orders = count(array_unique(array_filter(array_column($attempts, 'order_id')))); + $distinct_cards = count(array_unique(array_filter(array_column($attempts, 'card_last4')))); + $multi_source = ($distinct_orders >= 3 || $distinct_cards >= 2); + // Pattern 1: Multiple small amounts (< $5) $small_amounts = array_filter($attempts, function ($a) { return isset($a['amount']) && (float) $a['amount'] < 5.00; }); - if (count($small_amounts) >= 3) { + if ($multi_source && count($small_amounts) >= 3) { return true; } @@ -170,11 +186,12 @@ private function detect_card_testing(string $ip): bool return isset($a['status']) && $a['status'] === 'declined'; }); - if (count($declined) >= 3) { + if ($multi_source && count($declined) >= 3) { return true; } - // Pattern 3: Multiple different cards from same IP + // Pattern 3: Multiple different cards from same IP. Already keyed on distinct + // cards, so it is its own multi-source evidence and needs no extra gate. $card_last4s = array_unique(array_filter(array_column($attempts, 'card_last4'))); if (count($card_last4s) >= 3) { @@ -182,7 +199,7 @@ private function detect_card_testing(string $ip): bool } // Pattern 4: Rapid succession of attempts (< 30 seconds apart) - if (count($attempts) >= 3) { + if ($multi_source && count($attempts) >= 3) { $timestamps = array_column($attempts, 'created_at'); sort($timestamps); diff --git a/webdecoy.php b/webdecoy.php index e164938..5e57a76 100644 --- a/webdecoy.php +++ b/webdecoy.php @@ -1482,7 +1482,7 @@ private function enforce_block(string $ip, string $reason, ?int $duration = null /** * Count what enforcement would have done, so monitor mode produces a number * rather than silence. Kept as a bounded option — no new table, no per-request - * write beyond a single autoloaded counter. + * write beyond a single non-autoloaded counter. */ private function record_suppressed_action(string $suppression, string $reason): void { @@ -1527,17 +1527,19 @@ private function handle_rule_decision(\WebDecoy\Rules\RuleEngineResult $result, * @param \WebDecoy\Rules\RuleEngineResult $result */ do_action('webdecoy_enforcement_suppressed', $ip, $suppression, $result); - // Deliberately NOT counted for THROTTLE. Rate limiting is high-volume by - // nature, it previously performed no database write on this path, and one - // option write per throttled request would turn a bot flood into an - // options-table flood — on every install, since monitor mode is the - // default. The rate limiter's own counters already carry that number. - if ($result->action !== \WebDecoy\Rules\RuleResult::THROTTLE) { - $this->record_suppressed_action( - $suppression, - $result->reason ?? ('Rule enforced: ' . ($result->rule ?? 'rule')) - ); - } + // THROTTLE is counted too: rate limiting is the action most likely to meet + // real traffic, so omitting it made the number the owner decides on + // meaningless — "no request has met the bar" while everything was being + // throttled. Keyed on the RULE NAME, not the reason, because a throttle + // reason embeds the request counter and would mint a fresh breakdown key + // per request. A throttled request already writes webdecoy_rate_limits and + // inserts a detections row, so this adds no new order of cost. + $this->record_suppressed_action( + $suppression, + $result->action === \WebDecoy\Rules\RuleResult::THROTTLE + ? ('Throttled: ' . ($result->rule ?? 'rate-limit')) + : ($result->reason ?? ('Rule enforced: ' . ($result->rule ?? 'rule'))) + ); return; }