[3.0] Fixes password reminders never validating - #9380
Open
albertlast wants to merge 1 commit into
Open
Conversation
Expiring validation codes changed the stored value to 'code|time', but neither half of that landed correctly. setPassword2() destructured the value with list(), and validation_code is a string, not an array. list() on a string yields nulls, so $real_code was never the submitted code and $issue_time collapsed to 0, which made the expiry check true every time. Every reminder was refused, and each refusal burned a validatePasswordFlood() strike on the way out. The column had no room for it either. members.validation_code is 10 characters, and 'code|time' is 21, so saving it was a hard error under MySQL's strict mode rather than a truncation: the reminder died before the mail was ever queued. Splits the value on the separator instead, padding so a code stored before this change still parses, and widens the column to 32. Fresh installs pick the width up from the schema, but Table::normalize() cannot help an existing forum, because Table::exists() compares a database-qualified prefix against the bare names list_tables() returns and so never matches, leaving normalize() to return early. Adds a migration to cover upgrades. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
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.
Description
Password reset by emailed link is broken on
release-3.0. On MySQL's strict mode it failsbefore the mail is even queued, with
Data too long for column 'validation_code' at row 1and a "Database Error" page; if it gets past that, the code is refused anyway.
Both faults come from
ec7a781d2"Expire validation codes", which changed the stored valueto
code|time:Reminder::setPassword2()doeslist($real_code, $issue_time) = $this->member->validation_code;, butvalidation_codeis a
string(User::$validation_code), not an array.list()on a string yieldsnulls, so$real_codeis never the submitted code and$issue_timefalls back to0— which makes
$issue_time + 3600 < time()true on every attempt. Each refusal alsoburns a
validatePasswordFlood()strike. The same commit's change toLogin2is fine;only
Reminderis affected.members.validation_codeisvarchar(10), andcode|timeis 21 characters (10 hex + separator + 10 digits).Nothing else is affected:
Security::generateValidationCode()returns exactly 10 chars, andActivate,Register2andAdmin\Membersall store the bare code, which still fits.The fix splits on the separator (padded, so a code stored before this change still parses
rather than warning) and widens the column to 32.
On the migration. Fresh installs pick the width up from the schema class, but existing
forums need help:
Table::normalize()opens withif (!$this->exists()), andTable::exists()comparesDb::$db->prefix . $nameagainstDb::$db->list_tables(). Theprefix is database-qualified (
`smf`.smf_) whilelist_tables()returns baresmf_members, so it never matches andnormalize()returns early without alteringanything. Hence the explicit migration rather than relying on normalize.
Testing. Verified on both engines in the Docker environment — fresh install, request a
reminder, follow the real emailed link, set a new password, and log in with it. Also
confirmed a wrong code is refused, a code backdated past an hour is refused (so the expiry
the original commit wanted now actually works), and that the migration takes an existing
install from
varchar(10)tovarchar(32)on MySQL and PostgreSQL.Issues References (Fixes|Related|Closes)