Consolidate use of magical cookie name strings

Summary: Ref T4339. We have more magical cookie names than we should, move them all to a central location.

Test Plan: Registered, logged in, linked account, logged out. See inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

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

src/__phutil_library_map__.php

@@ -1319,6 +1319,7 @@ phutil_register_library_map(array(
     'PhabricatorContentSource' => 'applications/metamta/contentsource/PhabricatorContentSource.php',
     'PhabricatorContentSourceView' => 'applications/metamta/contentsource/PhabricatorContentSourceView.php',
     'PhabricatorController' => 'applications/base/controller/PhabricatorController.php',
+    'PhabricatorCookies' => 'applications/auth/constants/PhabricatorCookies.php',
     'PhabricatorCoreConfigOptions' => 'applications/config/option/PhabricatorCoreConfigOptions.php',
     'PhabricatorCountdown' => 'applications/countdown/storage/PhabricatorCountdown.php',
     'PhabricatorCountdownCapabilityDefaultView' => 'applications/countdown/capability/PhabricatorCountdownCapabilityDefaultView.php',
@@ -3934,6 +3935,7 @@ phutil_register_library_map(array(
     'PhabricatorConpherencePHIDTypeThread' => 'PhabricatorPHIDType',
     'PhabricatorContentSourceView' => 'AphrontView',
     'PhabricatorController' => 'AphrontController',
+    'PhabricatorCookies' => 'Phobject',
     'PhabricatorCoreConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorCountdown' =>
     array(

src/aphront/console/DarkConsoleDataController.php

@@ -58,12 +58,17 @@ final class DarkConsoleDataController extends PhabricatorController {
 
         $panel = $obj->renderPanel();
 
-        if (!empty($_COOKIE['phsid'])) {
+        // Because cookie names can now be prefixed, wipe out any cookie value
-          $panel = PhutilSafeHTML::applyFunction(
+        // with the session cookie name anywhere in its name.
-            'str_replace',
+        $pattern = '('.preg_quote(PhabricatorCookies::COOKIE_SESSION).')';
-            $_COOKIE['phsid'],
+        foreach ($_COOKIE as $cookie_name => $cookie_value) {
-            '(session-key)',
+          if (preg_match($pattern, $cookie_name)) {
-            $panel);
+            $panel = PhutilSafeHTML::applyFunction(
+              'str_replace',
+              $cookie_value,
+              '(session-key)',
+              $panel);
+          }
         }
 
         $output['panel'][$class] = $panel;

src/applications/auth/constants/PhabricatorCookies.php

@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * Consolidates Phabricator application cookies, including registration
+ * and session management.
+ */
+final class PhabricatorCookies extends Phobject {
+
+  /**
+   * Stores the login username for password authentication. This is just a
+   * display value for convenience, used to prefill the login form. It is not
+   * authoritative.
+   */
+  const COOKIE_USERNAME       = 'phusr';
+
+
+  /**
+   * Stores the user's current session ID. This is authoritative and establishes
+   * the user's identity.
+   */
+  const COOKIE_SESSION        = 'phsid';
+
+
+  /**
+   * Stores a secret used during new account registration to prevent an attacker
+   * from tricking a victim into registering an account which is linked to
+   * credentials the attacker controls.
+   */
+  const COOKIE_REGISTRATION   = 'phreg';
+
+
+  /**
+   * Stores a secret used during OAuth2 handshakes to prevent various attacks
+   * where an attacker hands a victim a URI corresponding to the middle of an
+   * OAuth2 workflow and we might otherwise do something sketchy. Particularly,
+   * this corresponds to the OAuth2 "code".
+   */
+  const COOKIE_CLIENTID       = 'phcid';
+
+
+  /**
+   * Stores the URI to redirect the user to after login. This allows users to
+   * visit a path like `/feed/`, be prompted to login, and then be redirected
+   * back to `/feed/` after the workflow completes.
+   */
+  const COOKIE_NEXTURI        = 'next_uri';
+
+}

src/applications/auth/controller/PhabricatorAuthController.php

@@ -88,8 +88,12 @@ abstract class PhabricatorAuthController extends PhabricatorController {
       // there's no check for users being disabled here.
 
       $request = $this->getRequest();
-      $request->setCookie('phusr', $user->getUsername());
+      $request->setCookie(
-      $request->setCookie('phsid', $session_key);
+        PhabricatorCookies::COOKIE_USERNAME,
+        $user->getUsername());
+      $request->setCookie(
+        PhabricatorCookies::COOKIE_SESSION,
+        $session_key);
 
       $this->clearRegistrationCookies();
     }
@@ -101,15 +105,15 @@ abstract class PhabricatorAuthController extends PhabricatorController {
     $request = $this->getRequest();
 
     // Clear the registration key.
-    $request->clearCookie('phreg');
+    $request->clearCookie(PhabricatorCookies::COOKIE_REGISTRATION);
 
     // Clear the client ID / OAuth state key.
-    $request->clearCookie('phcid');
+    $request->clearCookie(PhabricatorCookies::COOKIE_CLIENTID);
   }
 
   private function buildLoginValidateResponse(PhabricatorUser $user) {
     $validate_uri = new PhutilURI($this->getApplicationURI('validate/'));
-    $validate_uri->setQueryParam('phusr', $user->getUsername());
+    $validate_uri->setQueryParam('expect', $user->getUsername());
 
     return id(new AphrontRedirectResponse())->setURI((string)$validate_uri);
   }
@@ -168,7 +172,8 @@ abstract class PhabricatorAuthController extends PhabricatorController {
       return array($account, $provider, $response);
     }
 
-    $registration_key = $request->getCookie('phreg');
+    $registration_key = $request->getCookie(
+      PhabricatorCookies::COOKIE_REGISTRATION);
 
     // NOTE: This registration key check is not strictly necessary, because
     // we're only creating new accounts, not linking existing accounts. It
@@ -181,7 +186,7 @@ abstract class PhabricatorAuthController extends PhabricatorController {
     // since you could have simply completed the process yourself.
 
     if (!$registration_key) {
-      $response =  $this->renderError(
+      $response = $this->renderError(
         pht(
           'Your browser did not submit a registration key with the request. '.
           'You must use the same browser to begin and complete registration. '.

src/applications/auth/controller/PhabricatorAuthLinkController.php

@@ -79,7 +79,10 @@ final class PhabricatorAuthLinkController
 
     $panel_uri = '/settings/panel/external/';
 
-    $request->setCookie('phcid', Filesystem::readRandomCharacters(16));
+    $request->setCookie(
+      PhabricatorCookies::COOKIE_CLIENTID,
+      Filesystem::readRandomCharacters(16));
+
     switch ($this->action) {
       case 'link':
         $form = $provider->buildLinkForm($this);

src/applications/auth/controller/PhabricatorAuthLoginController.php

@@ -166,7 +166,9 @@ final class PhabricatorAuthLoginController
       $account->save();
     unset($unguarded);
 
-    $this->getRequest()->setCookie('phreg', $registration_key);
+    $this->getRequest()->setCookie(
+      PhabricatorCookies::COOKIE_REGISTRATION,
+      $registration_key);
 
     return id(new AphrontRedirectResponse())->setURI($next_uri);
   }

src/applications/auth/controller/PhabricatorAuthStartController.php

@@ -24,10 +24,10 @@ final class PhabricatorAuthStartController
       return $this->processConduitRequest();
     }
 
-    if ($request->getCookie('phusr') && $request->getCookie('phsid')) {
+    if (strlen($request->getCookie(PhabricatorCookies::COOKIE_SESSION))) {
       // The session cookie is invalid, so clear it.
-      $request->clearCookie('phusr');
+      $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME);
-      $request->clearCookie('phsid');
+      $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
 
       return $this->renderError(
         pht(
@@ -71,8 +71,12 @@ final class PhabricatorAuthStartController
     }
 
     if (!$request->isFormPost()) {
-      $request->setCookie('next_uri', $next_uri);
+      $request->setCookie(
-      $request->setCookie('phcid', Filesystem::readRandomCharacters(16));
+        PhabricatorCookies::COOKIE_NEXTURI,
+        $next_uri);
+      $request->setCookie(
+        PhabricatorCookies::COOKIE_CLIENTID,
+        Filesystem::readRandomCharacters(16));
     }
 
     $not_buttons = array();

src/applications/auth/controller/PhabricatorAuthValidateController.php

@@ -13,7 +13,7 @@ final class PhabricatorAuthValidateController
 
     $failures = array();
 
-    if (!strlen($request->getStr('phusr'))) {
+    if (!strlen($request->getStr('expect'))) {
       return $this->renderErrors(
         array(
           pht(
@@ -21,8 +21,8 @@ final class PhabricatorAuthValidateController
             'phusr')));
     }
 
-    $expect_phusr = $request->getStr('phusr');
+    $expect_phusr = $request->getStr('expect');
-    $actual_phusr = $request->getCookie('phusr');
+    $actual_phusr = $request->getCookie(PhabricatorCookies::COOKIE_USERNAME);
     if ($actual_phusr != $expect_phusr) {
       if ($actual_phusr) {
         $failures[] = pht(
@@ -54,8 +54,8 @@ final class PhabricatorAuthValidateController
       return $this->renderErrors($failures);
     }
 
-    $next = $request->getCookie('next_uri');
+    $next = $request->getCookie(PhabricatorCookies::COOKIE_NEXTURI);
-    $request->clearCookie('next_uri');
+    $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
 
     if (!PhabricatorEnv::isValidLocalWebResource($next)) {
       $next = '/';

src/applications/auth/controller/PhabricatorEmailTokenController.php

@@ -86,7 +86,7 @@ final class PhabricatorEmailTokenController
           ));
     }
 
-    $request->setCookie('next_uri', $next);
+    $request->setCookie(PhabricatorCookies::COOKIE_NEXTURI, $next);
 
     return $this->loginUser($target_user);
   }

src/applications/auth/controller/PhabricatorLogoutController.php

@@ -32,8 +32,8 @@ final class PhabricatorLogoutController
       // Destroy the user's session in the database so logout works even if
       // their cookies have some issues. We'll detect cookie issues when they
       // try to login again and tell them to clear any junk.
-      $phsid = $request->getCookie('phsid');
+      $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
-      if ($phsid) {
+      if (strlen($phsid)) {
         $session = id(new PhabricatorAuthSessionQuery())
           ->setViewer($user)
           ->withSessionKeys(array($phsid))
@@ -42,7 +42,7 @@ final class PhabricatorLogoutController
           $session->delete();
         }
       }
-      $request->clearCookie('phsid');
+      $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
 
       return id(new AphrontRedirectResponse())
         ->setURI('/login/');
@@ -52,8 +52,7 @@ final class PhabricatorLogoutController
       $dialog = id(new AphrontDialogView())
         ->setUser($user)
         ->setTitle(pht('Log out of Phabricator?'))
-        ->appendChild(phutil_tag('p', array(), pht(
+        ->appendChild(pht('Are you sure you want to log out?'))
-          'Are you sure you want to log out?')))
         ->addSubmitButton(pht('Logout'))
         ->addCancelButton('/');
 

src/applications/auth/provider/PhabricatorAuthProviderOAuth.php

@@ -35,9 +35,11 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
 
   protected function renderLoginForm(AphrontRequest $request, $mode) {
     $adapter = $this->getAdapter();
-    $adapter->setState(PhabricatorHash::digest($request->getCookie('phcid')));
+    $adapter->setState(
+      PhabricatorHash::digest(
+        $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID)));
 
-    $scope = $request->getStr("scope");
+    $scope = $request->getStr('scope');
     if ($scope) {
       $adapter->setScope($scope);
     }
@@ -81,14 +83,15 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
     }
 
     if ($adapter->supportsStateParameter()) {
-      $phcid = $request->getCookie('phcid');
+      $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID);
       if (!strlen($phcid)) {
         $response = $controller->buildProviderErrorResponse(
           $this,
           pht(
-            'Your browser did not submit a "phcid" cookie with OAuth state '.
+            'Your browser did not submit a "%s" cookie with OAuth state '.
             'information in the request. Check that cookies are enabled. '.
-            'If this problem persists, you may need to clear your cookies.'));
+            'If this problem persists, you may need to clear your cookies.',
+            PhabricatorCookies::COOKIE_CLIENTID));
       }
 
       $state = $request->getStr('state');

src/applications/auth/provider/PhabricatorAuthProviderPassword.php

@@ -87,7 +87,7 @@ final class PhabricatorAuthProviderPassword
 
     $v_user = nonempty(
       $request->getStr('username'),
-      $request->getCookie('phusr'));
+      $request->getCookie(PhabricatorCookies::COOKIE_USERNAME));
 
     $e_user = null;
     $e_pass = null;
@@ -191,8 +191,8 @@ final class PhabricatorAuthProviderPassword
         PhabricatorUserLog::ACTION_LOGIN_FAILURE);
       $log->save();
 
-      $request->clearCookie('phusr');
+      $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME);
-      $request->clearCookie('phsid');
+      $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
 
       $response = $controller->buildProviderPageResponse(
         $this,

src/applications/base/controller/PhabricatorController.php

@@ -35,7 +35,7 @@ abstract class PhabricatorController extends AphrontController {
     } else {
       $user = new PhabricatorUser();
 
-      $phsid = $request->getCookie('phsid');
+      $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
       if ($phsid) {
         $session_user = id(new PhabricatorAuthSessionEngine())
           ->loadUserForSession(PhabricatorAuthSession::TYPE_WEB, $phsid);

src/applications/people/storage/PhabricatorUserLog.php

@@ -66,7 +66,9 @@ final class PhabricatorUserLog extends PhabricatorUserDAO {
       $this->remoteAddr = idx($_SERVER, 'REMOTE_ADDR', '');
     }
     if (!$this->session) {
-      $this->setSession(idx($_COOKIE, 'phsid'));
+      // TODO: This is not correct if there's a cookie prefix. This object
+      // should take an AphrontRequest.
+      $this->setSession(idx($_COOKIE, PhabricatorCookies::COOKIE_SESSION));
     }
     $this->details['host'] = php_uname('n');
     $this->details['user_agent'] = AphrontRequest::getHTTPHeader('User-Agent');

src/applications/settings/panel/PhabricatorSettingsPanelSessions.php

@@ -40,11 +40,8 @@ final class PhabricatorSettingsPanelSessions
       ->withPHIDs($identity_phids)
       ->execute();
 
-    // TODO: Once this has a real ID column, use that instead.
+    $current_key = PhabricatorHash::digest(
-    $sessions = msort($sessions, 'getSessionStart');
+      $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
-    $sessions = array_reverse($sessions);
-
-    $current_key = PhabricatorHash::digest($request->getCookie('phsid'));
 
     $rows = array();
     $rowc = array();