After writing "next_uri", don't write it again for a while

Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:

  - User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
  - User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).

To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.

The comment in the body text describes this in more detail.

This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.

Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3793

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

src/applications/auth/constants/PhabricatorCookies.php

@@ -3,6 +3,8 @@
 /**
  * Consolidates Phabricator application cookies, including registration
  * and session management.
+ *
+ * @task next Next URI Cookie
  */
 final class PhabricatorCookies extends Phobject {
 
@@ -45,4 +47,84 @@ final class PhabricatorCookies extends Phobject {
    */
   const COOKIE_NEXTURI        = 'next_uri';
 
+
+/* -(  Next URI Cookie  )---------------------------------------------------- */
+
+
+  /**
+   * Set the Next URI cookie. We only write the cookie if it wasn't recently
+   * written, to avoid writing over a real URI with a bunch of "humans.txt"
+   * stuff. See T3793 for discussion.
+   *
+   * @param   AphrontRequest    Request to write to.
+   * @param   string            URI to write.
+   * @param   bool              Write this cookie even if we have a fresh
+   *                            cookie already.
+   * @return  void
+   *
+   * @task next
+   */
+  public static function setNextURICookie(
+    AphrontRequest $request,
+    $next_uri,
+    $force = false) {
+
+    if (!$force) {
+      $cookie_value = $request->getCookie(self::COOKIE_NEXTURI);
+      list($set_at, $current_uri) = self::parseNextURICookie($cookie_value);
+
+      // If the cookie was set within the last 2 minutes, don't overwrite it.
+      // Primarily, this prevents browser requests for resources which do not
+      // exist (like "humans.txt" and various icons) from overwriting a normal
+      // URI like "/feed/".
+      if ($set_at > (time() - 120)) {
+        return;
+      }
+    }
+
+    $new_value = time().','.$next_uri;
+    $request->setCookie(self::COOKIE_NEXTURI, $new_value);
+  }
+
+
+  /**
+   * Read the URI out of the Next URI cookie.
+   *
+   * @param   AphrontRequest  Request to examine.
+   * @return  string|null     Next URI cookie's URI value.
+   *
+   * @task next
+   */
+  public static function getNextURICookie(AphrontRequest $request) {
+    $cookie_value = $request->getCookie(self::COOKIE_NEXTURI);
+    list($set_at, $next_uri) = self::parseNExtURICookie($cookie_value);
+
+    return $next_uri;
+  }
+
+
+  /**
+   * Parse a Next URI cookie into its components.
+   *
+   * @param   string        Raw cookie value.
+   * @return  list<string>  List of timestamp and URI.
+   *
+   * @task next
+   */
+  private static function parseNextURICookie($cookie) {
+    // Old cookies look like: /uri
+    // New cookies look like: timestamp,/uri
+
+    if (!strlen($cookie)) {
+      return null;
+    }
+
+    if (strpos($cookie, ',') !== false) {
+      list($timestamp, $uri) = explode(',', $cookie, 2);
+      return array((int)$timestamp, $uri);
+    }
+
+    return array(0, $cookie);
+  }
+
 }

src/applications/auth/controller/PhabricatorAuthStartController.php

@@ -86,9 +86,8 @@ final class PhabricatorAuthStartController
     }
 
     if (!$request->isFormPost()) {
-      $request->setCookie(
-        PhabricatorCookies::COOKIE_NEXTURI,
-        $next_uri);
+      PhabricatorCookies::setNextURICookie($request, $next_uri);
+
       $request->setCookie(
         PhabricatorCookies::COOKIE_CLIENTID,
         Filesystem::readRandomCharacters(16));

src/applications/auth/controller/PhabricatorAuthValidateController.php

@@ -54,7 +54,7 @@ final class PhabricatorAuthValidateController
       return $this->renderErrors($failures);
     }
 
-    $next = $request->getCookie(PhabricatorCookies::COOKIE_NEXTURI);
+    $next = PhabricatorCookies::getNextURICookie($request);
     $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
 
     if (!PhabricatorEnv::isValidLocalWebResource($next)) {

src/applications/auth/controller/PhabricatorEmailTokenController.php

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

src/applications/base/controller/Phabricator404Controller.php

@@ -2,35 +2,6 @@
 
 final class Phabricator404Controller extends PhabricatorController {
 
-  public function shouldRequireLogin() {
-
-    // NOTE: See T2102 for discussion. When a logged-out user requests a page,
-    // we give them a login form and set a `next_uri` cookie so we send them
-    // back to the page they requested after they login. However, some browsers
-    // or extensions request resources which may not exist (like
-    // "apple-touch-icon.png" and "humans.txt") and these requests may overwrite
-    // the stored "next_uri" after the login page loads. Our options for dealing
-    // with this are all bad:
-    //
-    //  1. We can't put the URI in the form because some login methods (OAuth2)
-    //     issue redirects to third-party sites. After T1536 we might be able
-    //     to.
-    //  2. We could set the cookie only if it doesn't exist, but then a user who
-    //     declines to login will end up in the wrong place if they later do
-    //     login.
-    //  3. We can blacklist all the resources browsers request, but this is a
-    //     mess.
-    //  4. We can just allow users to access the 404 page without login, so
-    //     requesting bad URIs doesn't set the cookie.
-    //
-    // This implements (4). The main downside is that an attacker can now detect
-    // if a URI is routable (i.e., some application is installed) by testing for
-    // 404 vs login. If possible, we should implement T1536 in such a way that
-    // we can pass the next URI through the login process.
-
-    return false;
-  }
-
   public function processRequest() {
     return new Aphront404Response();
   }