Description of the Problem
A registered Phorge user may add unverified emails in their profile. Doing so, these users can effectively claim some Repository Identities, which may create spam in Diffusion commits that is difficult to combat at the moment, because of this sub-problem:
T15556: Improve Diffusion identity reassignment propagation
Attack Surface
Phorge/Phabricator installations are affected if they may have untrusted users, that may add unverified email in their profile.
A Phorge installation is particularly affected by this problem if it has an open registration (check your /auth/register/ page).
Examples:
- check if a generic user can add a new unverified personal email from /settings/user/my.user.name.lol/page/email/ (yes by default for obvious reasons)
- check if a new user can register from /auth/register/, for example via email
- check if your page /auth/register/ allows to login using a social network (with or without "Trust Email Addresses" from /auth/config/). And check if later that user can add unverified emails
Affected Code - part 1 (CLI)
The workflow PhabricatorRepositoryManagementRebuildIdentitiesWorkflow seems affected by this problem. That is, this script:
./bin/repository rebuild-identities
It reads whatever email, and not just verified emails:
Proposed solution:
diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php index 9a0f68713e..9df88ad055 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php @@ -281,7 +281,7 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow $user->getMonogram())); $emails = id(new PhabricatorUserEmail())->loadAllWhere( - 'userPHID = %s', + 'userPHID = %s AND isVerified = 1', $user->getPHID()); if ($emails) { $this->rebuildEmails(mpull($emails, 'getAddress'));
Affected Code - part 2 (DiffusionResolveUserQuery)
Also (end probably most importantly) the method didUpdateEmailAddress deserves attention since it seems it's triggered even if the email is unverified:
So whenever you add an unverified email it seems it triggers this:
That ... seems to call PhabricatorUser::loadOneWithEmailAddress() at a certain point:
That finds user by whatever UNVERIFIED addresses:
Proposed solution, adding a new function that finds users by their verified email addresses and use that:
diff --git a/src/applications/diffusion/query/DiffusionResolveUserQuery.php b/src/applications/diffusion/query/DiffusionResolveUserQuery.php index 8ad13f660e..69a2e66c5f 100644 --- a/src/applications/diffusion/query/DiffusionResolveUserQuery.php +++ b/src/applications/diffusion/query/DiffusionResolveUserQuery.php @@ -93,7 +93,7 @@ final class DiffusionResolveUserQuery extends Phobject { private function findUserByEmailAddress($email_address) { - $by_email = PhabricatorUser::loadOneWithEmailAddress($email_address); + $by_email = PhabricatorUser::loadOneWithVerifiedEmailAddress($email_address); if ($by_email) { return $by_email->getPHID(); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 2c9cf9f877..b20deb6ca6 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -652,6 +652,28 @@ final class PhabricatorUser return $this->getUsername(); } + /** + * Load one user from their verified email address. + * @param string $address + * @return PhabricatorUser|null + */ + public static function loadOneWithVerifiedEmailAddress($address) { + $email = id(new PhabricatorUserEmail())->loadOneWhere( + 'address = %s AND isVerified = 1', + $address); + if (!$email) { + return null; + } + return id(new PhabricatorUser())->loadOneWhere( + 'phid = %s', + $email->getUserPHID()); + } + + /** + * Load one user from their potentially unverified email address. + * @param string $address + * @return PhabricatorUser|null + */ public static function loadOneWithEmailAddress($address) { $email = id(new PhabricatorUserEmail())->loadOneWhere( 'address = %s',
This seems the most safe approach to do not alter the PhabricatorPasswordAuthProvider business logic that may use an unverified email to try the login.
Discussion
Thanks for any opinion lol