From 559c0fe886633f5bcd4b27dc5700ed6522adfa94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 14 Mar 2014 14:33:31 -0700 Subject: [PATCH] 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 | 68 +++++++++++++++++-- .../auth/constants/PhabricatorCookies.php | 32 ++++++++- .../PhabricatorAuthLinkController.php | 4 +- .../PhabricatorAuthLoginController.php | 2 +- .../PhabricatorAuthStartController.php | 6 +- 5 files changed, 95 insertions(+), 17 deletions(-) diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index 99e051a1ec..8c4b66c3ec 100644 --- a/src/aphront/AphrontRequest.php +++ b/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') { diff --git a/src/applications/auth/constants/PhabricatorCookies.php b/src/applications/auth/constants/PhabricatorCookies.php index 0bb34569f3..ffba1b86e2 100644 --- a/src/applications/auth/constants/PhabricatorCookies.php +++ b/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); } diff --git a/src/applications/auth/controller/PhabricatorAuthLinkController.php b/src/applications/auth/controller/PhabricatorAuthLinkController.php index 44be8adfb3..46edb68126 100644 --- a/src/applications/auth/controller/PhabricatorAuthLinkController.php +++ b/src/applications/auth/controller/PhabricatorAuthLinkController.php @@ -79,9 +79,7 @@ final class PhabricatorAuthLinkController $panel_uri = '/settings/panel/external/'; - $request->setCookie( - PhabricatorCookies::COOKIE_CLIENTID, - Filesystem::readRandomCharacters(16)); + PhabricatorCookies::setClientIDCookie($request); switch ($this->action) { case 'link': diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index af0708b57a..804a2731d2 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/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); diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index 3c1cb7491d..bcb0fef83f 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/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); - - $request->setCookie( - PhabricatorCookies::COOKIE_CLIENTID, - Filesystem::readRandomCharacters(16)); + PhabricatorCookies::setClientIDCookie($request); } $not_buttons = array(); -- 2.51.2