Skip to content

[3.0] Fixes password reminders never validating - #9380

Open
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:3.0/fix-reminder-validation-code
Open

[3.0] Fixes password reminders never validating#9380
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:3.0/fix-reminder-validation-code

Conversation

@albertlast

Copy link
Copy Markdown
Collaborator

Description

Password reset by emailed link is broken on release-3.0. On MySQL's strict mode it fails
before the mail is even queued, with Data too long for column 'validation_code' at row 1
and 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 value
to code|time:

  1. The value is never parsed. Reminder::setPassword2() does
    list($real_code, $issue_time) = $this->member->validation_code;, but validation_code
    is a string (User::$validation_code), not an array. list() on a string yields
    nulls, so $real_code is never the submitted code and $issue_time falls back to 0
    — which makes $issue_time + 3600 < time() true on every attempt. Each refusal also
    burns a validatePasswordFlood() strike. The same commit's change to Login2 is fine;
    only Reminder is affected.
  2. The column is too small. members.validation_code is varchar(10), and
    code|time is 21 characters (10 hex + separator + 10 digits).

Nothing else is affected: Security::generateValidationCode() returns exactly 10 chars, and
Activate, Register2 and Admin\Members all 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 with if (!$this->exists()), and
Table::exists() compares Db::$db->prefix . $name against Db::$db->list_tables(). The
prefix is database-qualified (`smf`.smf_) while list_tables() returns bare
smf_members, so it never matches and normalize() returns early without altering
anything. 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) to varchar(32) on MySQL and PostgreSQL.

Issues References (Fixes|Related|Closes)

  1. Regression from ec7a781 ("Expire validation codes")

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant