recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Terminate other sessions on credential changes

Summary:
Fixes T5509. Currently, existing sessions live on even if you change your password.

Over the course of the program, we've recieved a lot of HackerOne reports that sessions do not terminate when users change their passwords. I hold that this isn't a security vulnerability: users can explicitly manage sessions, and this is more general and more powerful than tying session termination to password resets. In particular, many installs do not use a password provider at all (and no researcher has reported this in a general, application-aware way that discusses multiple authentication providers).

That said, dealing with these false positives is vaguely time consuming, and the "expected" behavior isn't bad for users, so just align behavior with researcher expectations: when passwords are changed, providers are removed, or multi-factor authentication is added to an account, terminate all other active login sessions.

Test Plan:
- Using two browsers, established multiple login sessions.
- In one browser, changed account password. Saw session terminate and logout in the second browser.
- In one browser, removed an authentication provider. Saw session terminate and logout in the second browser.
- In one browser, added MFA. Saw session terminate and logout in the second browser.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5509

Differential Revision: https://secure.phabricator.com/D10135

epriestley 95eeffff e56dc8f2

+65 -1
+10 -1
src/applications/auth/controller/PhabricatorAuthUnlinkController.php
··· 57 57 58 58 if ($request->isDialogFormPost()) { 59 59 $account->delete(); 60 + 61 + id(new PhabricatorAuthSessionEngine())->terminateLoginSessions( 62 + $viewer, 63 + $request->getCookie(PhabricatorCookies::COOKIE_SESSION)); 64 + 60 65 return id(new AphrontRedirectResponse())->setURI($this->getDoneURI()); 61 66 } 62 67 ··· 130 135 $dialog = id(new AphrontDialogView()) 131 136 ->setUser($this->getRequest()->getUser()) 132 137 ->setTitle($title) 133 - ->appendChild($body) 138 + ->appendParagraph($body) 139 + ->appendParagraph( 140 + pht( 141 + 'Note: Unlinking an authentication provider will terminate any '. 142 + 'other active login sessions.')) 134 143 ->addSubmitButton(pht('Unlink Account')) 135 144 ->addCancelButton($this->getDoneURI()); 136 145
+37
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
··· 250 250 } 251 251 252 252 253 + /** 254 + * Terminate all of a user's login sessions. 255 + * 256 + * This is used when users change passwords, linked accounts, or add 257 + * multifactor authentication. 258 + * 259 + * @param PhabricatorUser User whose sessions should be terminated. 260 + * @param string|null Optionally, one session to keep. Normally, the current 261 + * login session. 262 + * 263 + * @return void 264 + */ 265 + public function terminateLoginSessions( 266 + PhabricatorUser $user, 267 + $except_session = null) { 268 + 269 + $sessions = id(new PhabricatorAuthSessionQuery()) 270 + ->setViewer($user) 271 + ->withIdentityPHIDs(array($user->getPHID())) 272 + ->execute(); 273 + 274 + if ($except_session !== null) { 275 + $except_session = PhabricatorHash::digest($except_session); 276 + } 277 + 278 + foreach ($sessions as $key => $session) { 279 + if ($except_session !== null) { 280 + if ($except_session == $session->getSessionKey()) { 281 + continue; 282 + } 283 + } 284 + 285 + $session->delete(); 286 + } 287 + } 288 + 289 + 253 290 /* -( High Security )------------------------------------------------------ */ 254 291 255 292
+7
src/applications/settings/panel/PhabricatorSettingsPanelMultiFactor.php
··· 198 198 199 199 $user->updateMultiFactorEnrollment(); 200 200 201 + // Terminate other sessions so they must log in and survive the 202 + // multi-factor auth check. 203 + 204 + id(new PhabricatorAuthSessionEngine())->terminateLoginSessions( 205 + $user, 206 + $request->getCookie(PhabricatorCookies::COOKIE_SESSION)); 207 + 201 208 return id(new AphrontRedirectResponse()) 202 209 ->setURI($this->getPanelURI('?id='.$config->getID())); 203 210 }
+11
src/applications/settings/panel/PhabricatorSettingsPanelPassword.php
··· 116 116 $next = $this->getPanelURI('?saved=true'); 117 117 } 118 118 119 + id(new PhabricatorAuthSessionEngine())->terminateLoginSessions( 120 + $user, 121 + $request->getCookie(PhabricatorCookies::COOKIE_SESSION)); 122 + 119 123 return id(new AphrontRedirectResponse())->setURI($next); 120 124 } 121 125 } ··· 177 181 ->setLabel(pht('Best Available Algorithm')) 178 182 ->setValue(PhabricatorPasswordHasher::getBestAlgorithmName())); 179 183 184 + $form->appendRemarkupInstructions( 185 + pht( 186 + 'NOTE: Changing your password will terminate any other outstanding '. 187 + 'login sessions.')); 188 + 180 189 $form_box = id(new PHUIObjectBoxView()) 181 190 ->setHeaderText(pht('Change Password')) 182 191 ->setFormSaved($request->getStr('saved')) ··· 187 196 $form_box, 188 197 ); 189 198 } 199 + 200 + 190 201 }