diff --git a/src/applications/auth/constants/PhabricatorCookies.php b/src/applications/auth/constants/PhabricatorCookies.php index 19e27a1591..0bb34569f3 100644 --- a/src/applications/auth/constants/PhabricatorCookies.php +++ b/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 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); + } + } diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index 223acaa757..3c1cb7491d 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/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)); diff --git a/src/applications/auth/controller/PhabricatorAuthValidateController.php b/src/applications/auth/controller/PhabricatorAuthValidateController.php index 8454ba24a2..18524163e0 100644 --- a/src/applications/auth/controller/PhabricatorAuthValidateController.php +++ b/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)) { diff --git a/src/applications/auth/controller/PhabricatorEmailTokenController.php b/src/applications/auth/controller/PhabricatorEmailTokenController.php index f1288761be..b4f998113e 100644 --- a/src/applications/auth/controller/PhabricatorEmailTokenController.php +++ b/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); } diff --git a/src/applications/base/controller/Phabricator404Controller.php b/src/applications/base/controller/Phabricator404Controller.php index e47c51fd4b..1aa785a040 100644 --- a/src/applications/base/controller/Phabricator404Controller.php +++ b/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(); }