fix(security): mail password resets to the stored admin address

AGENTS.md

@@ -1124,6 +1124,23 @@ The contract:
   lies to the user in the missed-branch case ("If that email is
   registered, a link has been sent" → it wasn't, but they don't
   get told), which is the intended behavior.
+- **Mail the stored address, never the request value.** After the
+  lookup matches, every downstream step keys on the stored row:
+  `Mail::send($row['email'], …)`, links built from `$row['email']`,
+  `UPDATE … WHERE aid = :aid`. `:prefix_admins.email` uses the
+  table's case- and accent-insensitive collation
+  (`utf8mb4_general_ci`), so the request value can differ from the
+  stored address and still match. The stored address is the only
+  one the account owner has proven they control. `email` is also
+  not unique, so never `UPDATE … WHERE email = :email` either: it
+  rolls every collation-equal duplicate.
+- **Compare secrets byte-exact in PHP, not via SQL.** The same
+  collation makes `WHERE validate = :token` case-insensitive.
+  Look the row up by its identifier, then
+  `hash_equals((string) $row['validate'], (string) $token)`, and
+  treat a NULL / empty stored secret as never matching. Reference
+  shape: the `?email=…&validation=…` branch in
+  `web/pages/page.lostpassword.php`.
 
 **Caveat: response-time differential remains.** The matched branch
 performs a DB write + an SMTP round-trip; the unmatched branch
@@ -1147,7 +1164,15 @@ the canonical "byte-for-byte identical" assertion) +
 `web/tests/api/__snapshots__/auth/lost_password_generic.json` (the
 locked wire-format) + `web/tests/e2e/specs/flows/lostpassword-toast.spec.ts`
 (the chrome-side parity test — same painted toast for known +
-unknown emails).
+unknown emails). The stored-address / byte-exact rules are pinned by
+`AuthTest::testLostPasswordMailsOnlyTheStoredAddressForCollationVariants`
+(case- and accent-insensitive request values; recipients captured via
+the test-only `Sbpp\Mail\Mailer::overrideTransportForTests()` seam)
++ `testLostPasswordRollsTokenOnlyOnTheMatchedAidWhenEmailsCollide`,
+and on the page side by the "Collation-variant email" + "Token with
+different letter case" tests in `lostpassword-toast.spec.ts`
+(mailpit-backed, per-run admin rows via
+`seedLostpasswordE2e(email)`).
 
 **`api_auth_login` (#59):** the empty-password check runs **before**
 the user lookup, so `{username: 'admin', password: ''}` and

CHANGELOG.md

@@ -10,6 +10,15 @@ Legend:
 ? = Other stuff
 ```
 
+(26/10/26): Version 2.2.1-srcdslab
+-----------------------
+```
+01. ! Security: password reset mails go to the address stored on the
+        admin row, never to a case- or accent-variant of it typed in
+        the form; the reset token is rolled by admin id
+02. ! Security: the reset token is compared byte-exact (hash_equals)
+```
+
 (22/10/26): Version 2.1.26-srcdslab
 -----------------------
 ```

web/api/handlers/auth.php

@@ -175,7 +175,11 @@ function api_auth_lost_password(array $params): array
 
     $email = (string)($params['email'] ?? '');
 
-    $GLOBALS['PDO']->query("SELECT aid, user FROM `:prefix_admins` WHERE email = :email");
+    // `email` uses the table's case- and accent-insensitive collation, so
+    // the request value can differ from the stored address. Everything
+    // past the lookup keys on the stored row (`aid`, `email`), never on
+    // the request string.
+    $GLOBALS['PDO']->query("SELECT aid, user, email FROM `:prefix_admins` WHERE email = :email");
     $GLOBALS['PDO']->bind(':email', $email);
     $row = $GLOBALS['PDO']->single();
 
@@ -191,16 +195,17 @@ function api_auth_lost_password(array $params): array
         return _api_auth_lost_password_generic_response();
     }
 
-    $validation = Crypto::recoveryHash();
-    $GLOBALS['PDO']->query("UPDATE `:prefix_admins` SET validate = :validate WHERE email = :email");
+    $storedEmail = (string) $row['email'];
+    $validation  = Crypto::recoveryHash();
+    $GLOBALS['PDO']->query("UPDATE `:prefix_admins` SET validate = :validate WHERE aid = :aid");
     $GLOBALS['PDO']->bind(':validate', $validation);
-    $GLOBALS['PDO']->bind(':email', $email);
+    $GLOBALS['PDO']->bind(':aid', (int) $row['aid']);
     $GLOBALS['PDO']->execute();
 
-    $url = Host::panelUrl() . '/index.php?p=lostpassword&email=' . urlencode($email)
+    $url = Host::panelUrl() . '/index.php?p=lostpassword&email=' . urlencode($storedEmail)
         . '&validation=' . urlencode($validation);
 
-    $sent = Mail::send($email, EmailType::PasswordReset, [
+    $sent = Mail::send($storedEmail, EmailType::PasswordReset, [
         '{link}' => $url,
         '{name}' => $row['user'],
         '{home}' => Host::panelUrl(),

web/includes/Mail/Mailer.php

@@ -13,6 +13,7 @@
 use Sbpp\Log;
 use Symfony\Component\Mailer\Exception\TransportExceptionInterface;
 use Symfony\Component\Mailer\Transport;
+use Symfony\Component\Mailer\Transport\TransportInterface;
 use Symfony\Component\Mime\Email;
 
 final class Mailer
@@ -23,6 +24,9 @@ final class Mailer
     /** Latched so we only emit one deprecation warning per process when the legacy SB_EMAIL fallback is used. */
     private static bool $sbEmailDeprecationLogged = false;
 
+    /** Test-only replacement for the SMTP transport {@see send()} builds from the configured DSN. */
+    private static ?TransportInterface $transportOverride = null;
+
     public function __construct(
         private readonly string $host,
         private readonly string $user,
@@ -52,7 +56,7 @@ public function send(array|string $destination,
         if(!$this->verifyPeer)
             $dsn .= '?verify_peer=false';
 
-        $mailer = Transport::fromDsn($dsn);
+        $mailer = self::$transportOverride ?? Transport::fromDsn($dsn);
 
         $mail = $this->buildMessage($destination, $subject, $body, $files);
 
@@ -169,6 +173,17 @@ public static function resetDeprecationLatch(): void
         self::$sbEmailDeprecationLogged = false;
     }
 
+    /**
+     * Test-only seam: route every {@see send()} through `$transport`
+     * (e.g. a capturing `AbstractTransport`) instead of SMTP, so tests
+     * can assert recipients without a live mail server. Pass `null` to
+     * restore the configured transport.
+     */
+    public static function overrideTransportForTests(?TransportInterface $transport): void
+    {
+        self::$transportOverride = $transport;
+    }
+
     private static function formatFrom(string $name, string $email): string
     {
         return sprintf('"%s" <%s>', addcslashes($name, '"\\'), $email);

web/pages/page.lostpassword.php

@@ -60,12 +60,24 @@
         PageDie();
     }
 
-    $GLOBALS['PDO']->query("SELECT aid, user FROM `:prefix_admins` WHERE `email` = :email AND `validate` = :validate");
+    // Both columns use a case- and accent-insensitive collation, so SQL
+    // only narrows by email: the token is checked byte-exact here, and
+    // the new password goes to the stored address, never the
+    // collation-equal request value.
+    $GLOBALS['PDO']->query("SELECT aid, user, email, validate FROM `:prefix_admins` WHERE `email` = :email");
     $GLOBALS['PDO']->bind(':email', $email);
-    $GLOBALS['PDO']->bind(':validate', $validation);
-    $result = $GLOBALS['PDO']->single();
+    $candidates = $GLOBALS['PDO']->resultset();
 
-    if (empty($result['aid'])) {
+    $result = null;
+    foreach ($candidates as $candidate) {
+        $storedToken = (string) $candidate['validate'];
+        if ($storedToken !== '' && hash_equals($storedToken, (string) $validation)) {
+            $result = $candidate;
+            break;
+        }
+    }
+
+    if ($result === null) {
         \Sbpp\View\Toast::emit('error', 'Error', 'The validation string does not match the email for this reset request.');
         PageDie();
     }
@@ -83,7 +95,7 @@
     // surface the error otherwise so the user can retry once SMTP is
     // configured.
     $password    = Crypto::genSecret(MIN_PASS_LENGTH + 8);
-    $isEmailSent = Mail::send($email, EmailType::PasswordResetSuccess, [
+    $isEmailSent = Mail::send((string) $result['email'], EmailType::PasswordResetSuccess, [
         '{password}' => $password,
         '{name}'     => $result['user'],
         '{home}'     => Host::panelUrl()

web/tests/api/AuthTest.php

@@ -2,11 +2,20 @@
 
 namespace Sbpp\Tests\Api;
 
+use PHPUnit\Framework\Attributes\DataProvider;
+use Sbpp\Mail\Mailer;
 use Sbpp\Tests\ApiTestCase;
 use Sbpp\Tests\Fixture;
+use Symfony\Component\Mailer\SentMessage;
+use Symfony\Component\Mailer\Transport\AbstractTransport;
+use Symfony\Component\Mime\Address;
+use Symfony\Component\Mime\Email;
 
 final class AuthTest extends ApiTestCase
 {
+    /** @var list<SentMessage> Messages recorded by {@see captureMail()}. */
+    private array $sentMail = [];
+
     /**
      * #1456 — privacy fix. The pre-fix handler threw
      * `ApiError('not_registered', …)` on the miss branch and let any
@@ -207,6 +216,166 @@ public function testLostPasswordReturnsDisabledEnvelopeUniformlyWhenNormalLoginI
         }
     }
 
+    /**
+     * `:prefix_admins.email` uses a case- and accent-insensitive
+     * collation, so a request value that differs in case or accents
+     * matches the stored row. The reset link must still go ONLY to the
+     * stored address.
+     *
+     * @return iterable<string, array{0: string, 1: string}>
+     */
+    public static function collationVariantEmails(): iterable
+    {
+        yield 'upper-case'     => ['admin@example.test', 'ADMIN@EXAMPLE.TEST'];
+        yield 'accent variant' => ['owner@example.test', 'ówner@example.test'];
+    }
+
+    #[DataProvider('collationVariantEmails')]
+    public function testLostPasswordMailsOnlyTheStoredAddressForCollationVariants(string $stored, string $requested): void
+    {
+        $ownerAid  = $this->seedAdmin('owner', 'owner@example.test');
+        $targetAid = $stored === 'admin@example.test' ? Fixture::adminAid() : $ownerAid;
+        $otherAid  = $targetAid === $ownerAid ? Fixture::adminAid() : $ownerAid;
+        $this->assertSame(1, $this->countAdminsByEmail($requested),
+            'precondition: the request value must be collation-equal to exactly the stored row');
+
+        $this->captureMail();
+        $env = $this->api('auth.lost_password', ['email' => $requested]);
+
+        $this->assertTrue($env['ok'] ?? false, 'expected ok envelope: ' . json_encode($env));
+        $this->assertSnapshot('auth/lost_password_generic', $env);
+
+        $this->assertCount(1, $this->sentMail, 'exactly one reset mail must be sent');
+        $this->assertSame([$stored], $this->recipients($this->sentMail[0]));
+
+        $target = $this->row('admins', ['aid' => $targetAid]);
+        $this->assertNotEmpty($target['validate'] ?? null, 'the matched admin\'s token must be rolled');
+        $this->assertNull($this->row('admins', ['aid' => $otherAid])['validate'] ?? null,
+            'no other admin\'s token may be touched');
+
+        $body = $this->htmlBody($this->sentMail[0]);
+        $this->assertStringContainsString(
+            'email=' . urlencode($stored) . '&validation=' . urlencode((string) $target['validate']),
+            $body,
+            'the reset link must carry the stored address and the freshly rolled token',
+        );
+        $this->assertStringNotContainsString('email=' . urlencode($requested) . '&', $body);
+    }
+
+    /**
+     * `email` is not unique, and collation-equal duplicates are easy to
+     * end up with. Rolling `validate` by email would reset every
+     * duplicate's token (invalidating their in-flight links) while only
+     * one of them gets a mail; the handler keys the UPDATE on `aid`.
+     */
+    public function testLostPasswordRollsTokenOnlyOnTheMatchedAidWhenEmailsCollide(): void
+    {
+        $firstAid  = $this->seedAdmin('dup-one', 'dup@example.test');
+        $secondAid = $this->seedAdmin('dup-two', 'DUP@example.test');
+
+        $this->captureMail();
+        $this->api('auth.lost_password', ['email' => 'Dup@Example.Test']);
+
+        $rolled = array_values(array_filter(
+            [$this->row('admins', ['aid' => $firstAid]), $this->row('admins', ['aid' => $secondAid])],
+            static fn (?array $r): bool => !empty($r['validate']),
+        ));
+
+        $this->assertCount(1, $rolled, 'exactly one of the collation-equal admins may have its token rolled');
+        $this->assertCount(1, $this->sentMail);
+        $this->assertSame([(string) $rolled[0]['email']], $this->recipients($this->sentMail[0]),
+            'the mail must go to the stored address of the row whose token was rolled');
+    }
+
+    protected function tearDown(): void
+    {
+        Mailer::overrideTransportForTests(null);
+        parent::tearDown();
+    }
+
+    private function seedAdmin(string $user, string $email): int
+    {
+        $pdo  = Fixture::rawPdo();
+        $stmt = $pdo->prepare(sprintf(
+            'INSERT INTO `%s_admins` (user, authid, password, gid, email, validate, extraflags, immunity)
+             VALUES (?, \'\', ?, -1, ?, NULL, 0, 0)',
+            DB_PREFIX,
+        ));
+        $stmt->execute([$user, password_hash(bin2hex(random_bytes(8)), PASSWORD_BCRYPT), $email]);
+        return (int) $pdo->lastInsertId();
+    }
+
+    private function countAdminsByEmail(string $email): int
+    {
+        $stmt = Fixture::rawPdo()->prepare(sprintf(
+            'SELECT COUNT(*) FROM `%s_admins` WHERE email = ?',
+            DB_PREFIX,
+        ));
+        $stmt->execute([$email]);
+        return (int) $stmt->fetchColumn();
+    }
+
+    /**
+     * Configure SMTP just enough for `Mailer::create()` to build a
+     * mailer, and swap the transport for one that appends every
+     * message to `$this->sentMail` instead of opening a socket.
+     */
+    private function captureMail(): void
+    {
+        $stmt = Fixture::rawPdo()->prepare(sprintf(
+            'REPLACE INTO `%s_settings` (`setting`, `value`) VALUES (?, ?)',
+            DB_PREFIX,
+        ));
+        foreach ([
+            'smtp.host'              => 'smtp.invalid',
+            'smtp.user'              => 'user',
+            'smtp.pass'              => 'pass',
+            'config.mail.from_email' => 'noreply@example.test',
+        ] as $key => $value) {
+            $stmt->execute([$key, $value]);
+        }
+        \Config::init($GLOBALS['PDO']);
+
+        $this->sentMail = [];
+        $record = function (SentMessage $message): void {
+            $this->sentMail[] = $message;
+        };
+        Mailer::overrideTransportForTests(new class ($record) extends AbstractTransport {
+            public function __construct(private readonly \Closure $record)
+            {
+                parent::__construct();
+            }
+
+            protected function doSend(SentMessage $message): void
+            {
+                ($this->record)($message);
+            }
+
+            public function __toString(): string
+            {
+                return 'capture://';
+            }
+        });
+    }
+
+    /** @return list<string> */
+    private function recipients(SentMessage $message): array
+    {
+        return array_map(
+            static fn (Address $a): string => $a->getAddress(),
+            $message->getEnvelope()->getRecipients(),
+        );
+    }
+
+    private function htmlBody(SentMessage $message): string
+    {
+        $email = $message->getOriginalMessage();
+        $this->assertInstanceOf(Email::class, $email);
+        // Template values are HTML-escaped (`&` → `&amp;`), so decode
+        // before matching the reset link's query string.
+        return html_entity_decode((string) $email->getHtmlBody(), ENT_QUOTES | ENT_HTML5, 'UTF-8');
+    }
+
     /**
      * Helper: count the number of `:prefix_log` rows whose `title`
      * column matches the given literal. Used by

web/tests/e2e/fixtures/db.ts

@@ -285,13 +285,19 @@ export interface LostpasswordSeed {
  * single source of truth for which settings the happy-path requires
  * (kept out of the TS side so the schema-shape of `:prefix_settings`
  * stays Python-style invisible to the spec author).
+ *
+ * Pass `email` to seed (and target) a dedicated admin row for that
+ * address instead of the default `lostpw-happy@example.test`.
  */
-export async function seedLostpasswordE2e(): Promise<LostpasswordSeed> {
+export async function seedLostpasswordE2e(email?: string): Promise<LostpasswordSeed> {
     const inContainer = process.env.E2E_IN_CONTAINER === '1';
     const cmd = inContainer ? 'php' : 'docker';
-    const cmdArgs = inContainer
+    const scriptArgs = email === undefined
         ? [SEED_LOSTPASSWORD_INSIDE_CONTAINER]
-        : ['compose', 'exec', '-T', 'web', 'php', SEED_LOSTPASSWORD_INSIDE_CONTAINER];
+        : [SEED_LOSTPASSWORD_INSIDE_CONTAINER, email];
+    const cmdArgs = inContainer
+        ? scriptArgs
+        : ['compose', 'exec', '-T', 'web', 'php', ...scriptArgs];
 
     const { stdout, stderr } = await execFileP(cmd, cmdArgs, {
         maxBuffer: 1 * 1024 * 1024,