From 8fafe2cc54954a4204df799dc4ea56469020c53e Mon Sep 17 00:00:00 2001 From: Tim Kelty Date: Mon, 14 Apr 2025 18:52:39 -0400 Subject: [PATCH 1/4] Prevent email enumeration on public registration --- src/controllers/UsersController.php | 188 +++++++++++++++------------- 1 file changed, 102 insertions(+), 86 deletions(-) diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php index 26a35e8ae5c..2014625a515 100644 --- a/src/controllers/UsersController.php +++ b/src/controllers/UsersController.php @@ -1483,6 +1483,7 @@ public function actionSaveUser(): ?Response // Manually validate the user so we can pass $clearErrors=false $success = $user->validate(null, false) && Craft::$app->getElements()->saveElement($user, false); + $existingEmailUser = null; if (!$success) { Craft::info('User not saved due to validation error.', __METHOD__); @@ -1493,111 +1494,126 @@ public function actionSaveUser(): ?Response $user->clearErrors('newPassword'); } - // Copy any 'unverifiedEmail' errors to 'email' - if (!$user->hasErrors('email')) { + if ($user->hasErrors('email')) { + $existingEmailUser = $isPublicRegistration && $user->email ? User::find() + ->email(Db::escapeParam($user->email)) + ->status(null) + ->one() : null; + + if ($existingEmailUser) { + $user->clearErrors('email'); + } + } else { + // Copy any 'unverifiedEmail' errors to 'email' $user->addErrors(['email' => $user->getErrors('unverifiedEmail')]); $user->clearErrors('unverifiedEmail'); } - return $this->asModelFailure( - $user, - StringHelper::upperCaseFirst(Craft::t('app', 'Couldn’t save {type}.', [ - 'type' => User::lowerDisplayName(), - ])), - $userVariable - ); - } - - // If this is a new user and email verification isn't required, - // go ahead and activate them now. - if ($isNewUser && !$requireEmailVerification && !$deactivateByDefault) { - Craft::$app->getUsers()->activateUser($user); + if ($user->hasErrors()) { + return $this->asModelFailure( + $user, + StringHelper::upperCaseFirst(Craft::t('app', 'Couldn’t save {type}.', [ + 'type' => User::lowerDisplayName(), + ])), + $userVariable + ); + } } - if ($isCurrentUser) { - // Save their preferences too - $preferredLocale = $this->request->getBodyParam('preferredLocale', $user->getPreference('locale')) ?: null; - if ($preferredLocale === '__blank__') { - $preferredLocale = null; + if ($existingEmailUser) { + Craft::$app->getUsers()->sendNewEmailVerifyEmail($existingEmailUser); + } else { + // If this is a new user and email verification isn't required, + // go ahead and activate them now. + if ($isNewUser && !$requireEmailVerification && !$deactivateByDefault) { + Craft::$app->getUsers()->activateUser($user); } - $preferences = [ - 'language' => $this->request->getBodyParam('preferredLanguage', $user->getPreference('language')), - 'locale' => $preferredLocale, - 'weekStartDay' => $this->request->getBodyParam('weekStartDay', $user->getPreference('weekStartDay')), - 'alwaysShowFocusRings' => (bool)$this->request->getBodyParam('alwaysShowFocusRings', $user->getPreference('alwaysShowFocusRings')), - 'useShapes' => (bool)$this->request->getBodyParam('useShapes', $user->getPreference('useShapes')), - 'underlineLinks' => (bool)$this->request->getBodyParam('underlineLinks', $user->getPreference('underlineLinks')), - 'notificationDuration' => $this->request->getBodyParam('notificationDuration', $user->getPreference('notificationDuration')), - ]; - if ($user->admin) { - $preferences = array_merge($preferences, [ - 'showFieldHandles' => (bool)$this->request->getBodyParam('showFieldHandles', $user->getPreference('showFieldHandles')), - 'enableDebugToolbarForSite' => (bool)$this->request->getBodyParam('enableDebugToolbarForSite', $user->getPreference('enableDebugToolbarForSite')), - 'enableDebugToolbarForCp' => (bool)$this->request->getBodyParam('enableDebugToolbarForCp', $user->getPreference('enableDebugToolbarForCp')), - 'showExceptionView' => (bool)$this->request->getBodyParam('showExceptionView', $user->getPreference('showExceptionView')), - 'profileTemplates' => (bool)$this->request->getBodyParam('profileTemplates', $user->getPreference('profileTemplates')), - ]); - } + if ($isCurrentUser) { + // Save their preferences too + $preferredLocale = $this->request->getBodyParam('preferredLocale', $user->getPreference('locale')) ?: null; + if ($preferredLocale === '__blank__') { + $preferredLocale = null; + } + $preferences = [ + 'language' => $this->request->getBodyParam('preferredLanguage', $user->getPreference('language')), + 'locale' => $preferredLocale, + 'weekStartDay' => $this->request->getBodyParam('weekStartDay', $user->getPreference('weekStartDay')), + 'alwaysShowFocusRings' => (bool)$this->request->getBodyParam('alwaysShowFocusRings', $user->getPreference('alwaysShowFocusRings')), + 'useShapes' => (bool)$this->request->getBodyParam('useShapes', $user->getPreference('useShapes')), + 'underlineLinks' => (bool)$this->request->getBodyParam('underlineLinks', $user->getPreference('underlineLinks')), + 'notificationDuration' => $this->request->getBodyParam('notificationDuration', $user->getPreference('notificationDuration')), + ]; - Craft::$app->getUsers()->saveUserPreferences($user, $preferences); - Craft::$app->updateTargetLanguage(); - } + if ($user->admin) { + $preferences = array_merge($preferences, [ + 'showFieldHandles' => (bool)$this->request->getBodyParam('showFieldHandles', $user->getPreference('showFieldHandles')), + 'enableDebugToolbarForSite' => (bool)$this->request->getBodyParam('enableDebugToolbarForSite', $user->getPreference('enableDebugToolbarForSite')), + 'enableDebugToolbarForCp' => (bool)$this->request->getBodyParam('enableDebugToolbarForCp', $user->getPreference('enableDebugToolbarForCp')), + 'showExceptionView' => (bool)$this->request->getBodyParam('showExceptionView', $user->getPreference('showExceptionView')), + 'profileTemplates' => (bool)$this->request->getBodyParam('profileTemplates', $user->getPreference('profileTemplates')), + ]); + } - // Is this the current user, and did their username just change? - // todo: remove comment when WI-51866 is fixed - /** @noinspection PhpUndefinedVariableInspection */ - if ($isCurrentUser && $user->username !== $oldUsername) { - // Update the username cookie - $userSession->sendUsernameCookie($user); - } + Craft::$app->getUsers()->saveUserPreferences($user, $preferences); + Craft::$app->updateTargetLanguage(); + } - // Save the user’s photo, if it was submitted - $this->_processUserPhoto($user); + // Is this the current user, and did their username just change? + // todo: remove comment when WI-51866 is fixed + /** @noinspection PhpUndefinedVariableInspection */ + if ($isCurrentUser && $user->username !== $oldUsername) { + // Update the username cookie + $userSession->sendUsernameCookie($user); + } - if (Craft::$app->getEdition() === Craft::Pro) { - // If this is public registration, assign the user to the default user group - if ($isPublicRegistration) { - // Assign them to the default user group - Craft::$app->getUsers()->assignUserToDefaultGroup($user); - } elseif ($currentUser) { - // Fire an 'afterBeforeGroupsAndPermissions' event - if ($this->hasEventHandlers(self::EVENT_BEFORE_ASSIGN_GROUPS_AND_PERMISSIONS)) { - $this->trigger(self::EVENT_BEFORE_ASSIGN_GROUPS_AND_PERMISSIONS, new UserEvent([ - 'user' => $user, - ])); - } + // Save the user’s photo, if it was submitted + $this->_processUserPhoto($user); + + if (Craft::$app->getEdition() === Craft::Pro) { + // If this is public registration, assign the user to the default user group + if ($isPublicRegistration) { + // Assign them to the default user group + Craft::$app->getUsers()->assignUserToDefaultGroup($user); + } elseif ($currentUser) { + // Fire an 'afterBeforeGroupsAndPermissions' event + if ($this->hasEventHandlers(self::EVENT_BEFORE_ASSIGN_GROUPS_AND_PERMISSIONS)) { + $this->trigger(self::EVENT_BEFORE_ASSIGN_GROUPS_AND_PERMISSIONS, new UserEvent([ + 'user' => $user, + ])); + } - // Assign user groups and permissions if the current user is allowed to do that - $this->_saveUserGroups($user, $currentUser); - $this->_saveUserPermissions($user, $currentUser); + // Assign user groups and permissions if the current user is allowed to do that + $this->_saveUserGroups($user, $currentUser); + $this->_saveUserPermissions($user, $currentUser); - // Fire an 'afterAssignGroupsAndPermissions' event - if ($this->hasEventHandlers(self::EVENT_AFTER_ASSIGN_GROUPS_AND_PERMISSIONS)) { - $this->trigger(self::EVENT_AFTER_ASSIGN_GROUPS_AND_PERMISSIONS, new UserEvent([ - 'user' => $user, - ])); + // Fire an 'afterAssignGroupsAndPermissions' event + if ($this->hasEventHandlers(self::EVENT_AFTER_ASSIGN_GROUPS_AND_PERMISSIONS)) { + $this->trigger(self::EVENT_AFTER_ASSIGN_GROUPS_AND_PERMISSIONS, new UserEvent([ + 'user' => $user, + ])); + } } } - } - // Do we need to send a verification email out? - if ($sendActivationEmail) { - // Temporarily set the unverified email on the User so the verification email goes to the - // right place - $originalEmail = $user->email; - $user->email = $user->unverifiedEmail; + // Do we need to send a verification email out? + if ($sendActivationEmail) { + // Temporarily set the unverified email on the User so the verification email goes to the + // right place + $originalEmail = $user->email; + $user->email = $user->unverifiedEmail; - if ($isNewUser) { - // Send the activation email - Craft::$app->getUsers()->sendActivationEmail($user); - } else { - // Send the standard verification email - Craft::$app->getUsers()->sendNewEmailVerifyEmail($user); - } + if ($isNewUser) { + // Send the activation email + Craft::$app->getUsers()->sendActivationEmail($user); + } else { + // Send the standard verification email + Craft::$app->getUsers()->sendNewEmailVerifyEmail($user); + } - // Put the original email back into place - $user->email = $originalEmail; + // Put the original email back into place + $user->email = $originalEmail; + } } // Is this public registration, and was the user going to be activated automatically? From ffb83f00f2fc3fd32567cf3d02cd800ddcd790d4 Mon Sep 17 00:00:00 2001 From: Tim Kelty Date: Tue, 15 Apr 2025 16:00:35 -0400 Subject: [PATCH 2/4] Check for preventUserEnumeration --- src/controllers/UsersController.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php index 2014625a515..1d1419d06a0 100644 --- a/src/controllers/UsersController.php +++ b/src/controllers/UsersController.php @@ -1495,13 +1495,15 @@ public function actionSaveUser(): ?Response } if ($user->hasErrors('email')) { - $existingEmailUser = $isPublicRegistration && $user->email ? User::find() - ->email(Db::escapeParam($user->email)) - ->status(null) - ->one() : null; + if (Craft::$app->getConfig()->getGeneral()->preventUserEnumeration && $isPublicRegistration && $user->email) { + $existingEmailUser = User::find() + ->email(Db::escapeParam($user->email)) + ->status(null) + ->one(); - if ($existingEmailUser) { - $user->clearErrors('email'); + if ($existingEmailUser) { + $user->clearErrors('email'); + } } } else { // Copy any 'unverifiedEmail' errors to 'email' From e6002f3d4dd714094a0389d48d04b32035fbfd5d Mon Sep 17 00:00:00 2001 From: Tim Kelty Date: Wed, 16 Apr 2025 03:38:38 -0400 Subject: [PATCH 3/4] Changelog # Conflicts: # CHANGELOG-WIP.md --- CHANGELOG-WIP.md | 54 +++--------------------------------------------- 1 file changed, 3 insertions(+), 51 deletions(-) diff --git a/CHANGELOG-WIP.md b/CHANGELOG-WIP.md index 7a90db25797..661758cb279 100644 --- a/CHANGELOG-WIP.md +++ b/CHANGELOG-WIP.md @@ -1,52 +1,4 @@ -# Release Notes for Craft CMS 4.15 (WIP) +# Release Notes for Craft CMS 4.15.1 (WIP) -### Content Management -- Condition rules for Checkboxes, Dropdown, Multi-select, and Radio Buttons fields now include “has a value” and “is empty” operators. ([#17015](https://github.com/craftcms/cms/pull/17015)) -- The Assets index page now prompts for confirmation when moving more than 50 assets, or assets totalling more than 50MB, at once. ([#16908](https://github.com/craftcms/cms/pull/16908)) -- The success notification displayed after an asset move now includes an “Undo” button, if less than 50 assets/50MB were involved in the move. ([#16908](https://github.com/craftcms/cms/pull/16908)) -- Window scrolling is now blocked when a modal window is open. ([#16768](https://github.com/craftcms/cms/pull/16768)) - -### Administration -- Added the `db/repair` command. ([#16812](https://github.com/craftcms/cms/pull/16812)) -- Added the `--batch-size` option for `resave/*` commands. ([#16586](https://github.com/craftcms/cms/issues/16586)) -- The `plugin/install` command now accepts an `edition` argument, and prompts for the default edition if none is specified. ([#17030](https://github.com/craftcms/cms/pull/17030)) -- The `plugin/uninstall` command now reports if no plugin is installed with the provided handle. ([#17030](https://github.com/craftcms/cms/pull/17030)) -- The `users/create` command now prompts to send an activation email, or outputs an activation URL. ([#16794](https://github.com/craftcms/cms/pull/16794)) -- Dragging headings within the Customize Sources modal now also drags any subsequent sources. ([#16737](https://github.com/craftcms/cms/issues/16737)) -- When switching field types, any field settings which are defined by the same base class are now preserved. ([#16783](https://github.com/craftcms/cms/pull/16783)) - -### Development -- Added the `searchTermOptions` GraphQL query argument. ([#16979](https://github.com/craftcms/cms/pull/16979)) -- Added the `revisionNotes` GraphQL entry mutation argument. ([#16943](https://github.com/craftcms/cms/issues/16943)) - -### Extensibility -- Global nav items and breadcrumbs can now have `aria-label` attributes via an `ariaLabel` property. -- Added `craft\base\ElementInterface::getSerializedFieldValuesForDb()`. -- Added `craft\base\FieldInterface::serializeValueForDb()`. -- Added `craft\base\conditions\BaseMultiSelectConditionRule::$includeEmptyOperators`. -- Added `craft\db\Connection::getIsMaria()`. -- Added `craft\db\Table::SEARCHINDEXQUEUE_FIELDS`. -- Added `craft\db\Table::SEARCHINDEXQUEUE`. -- Added `craft\db\mysql\QueryBuilder::jsonContains()`. -- Added `craft\db\mysql\QueryBuilder::jsonExtract()`. -- Added `craft\db\pgsql\QueryBuilder::jsonContains()`. -- Added `craft\db\pgsql\QueryBuilder::jsonExtract()`. -- Added `craft\queue\ReleasableQueueInterface`. ([#16672](https://github.com/craftcms/cms/pull/16672)) -- Added `craft\records\User::haveIndexAttributesChanged()`. -- Added `craft\services\Search::indexElementIfQueued()`. -- Added `craft\services\Search::queueIndexElement()`. -- `craft\cache\ElementQueryTagDependency` now merges cache tags provided by the element query with any tags already set on its `$tags` property. - -### System -- `craft\queue\Queue::release()` and `releaseAll()` now call `release()` and `releaseAll()` on the proxied queue if it implements `craft\queue\ReleasableQueueInterface`. ([#16672](https://github.com/craftcms/cms/pull/16672)) -- The `changedattributes` and `changedfields` tables are now cleaned up during garbage collection. ([#16531](https://github.com/craftcms/cms/pull/16531)) -- The `resourcepaths` table is now truncated when clearing control panel resources, via the Caches utility or the `clear-caches/cp-resources` command. ([#16514](https://github.com/craftcms/cms/issues/16514)) -- Date values for custom fields are now represented as ISO-8601 date strings (with time zones) within element exports. ([#16629](https://github.com/craftcms/cms/pull/16629)) -- “Updating search indexes” queue jobs no longer do anything if search indexes were already updated for the element since the job was created. ([#16644](https://github.com/craftcms/cms/pull/16644)) -- User caches are no longer invalidated on login attempts or when user management actions are taken. ([#16937](https://github.com/craftcms/cms/pull/16937)) -- Batchable queue jobs now spawn new batches when their execution time is getting uncomfortably close to their TTR duration. ([#16947](https://github.com/craftcms/cms/pull/16947)) -- Added compatibility with Symfony HTTP Client 7. ([#17065](https://github.com/craftcms/cms/pull/17065)) -- Updated Yii to 2.0.52. -- Updated yii2-debug to 2.1.26. -- Updated Axios to 1.8.4. -- Fixed a bug where `CRAFT_WEB_URL` and `CRAFT_WEB_ROOT` environment variables could be overridden by `@web` and `@webroot` aliases define by the `aliases` config setting. ([#16980](https://github.com/craftcms/cms/pull/16980)) +### Security +- Prevented potential email enumeration on public registration forms. ([#17085](https://github.com/craftcms/cms/pull/17085)) From 21fd6c5b7bc810450493217c17159a95ed530ed6 Mon Sep 17 00:00:00 2001 From: brandonkelly Date: Tue, 6 May 2025 10:26:51 -0700 Subject: [PATCH 4/4] Move the release note to the WIP changelog [ci skip] --- CHANGELOG-WIP.md | 3 +++ CHANGELOG.md | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG-WIP.md b/CHANGELOG-WIP.md index 93fcde51db4..4bf104c1446 100644 --- a/CHANGELOG-WIP.md +++ b/CHANGELOG-WIP.md @@ -6,3 +6,6 @@ ### Extensibility - Added `craft\fields\BaseRelationField::canShowSiteMenu()`. + +### System +- Prevented potential email enumeration on public registration forms. ([#17085](https://github.com/craftcms/cms/pull/17085)) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3fd415ea4d..ce0f9a7aed1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,6 @@ ## Unreleased - Return URLs are now sanitized before being saved to the PHP session. -- Prevented potential email enumeration on public registration forms. ([#17085](https://github.com/craftcms/cms/pull/17085)) - Fixed a bug where admin tables’ pagination footers could be positioned incorrectly in slideouts. ([#17187](https://github.com/craftcms/cms/issues/17187)) - Fixed a SQL error that could occur when running garbage collection. ([#17197](https://github.com/craftcms/cms/issues/17197)) - Fixed a PHP error that could occur if malformed UTF-8 data was passed to `craft\helpers\StringHepler::replaceMb4()`. ([#17202](https://github.com/craftcms/cms/issues/17202))