The card-testing success filter excludes nothing on COD/BACS/Cheque stores, because those gateways never call payment_complete()

[cport1]

Found by the adversarial review of PR #54 (2.3.2 safety release). Verified — the reviewer
attempted to refute this and could not.

includes/class-webdecoy-woocommerce.php:128 · severity high · lens woo

The defect

The new status <> 'success' filter assumes non-success means "failed attempt", but 'success' is only ever written by track_payment() (line 239) on woocommerce_payment_complete. WC_Gateway_COD / BACS / Cheque call $order->update_status('processing'|'on-hold') and never $order->payment_complete(), so on those stores no row is ever marked success — the filter excludes nothing and every paid-for order counts as a failed checkout attempt.

Failure scenario

Store whose only gateways are Direct bank transfer and Cash on delivery. Every order leaves a row status='attempt' permanently (order_status_failed never fires either). One IP places 5 orders in an hour — a shared office/school NAT, a rep entering phone orders, or any store behind an unconfigured proxy where all shoppers share one address — and the 6th order is refused at line 80 with "Too many checkout attempts." Pattern 1 also fires: three £3 orders (tip, donation, digital add-on) from that IP → count($small_amounts) >= 3 at line 154 → "Suspicious checkout activity detected." The comment at 138-142 asserts the new filter fixes exactly this false positive; it does not for any on-hold gateway.

Verified mechanism

Verified every link; the mechanism holds exactly as stated.

1. SET status = 'success' appears exactly once in the plugin, at includes/class-webdecoy-woocommerce.php:239, reachable only from track_payment(), hooked only to woocommerce_payment_complete (includes/class-webdecoy-woocommerce.php:662 and webdecoy.php:850).
2. Fetched WooCommerce trunk: WC_Gateway_COD::process_payment() calls $order->update_status(ON_HOLD|PROCESSING) when get_total() > 0 and reaches payment_complete() only in the zero-total else branch; WC_Gateway_BACS is identical with ON_HOLD; Cheque likewise. WC_Order::status_transition() never calls payment_complete() — do_action('woocommerce_payment_complete') fires from one place only, inside payment_complete() itself — so an admin later marking the order Processing/Completed does not fire it either.
3. track_attempt() (:214) inserts status='attempt' from woocommerce_checkout_order_processed (webdecoy.php:851), which fires before process_order_payment(). track_failure() is hooked only to woocommerce_order_status_failed. Nothing else revises the row. So on a COD/BACS/Cheque-only store every row stays 'attempt' for ever and status <> 'success' at :299 excludes nothing.
4. Arithmetic checks out: protect_checkout defaults true (webdecoy.php:291), limit 5 / window 3600. Five prior orders → count($attempts) < 5 false → refused at :80. Three prior sub-$5 rows → count($small_amounts) >= 3 at :157 → refused at :91. card_last4 is null for COD so Pattern 3 cannot fire; Pattern 4 needs 3 orders inside ~60s.

Two cosmetic corrections to the claim, neither affecting the mechanism: the comment is at 141-145 not 138-142, and the >= 3 test is at 157 not 154.

Not a regression: before the diff the query counted successes too, so on a COD store the row sets are identical and behaviour is unchanged (this release strictly improves it by removing the sitewide IP block). What makes it a blocker is the promise. changelog.txt states "Successful orders no longer count toward card-testing patterns or the checkout velocity limit" and prints the still-reproducible case as its own example: "Three successful sub-$5.00 orders from one address in an hour." readme.txt repeats it. Both are false on any COD/BACS/Cheque store — i.e. the release ships a fix claim that does not hold for a whole common gateway class, on a path that is ON by default.

Compounding: check_checkout() consults suppressed() only in the is_blocked branch (:65). The velocity, card-testing and bot-detection refusals at :80/:91/:105 — and the Store API equivalents at :724/:735 — do not. So monitor mode, the new default, does not suppress this refusal and the merchant has no safety net; a legitimate repeat buyer is turned away at checkout on a plugin that says it is blocking nothing.

Scoping note so the fix is not misattributed: the already-filed #55 UTC/site-local split narrows blast radius by accident. created_at is current_time('mysql') (:220) compared against a gmdate threshold (:288), so on UTC-negative sites no row matches and the check is inert, while on UTC+2 the effective window is 3 hours rather than 1. The false positive therefore lands on UTC>=0 sites — UK/EU/AU, exactly where BACS and COD dominate — and becomes universal once #55 is fixed. The fix below is independent of #55.

Rejected alternative fix: narrowing the read side to status IN ('failed','declined'). That would gut the check on card-gateway stores too, because many gateways decline without ever setting the order to 'failed', so those rows legitimately stay 'attempt' and are the check's main signal. The defect is on the write side — 'attempt' means "unresolved", not "failed", and nothing ever resolves it for an on-hold gateway.

Fix

Broaden the write side so an accepted order is recorded as such regardless of gateway. Two edits, both in includes/class-webdecoy-woocommerce.php.

1. includes/class-webdecoy-woocommerce.php:239 — key the UPDATE on order_id alone, so it still matches when the status transition happens in a later request (admin marking a BACS order paid) under a different client IP. order_id already identifies the row uniquely, one row per order from track_attempt().

   "UPDATE {$table} SET status = 'success' WHERE order_id = %d AND status = 'attempt'",
   $order_id

(drop the ip_address = %s AND predicate and the $ip argument; $ip at :234 then becomes unused.)

2. includes/class-webdecoy-woocommerce.php:661-670 — register the same handler on the transitions that mean "WooCommerce accepted this order", not just payment_complete. 'on-hold' is required or BACS and Cheque are still missed.

   // A gateway that never calls payment_complete() — Cash on delivery, Direct
   // bank transfer, Cheque, and anything else that parks the order awaiting
   // payment — still produced a real, accepted order. Without these hooks the
   // attempt row stays 'attempt' for ever, so every completed order counts
   // against the shopper's velocity and card-testing budget on those stores.
   // Refs Card-testing detection counts successful orders: three paid sub-$5 orders in one hour block the address sitewide #52.
   $wd_track_accepted = function ($order_id) {
   $options = get_option('webdecoy_options', []);
   if (empty($options['protect_checkout'])) {
   return;
   }
   (new WebDecoy_WooCommerce($options))->track_payment((int) $order_id);
   };
   add_action('woocommerce_payment_complete', $wd_track_accepted);
   add_action('woocommerce_order_status_processing', $wd_track_accepted);
   add_action('woocommerce_order_status_on-hold', $wd_track_accepted);
   add_action('woocommerce_order_status_completed', $wd_track_accepted);

Ordering is safe: woocommerce_checkout_order_processed (which inserts the row) fires before process_order_payment(), so the COD/BACS update_status() call in the same request flips the row to 'success' before the next checkout reads it. webdecoy.php:850 also hooks payment_complete -> track_payment; the duplicate run is a no-op because the row is no longer 'attempt'.

Counting an 'on-hold' order as accepted does exclude a fraud-review hold from carding detection. On a safety release that is the correct direction to err, and it is the only way to cover BACS/Cheque.

If this is deferred instead of fixed, the two changelog.txt and readme.txt bullets claiming "Successful/Completed orders no longer count toward card-testing patterns or the checkout velocity limit" must be qualified before publishing, because they are false on any store whose gateways do not call payment_complete().

---

Blocks the 2.3.2 release. Refs #54.