Index: src/aphront/AphrontRequest.php =================================================================== --- src/aphront/AphrontRequest.php +++ src/aphront/AphrontRequest.php @@ -212,22 +212,25 @@ // Add some diagnostic details so we can figure out if some CSRF issues // are JS problems or people accessing Ajax URIs directly with their // browsers. - if ($token) { - $token_info = "with an invalid CSRF token"; + $more_info = array(); + + if ($this->isAjax()) { + $more_info[] = pht('This was an Ajax request.'); } else { - $token_info = "without a CSRF token"; + $more_info[] = pht('This was a Web request.'); } - if ($this->isAjax()) { - $more_info = "(This was an Ajax request, {$token_info}.)"; + if ($token) { + $more_info[] = pht('This request had an invalid CSRF token.'); } else { - $more_info = "(This was a web request, {$token_info}.)"; + $more_info[] = pht('This request had no CSRF token.'); } // Give a more detailed explanation of how to avoid the exception // in developer mode. if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { - $more_info = $more_info . + // TODO: Clean this up, see T1921. + $more_info[] = "To avoid this error, use phabricator_form() to construct forms. " . "If you are already using phabricator_form(), make sure the form " . "'action' uses a relative URI (i.e., begins with a '/'). Forms " . @@ -249,14 +252,11 @@ // but give the user some indication of what happened since the workflow // is incredibly confusing otherwise. throw new AphrontCSRFException( - "The form you just submitted did not include a valid CSRF token. ". - "This token is a technical security measure which prevents a ". - "certain type of login hijacking attack. However, the token can ". - "become invalid if you leave a page open for more than six hours ". - "without a connection to the internet. To fix this problem: reload ". - "the page, and then resubmit it. All data inserted to the form will ". - "be lost in some browsers so copy them somewhere before reloading.\n\n". - $more_info); + pht( + "You are trying to save some data to Phabricator, but the request ". + "your browser made included an incorrect token. Reload the page ". + "and try again. You may need to clear your cookies.\n\n%s", + implode("\n", $more_info))); } return true; Index: src/applications/auth/provider/PhabricatorAuthProviderPassword.php =================================================================== --- src/applications/auth/provider/PhabricatorAuthProviderPassword.php +++ src/applications/auth/provider/PhabricatorAuthProviderPassword.php @@ -185,14 +185,15 @@ } if (!$account) { - $log = PhabricatorUserLog::initializeNewLog( - null, - $log_user ? $log_user->getPHID() : null, - PhabricatorUserLog::ACTION_LOGIN_FAILURE); - $log->save(); + if ($request->isFormPost()) { + $log = PhabricatorUserLog::initializeNewLog( + null, + $log_user ? $log_user->getPHID() : null, + PhabricatorUserLog::ACTION_LOGIN_FAILURE); + $log->save(); + } $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME); - $request->clearCookie(PhabricatorCookies::COOKIE_SESSION); $response = $controller->buildProviderPageResponse( $this, Index: src/applications/base/controller/PhabricatorController.php =================================================================== --- src/applications/base/controller/PhabricatorController.php +++ src/applications/base/controller/PhabricatorController.php @@ -34,14 +34,27 @@ $user = $request->getUser(); } else { $user = new PhabricatorUser(); + $session_engine = new PhabricatorAuthSessionEngine(); $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION); if (strlen($phsid)) { - $session_user = id(new PhabricatorAuthSessionEngine()) - ->loadUserForSession(PhabricatorAuthSession::TYPE_WEB, $phsid); + $session_user = $session_engine->loadUserForSession( + PhabricatorAuthSession::TYPE_WEB, + $phsid); if ($session_user) { $user = $session_user; } + } else { + // If the client doesn't have a session token, generate an anonymous + // session. This is used to provide CSRF protection to logged-out users. + $phsid = $session_engine->establishSession( + PhabricatorAuthSession::TYPE_WEB, + null); + $request->setCookie(PhabricatorCookies::COOKIE_SESSION, $phsid); + } + + if (!$user->isLoggedIn()) { + $user->attachAlternateCSRFString(PhabricatorHash::digest($phsid)); } $request->setUser($user); Index: src/applications/people/storage/PhabricatorUser.php =================================================================== --- src/applications/people/storage/PhabricatorUser.php +++ src/applications/people/storage/PhabricatorUser.php @@ -39,6 +39,8 @@ private $omnipotent = false; private $customFields = self::ATTACHABLE; + private $alternateCSRFString = self::ATTACHABLE; + protected function readField($field) { switch ($field) { case 'timezoneIdentifier': @@ -217,12 +219,7 @@ } public function validateCSRFToken($token) { - if (!$this->getPHID()) { - return true; - } - $salt = null; - $version = 'plain'; // This is a BREACH-mitigating token. See T3684. @@ -287,8 +284,15 @@ } private function generateToken($epoch, $frequency, $key, $len) { + if ($this->getPHID()) { + $vec = $this->getPHID().$this->getPasswordHash(); + } else { + $vec = $this->getAlternateCSRFString(); + } + $time_block = floor($epoch / $frequency); - $vec = $this->getPHID().$this->getPasswordHash().$key.$time_block; + $vec = $vec.$key.$time_block; + return substr(PhabricatorHash::digest($vec), 0, $len); } @@ -439,6 +443,15 @@ } } + public function getAlternateCSRFString() { + return $this->assertAttached($this->alternateCSRFString); + } + + public function attachAlternateCSRFString($string) { + $this->alternateCSRFString = $string; + return $this; + } + private static function tokenizeName($name) { if (function_exists('mb_strtolower')) { $name = mb_strtolower($name, 'UTF-8'); Index: src/view/page/PhabricatorStandardPageView.php =================================================================== --- src/view/page/PhabricatorStandardPageView.php +++ src/view/page/PhabricatorStandardPageView.php @@ -163,17 +163,6 @@ Javelin::initBehavior('history-install'); Javelin::initBehavior('phabricator-gesture'); - // If the client doesn't have a session token, generate an anonymous - // session. This is used to provide CSRF protection to logged-out users. - $session_token = $request->getCookie(PhabricatorCookies::COOKIE_SESSION); - if (!strlen($session_token)) { - $anonymous_session = id(new PhabricatorAuthSessionEngine()) - ->establishSession('web', null); - $request->setCookie( - PhabricatorCookies::COOKIE_SESSION, - $anonymous_session); - } - $current_token = null; if ($user) { $current_token = $user->getCSRFToken();