diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fc75e65838..b5426ac82e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -132,7 +132,6 @@ phutil_register_library_map(array( 'AphrontApplicationConfiguration' => 'aphront/configuration/AphrontApplicationConfiguration.php', 'AphrontBarView' => 'view/widget/bars/AphrontBarView.php', 'AphrontBoolHTTPParameterType' => 'aphront/httpparametertype/AphrontBoolHTTPParameterType.php', - 'AphrontCSRFException' => 'aphront/exception/AphrontCSRFException.php', 'AphrontCalendarEventView' => 'applications/calendar/view/AphrontCalendarEventView.php', 'AphrontController' => 'aphront/AphrontController.php', 'AphrontCursorPagerView' => 'view/control/AphrontCursorPagerView.php', @@ -4566,7 +4565,6 @@ phutil_register_library_map(array( 'AphrontApplicationConfiguration' => 'Phobject', 'AphrontBarView' => 'AphrontView', 'AphrontBoolHTTPParameterType' => 'AphrontHTTPParameterType', - 'AphrontCSRFException' => 'AphrontException', 'AphrontCalendarEventView' => 'AphrontView', 'AphrontController' => 'Phobject', 'AphrontCursorPagerView' => 'AphrontView', diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index a49821e10b..45b383c839 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -261,25 +261,30 @@ final class AphrontRequest extends Phobject { // 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. - $more_info = array(); + $info = array(); + + $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.'); if ($this->isAjax()) { - $more_info[] = pht('This was an Ajax request.'); + $info[] = pht('This was an Ajax request.'); } else { - $more_info[] = pht('This was a Web request.'); + $info[] = pht('This was a Web request.'); } if ($token) { - $more_info[] = pht('This request had an invalid CSRF token.'); + $info[] = pht('This request had an invalid CSRF token.'); } else { - $more_info[] = pht('This request had no CSRF token.'); + $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')) { // TODO: Clean this up, see T1921. - $more_info[] = pht( + $info[] = pht( "To avoid this error, use %s to construct forms. If you are already ". "using %s, make sure the form 'action' uses a relative URI (i.e., ". "begins with a '%s'). Forms using absolute URIs do not include CSRF ". @@ -299,16 +304,16 @@ final class AphrontRequest extends Phobject { 'setRenderAsForm(true)'); } + $message = implode("\n", $info); + // This should only be able to happen if you load a form, pull your // internet for 6 hours, and then reconnect and immediately submit, // but give the user some indication of what happened since the workflow // is incredibly confusing otherwise. - throw new AphrontCSRFException( - 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". - implode("\n", $more_info)); + throw new AphrontMalformedRequestException( + pht('Invalid Request (CSRF)'), + $message, + true); } return true; @@ -480,7 +485,8 @@ final class AphrontRequest extends Phobject { $configured_as = PhabricatorEnv::getEnvConfig('phabricator.base-uri'); $accessed_as = $this->getHost(); - throw new Exception( + throw new AphrontMalformedRequestException( + pht('Bad Host Header'), pht( 'This Phabricator install is configured as "%s", but you are '. 'using the domain name "%s" to access a page which is trying to '. @@ -488,7 +494,8 @@ final class AphrontRequest extends Phobject { 'domain or a configured alternate domain. Phabricator will not '. 'set cookies on other domains for security reasons.', $configured_as, - $accessed_as)); + $accessed_as), + true); } $base_domain = $base_domain_uri->getDomain(); diff --git a/src/aphront/exception/AphrontCSRFException.php b/src/aphront/exception/AphrontCSRFException.php deleted file mode 100644 index c7ff45e82f..0000000000 --- a/src/aphront/exception/AphrontCSRFException.php +++ /dev/null @@ -1,3 +0,0 @@ -getViewer($request); - // Always log the unhandled exception. - phlog($ex); + // Some types of uninteresting request exceptions don't get logged, usually + // because they are caused by the background radiation of bot traffic on + // the internet. These include requests with bad CSRF tokens and + // questionable "Host" headers. + $should_log = true; + if ($ex instanceof AphrontMalformedRequestException) { + $should_log = !$ex->getIsUnlogged(); + } + + if ($should_log) { + phlog($ex); + } $class = get_class($ex); $message = $ex->getMessage(); diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index c949764c9a..a3f77618e4 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -464,12 +464,14 @@ abstract class PhabricatorAuthProvider extends Phobject { public function getAuthCSRFCode(AphrontRequest $request) { $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID); if (!strlen($phcid)) { - throw new Exception( + throw new AphrontMalformedRequestException( + pht('Missing Client ID Cookie'), pht( 'Your browser did not submit a "%s" cookie with client state '. 'information in the request. Check that cookies are enabled. '. 'If this problem persists, you may need to clear your cookies.', - PhabricatorCookies::COOKIE_CLIENTID)); + PhabricatorCookies::COOKIE_CLIENTID), + true); } return PhabricatorHash::digest($phcid);