Page MenuHomePhabricator
Files F14061025

D8537.diff
No OneTemporary
Actions

D8537.diff
View Options

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -287,12 +287,26 @@
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 @@
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 @@
$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
--- 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 @@
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 @@
}
$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
--- a/src/applications/auth/controller/PhabricatorAuthLinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLinkController.php
@@ -79,9 +79,7 @@
$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
--- a/src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -181,7 +181,7 @@
$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
--- a/src/applications/auth/controller/PhabricatorAuthStartController.php
+++ b/src/applications/auth/controller/PhabricatorAuthStartController.php
@@ -30,6 +30,7 @@
// 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 @@
if (!$request->isFormPost()) {
PhabricatorCookies::setNextURICookie($request, $next_uri);
-
- $request->setCookie(
- PhabricatorCookies::COOKIE_CLIENTID,
- Filesystem::readRandomCharacters(16));
+ PhabricatorCookies::setClientIDCookie($request);
}
$not_buttons = array();

File Metadata

Mime Type
text/plain
Expires
Tue, Nov 19, 4:46 AM (6 h, 50 m ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6716860
Default Alt Text
D8537.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility