From ed33e59c5aa1ad53770366a9721b7cbabdf8c0c1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 19 Aug 2011 11:43:18 -0700
Subject: [PATCH] Fix login issue with stale HTTP vs HTTPS cookies

Summary:
In D758, I tightened the scope for which we issue cookies. Instead of setting
them on the whole domain we set them only on the subdomain, and we set them as
HTTPS only if the install is HTTPS.

However, this can leave the user with a stale HTTP cookie which the browser
sends and which never gets cleared. Handle this situation by:

  - Clear all four <domain, https> pairs when clearing cookies ("nuke it from
orbit").
  - Clear 'phsid' cookies when they're invalid.

Test Plan: Applied a hackier version of this patch to secure.phabricator.com and
was able to login with a stale HTTP cookie.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 838
---
 src/aphront/request/AphrontRequest.php        | 41 +++++++++++--------
 .../controller/base/PhabricatorController.php |  3 ++
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/src/aphront/request/AphrontRequest.php b/src/aphront/request/AphrontRequest.php
index fdb672b7b3..de6bad8356 100644
--- a/src/aphront/request/AphrontRequest.php
+++ b/src/aphront/request/AphrontRequest.php
@@ -204,26 +204,31 @@ class AphrontRequest {
 
     if ($value == '') {
       // NOTE: If we're clearing the cookie, also clear it on the entire
-      // domain. This allows us to clear older cookies which we didn't scope
-      // as tightly.
-      setcookie(
-        $name,
-        $value,
-        $expire,
-        $path = '/',
-        $domain = '',
-        $secure = ($base_protocol == 'https'),
-        $http_only = true);
+      // domain and both HTTP/HTTPS versions. This allows us to clear older
+      // cookies which we didn't scope as tightly. Eventually we could remove
+      // this, although it doesn't really hurt us. Basically, we're just making
+      // really sure that cookies get cleared when we try to clear them.
+      $secure_options = array(true, false);
+      $domain_options = array('', $base_domain);
+    } else {
+      // Otherwise, when setting cookies, set only one tightly-scoped cookie.
+      $is_secure = ($base_protocol == 'https');
+      $secure_options = array($is_secure);
+      $domain_options = array($base_domain);
     }
 
-    setcookie(
-      $name,
-      $value,
-      $expire,
-      $path = '/',
-      $base_domain,
-      $secure = ($base_protocol == 'https'),
-      $http_only = true);
+    foreach ($secure_options as $cookie_secure) {
+      foreach ($domain_options as $cookie_domain) {
+        setcookie(
+          $name,
+          $value,
+          $expire,
+          $path = '/',
+          $cookie_domain,
+          $cookie_secure,
+          $http_only = true);
+      }
+    }
   }
 
   final public function setUser($user) {
diff --git a/src/applications/base/controller/base/PhabricatorController.php b/src/applications/base/controller/base/PhabricatorController.php
index 69b2a0d4f7..d372c4c3ad 100644
--- a/src/applications/base/controller/base/PhabricatorController.php
+++ b/src/applications/base/controller/base/PhabricatorController.php
@@ -50,6 +50,9 @@ abstract class PhabricatorController extends AphrontController {
         $phsid);
       if ($info) {
         $user->loadFromArray($info);
+      } else {
+        // The session cookie is invalid, so clear it.
+        $request->clearCookie('phsid');
       }
     }