mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 22:10:55 +01:00
Invalidate outstanding password reset links when users adjust email addresses
Summary: Fixes T5506. Depends on D10133. When users remove an email address or change their primary email address, invalidate any outstanding password reset links. This is a very small security risk, but the current behavior is somewhat surprising, and an attacker could sit on a reset link for up to 24 hours and then use it to re-compromise an account. Test Plan: - Changed primary address and removed addreses. - Verified these actions invalidated outstanding one-time login temporary tokens. - Tried to use revoked reset links. - Revoked normally from new UI panel. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5506 Differential Revision: https://secure.phabricator.com/D10134
This commit is contained in:
parent
30f6405a86
commit
e56dc8f299
4 changed files with 62 additions and 8 deletions
|
@ -46,7 +46,7 @@ final class PhabricatorAuthRevokeTokenController
|
|||
|
||||
if ($request->isDialogFormPost()) {
|
||||
foreach ($tokens as $token) {
|
||||
$token->setTokenExpires(PhabricatorTime::getNow() - 1)->save();
|
||||
$token->revokeToken();
|
||||
}
|
||||
return id(new AphrontRedirectResponse())->setURI($panel_uri);
|
||||
}
|
||||
|
|
|
@ -44,6 +44,30 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO
|
|||
return false;
|
||||
}
|
||||
|
||||
public function revokeToken() {
|
||||
if ($this->isRevocable()) {
|
||||
$this->setTokenExpires(PhabricatorTime::getNow() - 1)->save();
|
||||
}
|
||||
return $this;
|
||||
}
|
||||
|
||||
public static function revokeTokens(
|
||||
PhabricatorUser $viewer,
|
||||
array $object_phids,
|
||||
array $token_types) {
|
||||
|
||||
$tokens = id(new PhabricatorAuthTemporaryTokenQuery())
|
||||
->setViewer($viewer)
|
||||
->withObjectPHIDs($object_phids)
|
||||
->withTokenTypes($token_types)
|
||||
->withExpired(false)
|
||||
->execute();
|
||||
|
||||
foreach ($tokens as $token) {
|
||||
$token->revokeToken();
|
||||
}
|
||||
}
|
||||
|
||||
/* -( PhabricatorPolicyInterface )----------------------------------------- */
|
||||
|
||||
|
||||
|
|
|
@ -427,6 +427,8 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
|
|||
$user->endWriteLocking();
|
||||
$user->saveTransaction();
|
||||
|
||||
$this->revokePasswordResetLinks($user);
|
||||
|
||||
return $this;
|
||||
}
|
||||
|
||||
|
@ -490,6 +492,9 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
|
|||
}
|
||||
$email->sendNewPrimaryEmail($user);
|
||||
|
||||
|
||||
$this->revokePasswordResetLinks($user);
|
||||
|
||||
return $this;
|
||||
}
|
||||
|
||||
|
@ -575,4 +580,19 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
|
|||
}
|
||||
}
|
||||
|
||||
private function revokePasswordResetLinks(PhabricatorUser $user) {
|
||||
// Revoke any outstanding password reset links. If an attacker compromises
|
||||
// an account, changes the email address, and sends themselves a password
|
||||
// reset link, it could otherwise remain live for a short period of time
|
||||
// and allow them to compromise the account again later.
|
||||
|
||||
PhabricatorAuthTemporaryToken::revokeTokens(
|
||||
$user,
|
||||
array($user->getPHID()),
|
||||
array(
|
||||
PhabricatorAuthSessionEngine::ONETIME_TEMPORARY_TOKEN_TYPE,
|
||||
PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE,
|
||||
));
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -276,9 +276,14 @@ final class PhabricatorSettingsPanelEmailAddresses
|
|||
->setUser($user)
|
||||
->addHiddenInput('delete', $email_id)
|
||||
->setTitle(pht("Really delete address '%s'?", $address))
|
||||
->appendChild(phutil_tag('p', array(), pht(
|
||||
'Are you sure you want to delete this address? You will no '.
|
||||
'longer be able to use it to login.')))
|
||||
->appendParagraph(
|
||||
pht(
|
||||
'Are you sure you want to delete this address? You will no '.
|
||||
'longer be able to use it to login.'))
|
||||
->appendParagraph(
|
||||
pht(
|
||||
'Note: Removing an email address from your account will invalidate '.
|
||||
'any outstanding password reset links.'))
|
||||
->addSubmitButton(pht('Delete'))
|
||||
->addCancelButton($uri);
|
||||
|
||||
|
@ -359,10 +364,15 @@ final class PhabricatorSettingsPanelEmailAddresses
|
|||
->setUser($user)
|
||||
->addHiddenInput('primary', $email_id)
|
||||
->setTitle(pht('Change primary email address?'))
|
||||
->appendChild(phutil_tag('p', array(), pht(
|
||||
'If you change your primary address, Phabricator will send'.
|
||||
' all email to %s.',
|
||||
$address)))
|
||||
->appendParagraph(
|
||||
pht(
|
||||
'If you change your primary address, Phabricator will send all '.
|
||||
'email to %s.',
|
||||
$address))
|
||||
->appendParagraph(
|
||||
pht(
|
||||
'Note: Changing your primary email address will invalidate any '.
|
||||
'outstanding password reset links.'))
|
||||
->addSubmitButton(pht('Change Primary Address'))
|
||||
->addCancelButton($uri);
|
||||
|
||||
|
|
Loading…
Reference in a new issue