Skip to content

[3.0] Improves handling of case sensitivity for email addresses - #9633

Merged
Sesquipedalian merged 24 commits into
SimpleMachines:release-3.0from
Sesquipedalian:3.0/email_address_ci
Sep 6, 2026
Merged

[3.0] Improves handling of case sensitivity for email addresses#9633
Sesquipedalian merged 24 commits into
SimpleMachines:release-3.0from
Sesquipedalian:3.0/email_address_ci

Conversation

@Sesquipedalian

Copy link
Copy Markdown
Member

Followup to #9596.

The main things this PR does:

  1. Adds a new SMF\EmailAddress class to facilitate non-destructive manipulation of Unicode characters and character case conversion in email addresses.
  2. Adds a new email_address_ci column to the members table. It holds the casefolded version of each member's real email address.
  3. Compares to the casefolded versions of email addresses to ensure uniqueness during registration, profile updates, etc.
  4. Uses the casefolded versions of email addresses for internal functions like searching, sorting, checking bans, etc.
  5. Retains each member's true email address in the email_address field.
  6. When sending email messages, uses SMF\EmailAddress::sendable() to normalize the domain part of recipient email addresses, thereby maximizing the deliverability of email addresses with internationalized domain names.

This PR also makes two small changes:

  1. Fixes an incorrect alias in Utils::$smcFunc. This change had nothing to do with the rest of this PR; I just noticed the mistake and tossed in a fix.
  2. Removes some obsolete platform and version specific logic regarding line endings in SMF\Mail::send(). Now that SMF's minimum PHP version is greater than PHP 8, we can use the same line endings in email messages regardless of platform.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Before PHP 8, the correct line break was platform dependent. Since PHP 8, the line break is always "\r\n".

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>

@albertlast albertlast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through this end to end and ran the new class against PHP 8.4 in a container. The design is good -- EmailAddress is the right shape for this, and {column_ci:} is used correctly in User::find(). Three things look like blockers, though, and the two that matter most are the same omission twice.

Blockers

  1. EmailAddress::__construct() fatals on a plain typo. idn_to_ascii() returns false for empty, over-long, or hyphen-flanked labels, and that goes straight into a string-typed property under strict_types=1. user@bar..com at registration, in the guest post email field, or as a mailto: in a post body is an uncaught TypeError, not a validation error.
  2. email_address_ci is never written for new members. Register2::registerMember() and the installer's admin insert both build the row by hand and so bypass User::saveBatch(), which is the only place that fills the column in. Every account created after this change has it empty, and the duplicate-email check in Register2 reads that column -- so the same address can be registered twice. Password reminder, activation, email bans and member search go the same way.
  3. NormalizeBannedEmailAddresses::getMax() queries the wrong table. It is the member migration's method, docblock and all, so the ban loop runs past the last id_ban, comes back with nothing, and dies on an empty {array_int:}. Every forum without email bans fails this upgrade step.

Smaller things, inline: the memberlist email search builds a parameter it never uses and folds nothing, so that search stays case-sensitive against a casefolded column; Security::checkBans() lost the ?? '' that guarded an uninitialised User::$email; and email ban patterns now accept % and _ as unescaped LIKE wildcards.

One question. User::find() dropped the moderate_forum guard on matching email addresses. I could not turn it into a live leak -- show_email still withholds the address, and PM.php re-checks the names it got back -- but find() is public API and the guard went away rather than being replaced. Intentional?

Tests. EmailAddress is exactly the "value object that parses or normalises a string" case that AGENTS.md calls testable: pure, no Db::$db, no User::$me, no output. A #[DataProvider] over valid, invalid, IDN, mixed-case and wildcard inputs would have caught the first blocker on its own, and would pin down the casefolded() / sendable() / __toString() distinction, which is subtle enough to be worth locking in.

Happy to open a PR against this branch with any of these if that is easier than applying them one at a time.

Comment thread Sources/EmailAddress.php Outdated
Comment thread Sources/Actions/Register2.php
Comment thread Sources/Maintenance/Tools/Install.php
Comment thread Sources/Maintenance/Migration/v3_0/NormalizeBannedEmailAddresses.php Outdated
Comment thread Sources/Actions/Memberlist.php Outdated
Comment thread Sources/Actions/Memberlist.php Outdated
Comment thread Sources/Security.php Outdated
Comment thread Sources/User.php
Comment thread Sources/Actions/Admin/Bans.php Outdated
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian

Copy link
Copy Markdown
Member Author

All flagged issues have been addressed.

@Sesquipedalian

Copy link
Copy Markdown
Member Author

Tests. EmailAddress is exactly the "value object that parses or normalises a string" case that AGENTS.md calls testable: pure, no Db::$db, no User::$me, no output. A #[DataProvider] over valid, invalid, IDN, mixed-case and wildcard inputs would have caught the first blocker on its own, and would pin down the casefolded() / sendable() / __toString() distinction, which is subtle enough to be worth locking in.

Happy to open a PR against this branch with any of these if that is easier than applying them one at a time.

By all means, please follow up on this PR will some unit tests.

@Sesquipedalian

Sesquipedalian commented Sep 6, 2026

Copy link
Copy Markdown
Member Author
  1. email_address_ci is never written for new members. Register2::registerMember() and the installer's admin insert both build the row by hand and so bypass User::saveBatch(), which is the only place that fills the column in. Every account created after this change has it empty, and the duplicate-email check in Register2 reads that column -- so the same address can be registered twice. Password reminder, activation, email bans and member search go the same way.

Not only that, but the admin's spoofdetector_name wasn't set by the installer either. Now the installer sets both spoofdetector_name and email_address_ci for the admin.

@Sesquipedalian
Sesquipedalian merged commit 684ea57 into SimpleMachines:release-3.0 Sep 6, 2026
9 checks passed
@Sesquipedalian
Sesquipedalian deleted the 3.0/email_address_ci branch September 6, 2026 00:43
@jdarwood007 jdarwood007 added this to the 3.0 Alpha 5 milestone Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants