diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index 484219afa4..64118bbdae 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -212,22 +212,25 @@ final class AphrontRequest { // 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"; - } else { - $token_info = "without a CSRF token"; - } + $more_info = array(); if ($this->isAjax()) { - $more_info = "(This was an Ajax request, {$token_info}.)"; + $more_info[] = pht('This was an Ajax request.'); } else { - $more_info = "(This was a web request, {$token_info}.)"; + $more_info[] = pht('This was a Web request.'); + } + + if ($token) { + $more_info[] = pht('This request had an invalid CSRF token.'); + } else { + $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 @@ final class AphrontRequest { // 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; diff --git a/src/applications/auth/provider/PhabricatorAuthProviderPassword.php b/src/applications/auth/provider/PhabricatorAuthProviderPassword.php index 1ed1706a4f..7b93fde350 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderPassword.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderPassword.php @@ -185,14 +185,15 @@ final class PhabricatorAuthProviderPassword } 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, diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 421a0a30ca..2cb84460b0 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -34,14 +34,27 @@ abstract class PhabricatorController extends AphrontController { $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); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index bb85d064e9..2a76f6481f 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -39,6 +39,8 @@ final class PhabricatorUser private $omnipotent = false; private $customFields = self::ATTACHABLE; + private $alternateCSRFString = self::ATTACHABLE; + protected function readField($field) { switch ($field) { case 'timezoneIdentifier': @@ -217,12 +219,7 @@ final class PhabricatorUser } 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 @@ final class PhabricatorUser } 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 @@ final class PhabricatorUser } } + 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'); diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index d3eeccb28c..d31c3567b8 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -163,17 +163,6 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView { 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();