[3.0] Improves handling of case sensitivity for email addresses - #9633
Conversation
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
left a comment
There was a problem hiding this comment.
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
EmailAddress::__construct()fatals on a plain typo.idn_to_ascii()returnsfalsefor empty, over-long, or hyphen-flanked labels, and that goes straight into astring-typed property understrict_types=1.user@bar..comat registration, in the guest post email field, or as amailto:in a post body is an uncaughtTypeError, not a validation error.email_address_ciis never written for new members.Register2::registerMember()and the installer's admin insert both build the row by hand and so bypassUser::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 inRegister2reads that column -- so the same address can be registered twice. Password reminder, activation, email bans and member search go the same way.NormalizeBannedEmailAddresses::getMax()queries the wrong table. It is the member migration's method, docblock and all, so the ban loop runs past the lastid_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.
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>
|
All flagged issues have been addressed. |
By all means, please follow up on this PR will some unit tests. |
Not only that, but the admin's |
Followup to #9596.
The main things this PR does:
email_address_cicolumn to the members table. It holds the casefolded version of each member's real email address.email_addressfield.This PR also makes two small changes: