Tune cookie behaviors for 'phcid', 'phreg', etc

Summary:
Fixes T3471. Specific issues:

  - Add the ability to set a temporary cookie (expires when the browser closes).
  - We overwrote 'phcid' on every page load. This creates some issues with browser extensions. Instead, only write it if isn't set. To counterbalance this, make it temporary.
  - Make the 'next_uri' cookie temporary.
  - Make the 'phreg' cookie temporary.
  - Fix an issue where deleted cookies would persist after 302 (?) in some cases (this is/was 100% for me locally).

Test Plan:
  - Closed my browser, reopned it, verified temporary cookies were gone.
  - Logged in, authed, linked, logged out.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3471

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

src/aphront/AphrontRequest.php

@@ -287,12 +287,26 @@ final class AphrontRequest {
 
   final public function getCookie($name, $default = null) {
     $name = $this->getPrefixedCookieName($name);
-    return idx($_COOKIE, $name, $default);
+    $value = idx($_COOKIE, $name, $default);
+
+    // Internally, PHP deletes cookies by setting them to the value 'deleted'
+    // with an expiration date in the past.
+
+    // At least in Safari, the browser may send this cookie anyway in some
+    // circumstances. After logging out, the 302'd GET to /login/ consistently
+    // includes deleted cookies on my local install. If a cookie value is
+    // literally 'deleted', pretend it does not exist.
+
+    if ($value === 'deleted') {
+      return null;
+    }
+
+    return $value;
   }
 
   final public function clearCookie($name) {
     $name = $this->getPrefixedCookieName($name);
-    $this->setCookie($name, '', time() - (60 * 60 * 24 * 30));
+    $this->setCookieWithExpiration($name, '', time() - (60 * 60 * 24 * 30));
     unset($_COOKIE[$name]);
   }
 
@@ -348,7 +362,51 @@ final class AphrontRequest {
     return (bool)$this->getCookieDomainURI();
   }
 
-  final public function setCookie($name, $value, $expire = null) {
+
+  /**
+   * Set a cookie which does not expire for a long time.
+   *
+   * To set a temporary cookie, see @{method:setTemporaryCookie}.
+   *
+   * @param string  Cookie name.
+   * @param string  Cookie value.
+   * @return this
+   * @task cookie
+   */
+  final public function setCookie($name, $value) {
+    $far_future = time() + (60 * 60 * 24 * 365 * 5);
+    return $this->setCookieWithExpiration($name, $value, $far_future);
+  }
+
+
+  /**
+   * Set a cookie which expires soon.
+   *
+   * To set a durable cookie, see @{method:setCookie}.
+   *
+   * @param string  Cookie name.
+   * @param string  Cookie value.
+   * @return this
+   * @task cookie
+   */
+  final public function setTemporaryCookie($name, $value) {
+    return $this->setCookieWithExpiration($name, $value, 0);
+  }
+
+
+  /**
+   * Set a cookie with a given expiration policy.
+   *
+   * @param string  Cookie name.
+   * @param string  Cookie value.
+   * @param int     Epoch timestamp for cookie expiration.
+   * @return this
+   * @task cookie
+   */
+  final private function setCookieWithExpiration(
+    $name,
+    $value,
+    $expire) {
 
     $is_secure = false;
 
@@ -371,10 +429,6 @@ final class AphrontRequest {
     $base_domain = $base_domain_uri->getDomain();
     $is_secure = ($base_domain_uri->getProtocol() == 'https');
 
-    if ($expire === null) {
-      $expire = time() + (60 * 60 * 24 * 365 * 5);
-    }
-
     $name = $this->getPrefixedCookieName($name);
 
     if (php_sapi_name() == 'cli') {

src/applications/auth/constants/PhabricatorCookies.php

@@ -4,7 +4,8 @@
  * Consolidates Phabricator application cookies, including registration
  * and session management.
  *
- * @task next Next URI Cookie
+ * @task clientid   Client ID Cookie
+ * @task next       Next URI Cookie
  */
 final class PhabricatorCookies extends Phobject {
 
@@ -48,6 +49,33 @@ final class PhabricatorCookies extends Phobject {
   const COOKIE_NEXTURI        = 'next_uri';
 
 
+/* -(  Client ID Cookie  )--------------------------------------------------- */
+
+
+  /**
+   * Set the client ID cookie. This is a random cookie used like a CSRF value
+   * during authentication workflows.
+   *
+   * @param AphrontRequest  Request to modify.
+   * @return void
+   * @task clientid
+   */
+  public static function setClientIDCookie(AphrontRequest $request) {
+
+    // NOTE: See T3471 for some discussion. Some browsers and browser extensions
+    // can make duplicate requests, so we overwrite this cookie only if it is
+    // not present in the request. The cookie lifetime is limited by making it
+    // temporary and clearing it when users log out.
+
+    $value = $request->getCookie(self::COOKIE_CLIENTID);
+    if (!strlen($value)) {
+      $request->setTemporaryCookie(
+        self::COOKIE_CLIENTID,
+        Filesystem::readRandomCharacters(16));
+    }
+  }
+
+
 /* -(  Next URI Cookie  )---------------------------------------------------- */
 
 
@@ -83,7 +111,7 @@ final class PhabricatorCookies extends Phobject {
     }
 
     $new_value = time().','.$next_uri;
-    $request->setCookie(self::COOKIE_NEXTURI, $new_value);
+    $request->setTemporaryCookie(self::COOKIE_NEXTURI, $new_value);
   }
 
 

src/applications/auth/controller/PhabricatorAuthLinkController.php

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

src/applications/auth/controller/PhabricatorAuthLoginController.php

@@ -181,7 +181,7 @@ final class PhabricatorAuthLoginController
       $account->save();
     unset($unguarded);
 
-    $this->getRequest()->setCookie(
+    $this->getRequest()->setTemporaryCookie(
       PhabricatorCookies::COOKIE_REGISTRATION,
       $registration_key);
 

src/applications/auth/controller/PhabricatorAuthStartController.php

@@ -30,6 +30,7 @@ final class PhabricatorAuthStartController
     // it and warn the user they may need to nuke their cookies.
 
     $session_token = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
+
     if (strlen($session_token)) {
       $kind = PhabricatorAuthSessionEngine::getSessionKindFromToken(
         $session_token);
@@ -87,10 +88,7 @@ final class PhabricatorAuthStartController
 
     if (!$request->isFormPost()) {
       PhabricatorCookies::setNextURICookie($request, $next_uri);
-
+      PhabricatorCookies::setClientIDCookie($request);
-      $request->setCookie(
-        PhabricatorCookies::COOKIE_CLIENTID,
-        Filesystem::readRandomCharacters(16));
     }
 
     $not_buttons = array();