From d601c52b42d081d4b60a643c8180390ae94c4832 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Wed, 8 Oct 2025 16:46:37 +0600 Subject: [PATCH 1/4] feat: restrict gmail misuse as recoveryEmail gmail address doesn't care (.) in it. Spammer use this to bypass recoveryEmail restriction in our system. Again gmail has 2 domains (gmail.com, googlemail.com) which target same emailAccount. So, we retrieve all possible already saved same gmail accounts from our DB using regex & restrict same gmail is being used as multiple accounts' recoveryEmail. Other changes are: - fix Alias max counter is actually letting 1 extra account. - improve DB query by using count instead of retrieving fields & then count them. issue: https://gitlab.e.foundation/e/infra/backlog/-/issues/4735 --- lib/Db/ConfigMapper.php | 41 ++++++++-- lib/Service/RecoveryEmailService.php | 109 +++++++++++++++++++++++++-- 2 files changed, 136 insertions(+), 14 deletions(-) diff --git a/lib/Db/ConfigMapper.php b/lib/Db/ConfigMapper.php index b7f3484..715770a 100644 --- a/lib/Db/ConfigMapper.php +++ b/lib/Db/ConfigMapper.php @@ -15,19 +15,46 @@ class ConfigMapper { } - public function getUsersByRecoveryEmail(string $pattern) : array { + public function countRecoveryEmail(string $pattern): int { $pattern = strtolower($pattern); $qb = $this->connection->getQueryBuilder(); - $qb->select('userid') + $qb->select($qb->func()->count('userid')) ->from("preferences")->where('LOWER(`configvalue`) like :pattern AND (`configkey` = "unverified-recovery-email" OR `configkey` = "recovery-email") AND `appid` = :appname ') ->setParameter('pattern', $pattern) ->setParameter('appname', $this->appName); - $result = $qb->execute(); - $userIDs = []; - while ($row = $result->fetch()) { - $userIDs[] = $row['userid']; + + $result = $qb->executeQuery(); + $count = $result->fetchOne(); + $result->closeCursor(); + + if ($count !== false) { + $count = (int)$count; + } else { + $count = 0; + } + + return $count; + } + + public function countRecoveryEmailByRegex(string $pattern): int { + $pattern = strtolower($pattern); + $qb = $this->connection->getQueryBuilder(); + $qb->select($qb->func()->count('userid')) + ->from("preferences")->where('LOWER(`configvalue`) REGEXP :pattern AND (`configkey` = "unverified-recovery-email" OR `configkey` = "recovery-email") AND `appid` = :appname ') + ->setParameter('pattern', $pattern) + ->setParameter('appname', $this->appName); + + $result = $qb->executeQuery(); + $count = $result->fetchOne(); + $result->closeCursor(); + + if ($count !== false) { + $count = (int)$count; + } else { + $count = 0; } - return $userIDs; + + return $count; } public function getAllVerifiedRecoveryEmails(): array { diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index 69a7e37..cb4bd7a 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -52,6 +52,8 @@ class RecoveryEmailService { private const RATE_LIMIT_DOMAIN = 'verifymail_domain_ratelimit'; private const RECOVERY_EMAIL_REMINDER_START_DATE = 'recovery-email-reminder-start-date'; private const UNVERIFIED_USER_DISABLE_AT = 'unverified-user-disabled-at'; + private const GMAIL_QUERY_LAST_PART = "(@gmail.com|@googlemail.com)"; //regex: is the str ends with any gmail's domains + private const GMAIL_QUERY_MIDDLE_PART = "\+[^@]+"; //regex: for alias email addresses, ignore anything after + private $cache; @@ -190,6 +192,11 @@ class RecoveryEmailService { throw new RecoveryEmailAlreadyFoundException($l->t('Recovery email address is already taken.')); } + if ($this->isGmailRecoveryEmailTaken($username, $recoveryEmail)) { + $this->logger->info("User ID $username's requested recovery email (gmail) address is already taken"); + throw new RecoveryEmailAlreadyFoundException($l->t('Recovery email address is already taken.')); + } + if (!$this->isAliasedRecoveryEmailValid($username, $recoveryEmail)) { $this->logger->info("User ID $username's requested recovery extended email address is already taken"); throw new RecoveryEmailAlreadyFoundException($l->t('This email address in invalid, please use another one.')); @@ -355,11 +362,12 @@ class RecoveryEmailService { return false; } - private function getUsernameAndDomain(string $email) : ?array { + private function getUsernameAndDomain(?string $email) : ?array { if ($email === null || empty($email)) { return null; } $email = strtolower($email); + $email = trim($email); $emailParts = explode('@', $email); $mailUsernameParts = explode('+', $emailParts[0]); $mailUsername = $mailUsernameParts[0]; @@ -367,6 +375,91 @@ class RecoveryEmailService { return [$mailUsername, $mailDomain]; } + /* + * if the provided recoveryMail is gmail check if this is already taken or not. + * return false if the recoveryMail is not gmail / alias mail / not taken + */ + private function isGmailRecoveryEmailTaken(string $userName, string $recoveryEmail): bool { + // if alias found in the mail address, ignore for now, it will be handle in the next section of the caller + if (str_contains($recoveryEmail, '+')) { + return false; + } + + $mailPrefix = $this->getGmailUserName($recoveryEmail); + // provided recoveryEmail is not gmail. + if ($mailPrefix === null) { + return false; + } + + if ($this->isRecoveryGmailBelongsToUser($userName, $mailPrefix)) { + return false; + } + + $regex = $this->getGmailQueryRegexFirstPart($mailPrefix); + $regex = $regex . self::GMAIL_QUERY_LAST_PART; + + $usersWithEmailRecovery = $this->configMapper->countRecoveryEmailByRegex($regex); + return $usersWithEmailRecovery !== 0; + } + + /** + * if provided user already has same gmail account setup as recoveryEmail / unverifiedRecoveryEmail, + * then we assume that is already passed verification once. + * So no need verfication run again. + */ + private function isRecoveryGmailBelongsToUser(string $userName, string $mailPrefix): bool { + $currentRecoveryEmail = $this->getRecoveryEmail($userName); + $currentUnverifiedRecoveryEmail = $this->getUnverifiedRecoveryEmail($userName); + $currentRecoveryEmailUserPart = $this->getGmailUserName($currentRecoveryEmail); + $currentUnverifiedRecoveryEmailUserPart = $this->getGmailUserName($currentUnverifiedRecoveryEmail); + + // requested recoveryEmail is already set by the current user + return (($mailPrefix === $currentRecoveryEmailUserPart) || ($mailPrefix === $currentUnverifiedRecoveryEmailUserPart)); + } + + /** + * return sanitized mailPrefix if provided mail address is gmail + * gmail doesn't care about (.) in the mail address. + */ + private function getGmailUserName(?string $email): ?string { + $emailParts = $this->getUsernameAndDomain($email); + $gmailDomains = ['gmail.com', 'googlemail.com']; + + if ($emailParts == null || !in_array($emailParts[1], $gmailDomains)) { + return null; + } + + return str_replace(".", "", $emailParts[0]); + } + + /** + * gmail doesn't care about (.) in the mail address. + * So, to retrieve all possible entries using regex from Database, we need to modify the mail prefix + * ex: if the mail address is abc@gmail.com, then the mailPrefix is abc. So, this method will return ^a[.]?b[.]?c[.]? + */ + private function getGmailQueryRegexFirstPart(string $mailPrefix): string { + $regex = "^"; + + foreach (str_split($mailPrefix) as $c) { + $regex = $regex . $c . "[.]?"; + } + + return $regex; + } + + private function isGmailAliasedRecoveryEmailValid(string $userName, string $mailPrefix, int $emailAliasLimit): bool { + if ($this->isRecoveryGmailBelongsToUser($userName, $mailPrefix)) { + return true; + } + + $regex = $this->getGmailQueryRegexFirstPart($mailPrefix); + $regex = $regex . self::GMAIL_QUERY_MIDDLE_PART; + $regex = $regex . self::GMAIL_QUERY_LAST_PART; + + $usersWithEmailRecovery = $this->configMapper->countRecoveryEmailByRegex($regex); + return $usersWithEmailRecovery < $emailAliasLimit; + } + public function isAliasedRecoveryEmailValid(string $username, string $recoveryEmail): bool { if (!str_contains($recoveryEmail, '+')) { return true; @@ -376,6 +469,12 @@ class RecoveryEmailService { if ($emailAliasLimit === -1) { return true; } + + $gmailPrefix = $this->getGmailUserName($recoveryEmail); + if ($gmailPrefix !== null) { + return $this->isGmailAliasedRecoveryEmailValid($username, $gmailPrefix, $emailAliasLimit); + } + $recoveryEmailregex = $recoveryEmailParts[0]."+%@".$recoveryEmailParts[1]; $currentRecoveryEmail = $this->getRecoveryEmail($username); $currentUnverifiedRecoveryEmail = $this->getUnverifiedRecoveryEmail($username); @@ -387,12 +486,8 @@ class RecoveryEmailService { return true; } - $usersWithEmailRecovery = $this->configMapper->getUsersByRecoveryEmail($recoveryEmailregex); - if (count($usersWithEmailRecovery) > $emailAliasLimit) { - return false; - } - - return true; + $usersWithEmailRecovery = $this->configMapper->countRecoveryEmail($recoveryEmailregex); + return $usersWithEmailRecovery < $emailAliasLimit; } public function updateRecoveryEmail(string $username, string $recoveryEmail) : void { -- GitLab From 0233cfab9f5ede002d465f289b3899bb211aeb9a Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Fri, 10 Oct 2025 15:36:21 +0600 Subject: [PATCH 2/4] chore: update lint according to review --- lib/Service/RecoveryEmailService.php | 36 ++++++++++++++-------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index cb4bd7a..08a6eda 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -379,27 +379,27 @@ class RecoveryEmailService { * if the provided recoveryMail is gmail check if this is already taken or not. * return false if the recoveryMail is not gmail / alias mail / not taken */ - private function isGmailRecoveryEmailTaken(string $userName, string $recoveryEmail): bool { + private function isGmailRecoveryEmailTaken(string $username, string $recoveryEmail): bool { // if alias found in the mail address, ignore for now, it will be handle in the next section of the caller if (str_contains($recoveryEmail, '+')) { return false; } - $mailPrefix = $this->getGmailUserName($recoveryEmail); + $mailPrefix = $this->getGmailUsername($recoveryEmail); // provided recoveryEmail is not gmail. if ($mailPrefix === null) { return false; } - if ($this->isRecoveryGmailBelongsToUser($userName, $mailPrefix)) { + if ($this->isRecoveryGmailBelongsToUser($username, $mailPrefix)) { return false; } $regex = $this->getGmailQueryRegexFirstPart($mailPrefix); $regex = $regex . self::GMAIL_QUERY_LAST_PART; - $usersWithEmailRecovery = $this->configMapper->countRecoveryEmailByRegex($regex); - return $usersWithEmailRecovery !== 0; + $recoveryEmailCount = $this->configMapper->countRecoveryEmailByRegex($regex); + return $recoveryEmailCount !== 0; } /** @@ -407,11 +407,11 @@ class RecoveryEmailService { * then we assume that is already passed verification once. * So no need verfication run again. */ - private function isRecoveryGmailBelongsToUser(string $userName, string $mailPrefix): bool { - $currentRecoveryEmail = $this->getRecoveryEmail($userName); - $currentUnverifiedRecoveryEmail = $this->getUnverifiedRecoveryEmail($userName); - $currentRecoveryEmailUserPart = $this->getGmailUserName($currentRecoveryEmail); - $currentUnverifiedRecoveryEmailUserPart = $this->getGmailUserName($currentUnverifiedRecoveryEmail); + private function isRecoveryGmailBelongsToUser(string $username, string $mailPrefix): bool { + $currentRecoveryEmail = $this->getRecoveryEmail($username); + $currentUnverifiedRecoveryEmail = $this->getUnverifiedRecoveryEmail($username); + $currentRecoveryEmailUserPart = $this->getGmailUsername($currentRecoveryEmail); + $currentUnverifiedRecoveryEmailUserPart = $this->getGmailUsername($currentUnverifiedRecoveryEmail); // requested recoveryEmail is already set by the current user return (($mailPrefix === $currentRecoveryEmailUserPart) || ($mailPrefix === $currentUnverifiedRecoveryEmailUserPart)); @@ -421,7 +421,7 @@ class RecoveryEmailService { * return sanitized mailPrefix if provided mail address is gmail * gmail doesn't care about (.) in the mail address. */ - private function getGmailUserName(?string $email): ?string { + private function getGmailUsername(?string $email): ?string { $emailParts = $this->getUsernameAndDomain($email); $gmailDomains = ['gmail.com', 'googlemail.com']; @@ -447,8 +447,8 @@ class RecoveryEmailService { return $regex; } - private function isGmailAliasedRecoveryEmailValid(string $userName, string $mailPrefix, int $emailAliasLimit): bool { - if ($this->isRecoveryGmailBelongsToUser($userName, $mailPrefix)) { + private function isGmailAliasedRecoveryEmailValid(string $username, string $mailPrefix, int $emailAliasLimit): bool { + if ($this->isRecoveryGmailBelongsToUser($username, $mailPrefix)) { return true; } @@ -456,8 +456,8 @@ class RecoveryEmailService { $regex = $regex . self::GMAIL_QUERY_MIDDLE_PART; $regex = $regex . self::GMAIL_QUERY_LAST_PART; - $usersWithEmailRecovery = $this->configMapper->countRecoveryEmailByRegex($regex); - return $usersWithEmailRecovery < $emailAliasLimit; + $recoveryEmailCount = $this->configMapper->countRecoveryEmailByRegex($regex); + return $recoveryEmailCount < $emailAliasLimit; } public function isAliasedRecoveryEmailValid(string $username, string $recoveryEmail): bool { @@ -470,7 +470,7 @@ class RecoveryEmailService { return true; } - $gmailPrefix = $this->getGmailUserName($recoveryEmail); + $gmailPrefix = $this->getGmailUsername($recoveryEmail); if ($gmailPrefix !== null) { return $this->isGmailAliasedRecoveryEmailValid($username, $gmailPrefix, $emailAliasLimit); } @@ -486,8 +486,8 @@ class RecoveryEmailService { return true; } - $usersWithEmailRecovery = $this->configMapper->countRecoveryEmail($recoveryEmailregex); - return $usersWithEmailRecovery < $emailAliasLimit; + $recoveryEmailCount = $this->configMapper->countRecoveryEmail($recoveryEmailregex); + return $recoveryEmailCount < $emailAliasLimit; } public function updateRecoveryEmail(string $username, string $recoveryEmail) : void { -- GitLab From 3d512dbb21dbe8e2309ca27696326d0ed9153d59 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Fri, 10 Oct 2025 22:32:12 +0600 Subject: [PATCH 3/4] chore: update according to review --- lib/Db/ConfigMapper.php | 28 +++++++--------------------- lib/Service/RecoveryEmailService.php | 16 ++++++++-------- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/lib/Db/ConfigMapper.php b/lib/Db/ConfigMapper.php index 715770a..3248d7c 100644 --- a/lib/Db/ConfigMapper.php +++ b/lib/Db/ConfigMapper.php @@ -14,12 +14,11 @@ class ConfigMapper { $this->appName = $appName; } - - public function countRecoveryEmail(string $pattern): int { + private function countRecoveryEmailEntry(string $pattern, string $operation): int { $pattern = strtolower($pattern); $qb = $this->connection->getQueryBuilder(); $qb->select($qb->func()->count('userid')) - ->from("preferences")->where('LOWER(`configvalue`) like :pattern AND (`configkey` = "unverified-recovery-email" OR `configkey` = "recovery-email") AND `appid` = :appname ') + ->from("preferences")->where("LOWER(`configvalue`) $operation :pattern AND (`configkey` = 'unverified-recovery-email' OR `configkey` = 'recovery-email') AND `appid` = :appname ") ->setParameter('pattern', $pattern) ->setParameter('appname', $this->appName); @@ -36,25 +35,12 @@ class ConfigMapper { return $count; } - public function countRecoveryEmailByRegex(string $pattern): int { - $pattern = strtolower($pattern); - $qb = $this->connection->getQueryBuilder(); - $qb->select($qb->func()->count('userid')) - ->from("preferences")->where('LOWER(`configvalue`) REGEXP :pattern AND (`configkey` = "unverified-recovery-email" OR `configkey` = "recovery-email") AND `appid` = :appname ') - ->setParameter('pattern', $pattern) - ->setParameter('appname', $this->appName); - - $result = $qb->executeQuery(); - $count = $result->fetchOne(); - $result->closeCursor(); - - if ($count !== false) { - $count = (int)$count; - } else { - $count = 0; - } + public function countRecoveryEmail(string $pattern): int { + return $this->countRecoveryEmailEntry($pattern, "like"); + } - return $count; + public function countRecoveryEmailByRegex(string $pattern): int { + return $this->countRecoveryEmailEntry($pattern, "REGEXP"); } public function getAllVerifiedRecoveryEmails(): array { diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index 08a6eda..e213545 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -54,6 +54,10 @@ class RecoveryEmailService { private const UNVERIFIED_USER_DISABLE_AT = 'unverified-user-disabled-at'; private const GMAIL_QUERY_LAST_PART = "(@gmail.com|@googlemail.com)"; //regex: is the str ends with any gmail's domains private const GMAIL_QUERY_MIDDLE_PART = "\+[^@]+"; //regex: for alias email addresses, ignore anything after + + private const GMAIL_DOMAINS = [ + 'gmail.com', + 'googlemail.com', + ]; private $cache; @@ -395,11 +399,10 @@ class RecoveryEmailService { return false; } - $regex = $this->getGmailQueryRegexFirstPart($mailPrefix); - $regex = $regex . self::GMAIL_QUERY_LAST_PART; + $regex = $this->getGmailQueryRegexFirstPart($mailPrefix) . self::GMAIL_QUERY_LAST_PART; $recoveryEmailCount = $this->configMapper->countRecoveryEmailByRegex($regex); - return $recoveryEmailCount !== 0; + return $recoveryEmailCount > 0; } /** @@ -423,9 +426,8 @@ class RecoveryEmailService { */ private function getGmailUsername(?string $email): ?string { $emailParts = $this->getUsernameAndDomain($email); - $gmailDomains = ['gmail.com', 'googlemail.com']; - if ($emailParts == null || !in_array($emailParts[1], $gmailDomains)) { + if ($emailParts == null || !in_array($emailParts[1], self::GMAIL_DOMAINS)) { return null; } @@ -452,9 +454,7 @@ class RecoveryEmailService { return true; } - $regex = $this->getGmailQueryRegexFirstPart($mailPrefix); - $regex = $regex . self::GMAIL_QUERY_MIDDLE_PART; - $regex = $regex . self::GMAIL_QUERY_LAST_PART; + $regex = $this->getGmailQueryRegexFirstPart($mailPrefix) . self::GMAIL_QUERY_MIDDLE_PART . self::GMAIL_QUERY_LAST_PART; $recoveryEmailCount = $this->configMapper->countRecoveryEmailByRegex($regex); return $recoveryEmailCount < $emailAliasLimit; -- GitLab From 1f4fb98d0326d51e64e3b752638dd9da3aa5f5ff Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Tue, 14 Oct 2025 13:50:36 +0600 Subject: [PATCH 4/4] chore: update the version to 11.0.2 --- appinfo/info.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index e24e90e..bb51644 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -5,7 +5,7 @@ Email Recovery Email Recovery App - 11.0.1 + 11.0.2 agpl MURENA SAS EmailRecovery -- GitLab