Fix antispam honeypot precedence bug (and/or vs && ||)#218
Open
HomesONE wants to merge 1 commit into
Open
Conversation
In `AntiSpam::check_user()` the two flag assignments used PHP's
low-precedence `and`/`or` operators, which bind *less* tightly
than `=` — so what reads as
$in_period = !empty($result) and time() < $result['no_query_period'];
actually evaluates as
($in_period = !empty($result)) and (time() < $result['no_query_period']);
The right-hand expression is computed but discarded, and
`$in_period` ends up being just `!empty($result)` — the
`no_query_period` timestamp check is silently dropped, so stale
rows keep returning "still in period" past their expiry until
`clean_no_query_periods()` evicts them on the next run.
$obvious_spam = !isset($_POST['dontlook']) or !empty($_POST['dontlook']);
likewise becomes `$obvious_spam = !isset($_POST['dontlook'])`,
so the honeypot fires when the hidden field is *missing* (false
for the bundled form) and never when a bot fills it — the
intended `!empty($_POST['dontlook'])` test is discarded.
Replace with `&&`/`||` and parenthesize so the intended semantics
hold:
- `$in_period`: a row exists AND its no_query_period has not
elapsed.
- `$obvious_spam`: the honeypot field is present AND non-empty.
No behavioural change for legitimate users (bundled form always
submits an empty dontlook field). Bots that fill the honeypot
now actually receive the long ban they were meant to receive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In
AntiSpam::check_user()(includes/antispam.php:126-127) two flag assignments use PHP's low-precedenceand/oroperators. Those bind less tightly than=, so:evaluates as
— the right-hand expression is computed but discarded.
$in_periodends up being just!empty($result), so theno_query_periodtimestamp check is silently dropped and stale rows keep returning "still in period" past their expiry (untilclean_no_query_periods()evicts them on the next run).The second line:
becomes
$obvious_spam = !isset($_POST['dontlook']). The actual honeypot test (!empty($_POST['dontlook'])) is also discarded. Practical effect:dontlookfield — the thing the honeypot is supposed to catch — gets$obvious_spam = false(becauseisset()is true →!isset()is false), so no penalty is applied.$obvious_spam = trueand gets thedegree = 512mega-ban — an unintended outcome.Fix
Use
&&/||(which bind tighter than=) and parenthesize for clarity:The
\$obvious_spamcondition is also inverted to express the actually-intended logic: "the honeypot field is present AND non-empty".Impact
dontlookfield, so\$obvious_spamstaysfalse(same as before, just via correct logic).Repro
Found via static review of v2.3.1. (Replaces #217, which I closed to recreate from a clean fork.)