== 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}
== 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:
https://we.phorge.it/source/phorge/browse/master/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php;48fd3f1c40def49cdfec3ad342e32225adc31a79$283-285
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 ( ==
Also (end probably most importantly) the method `didUpdateEmailAddress` deserves attention since it seems it's triggered even if the email is unverified:
https://we.phorge.it/source/phorge/browse/master/src/applications/people/editor/PhabricatorUserEditor.php;48fd3f1c40def49cdfec3ad342e32225adc31a79$88-93
So whenever you add an unverified email it seems it triggers this:
https://we.phorge.it/source/phorge/browse/master/src/applications/repository/worker/PhabricatorRepositoryIdentityChangeWorker.php
That ... seems to call `PhabricatorUser::loadOneWithEmailAddress()` at a certain point:
https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/query/DiffusionResolveUserQuery.php;48fd3f1c40def49cdfec3ad342e32225adc31a79$95-102
That finds user by whatever UNVERIFIED addresses:
https://we.phorge.it/source/phorge/browse/master/src/applications/people/storage/PhabricatorUser.php;48fd3f1c40def49cdfec3ad342e32225adc31a79$655-665
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