Fix. SFW. User agents priority fixed. - #874
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (5.71%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## dev #874 +/- ##
============================================
+ Coverage 29.19% 30.00% +0.81%
- Complexity 6446 6539 +93
============================================
Files 287 289 +2
Lines 26054 26259 +205
============================================
+ Hits 7607 7880 +273
+ Misses 18447 18379 -68 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts how SpamFireWall (SFW) and related firewall modules account for User-Agent (UA) blacklist/whitelist results when deciding priorities and when to short-circuit further module checks.
Changes:
- Introduces UA match metadata (
ua,ua_id,ua_status) at theFirewalllevel and passes it into loaded modules. - Adds a UA lookup against the UA blacklist table (
APBCT_TBL_AC_UA_BL) and uses the result to influence whitelist short-circuiting inFirewall::run(). - Modifies AntiCrawler UA-deny results to raise priority via
is_personaland changes UA-blacklist handling to early-return.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/Cleantalk/Common/Firewall/FirewallModuleAbstract.php | Adds shared user_agent_data storage to firewall modules. |
| lib/Cleantalk/Common/Firewall/FirewallModule.php | Adds setter to inject user_agent_data into modules. |
| lib/Cleantalk/Common/Firewall.php | Fetches UA status from UA BL table and uses it during module execution / whitelisting logic. |
| lib/Cleantalk/ApbctWP/Firewall/AntiCrawler.php | Adjusts UA-check result priority and early-return behavior; updates result structure to optionally mark “personal”. |
Suppressed comments (1)
lib/Cleantalk/ApbctWP/Firewall/AntiCrawler.php:303
- This comment block is now misleading: the code sets
early_returntotrue, so it no longer "continues to cookie check". It also contains typos (e.g., "HardCode", "dined") that make the intent harder to understand.
// Blacklisted — record but continue to cookie check
// HardCode - write AC dined by UA as personal: blacklisted user-agent may be only personally
// `is_personal` makes priority bigger, but we don't have a personal flag in the AC module yet, so this fix is needed
$is_personal = true;
return array(
'entries' => array($this->makeResult($current_ip, 'DENY_ANTICRAWLER_UA', $is_personal)),
'early_return' => true,
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
lib/Cleantalk/ApbctWP/Firewall/AntiCrawler.php:301
- These comments are now inconsistent with the behavior (blacklisted UA triggers
early_return => true, so it does not continue to the cookie check), and there’s a typo (“dined” -> “denied”). Also$is_personalcan be inlined.
// Blacklisted — record but continue to cookie check
// HardCode - write AC dined by UA as personal: blacklisted user-agent may be only personally
// `is_personal` makes priority bigger, but we don't have a personal flag in the AC module yet, so this fix is needed
$is_personal = true;
return array(
lib/Cleantalk/Common/Firewall.php:170
- The whitelist early-exit condition will never trigger when no UA blacklist entry matched, because
getUserAgentData()always setsua_status(null when not found) and(int) nullbecomes0. That means whitelisted/trusted requests can continue through other modules and still be blocked unintentionally.
if (
$this->isWhitelisted($results) &&
( ! isset($this->user_agent_data['ua_status']) || (int) $this->user_agent_data['ua_status'] !== 0 )
) {
https://app.doboard.com/1/task/39195