@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

Convert some whiny exceptions into quiet MalformedRequest exceptions

Summary:
Fixes T11480. This cleans up the error logs a little by quieting three common errors which are really malformed requests:

- The CSRF error happens when bots hit anything which does write checks.
- The "wrong cookie domain" errors happen when bots try to use the `security.alternate-file-domain` to browse stuff like `/auth/start/`.
- The "no phcid" errors happen when bots try to go through the login flow.

All of these are clearly communicated to human users, commonly encountered by bots, and not useful to log.

I collapsed the `CSRFException` type into a standard malformed request exception, since nothing catches it and I can't really come up with a reason why anything would ever care.

Test Plan:
Hit each error through some level of `curl -H ...` and/or fakery. Verified that they showed to users before/after, but no longer log.

Hit some other real errors, verified that they log.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11480

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

+37 -23
-2
src/__phutil_library_map__.php
··· 132 132 'AphrontApplicationConfiguration' => 'aphront/configuration/AphrontApplicationConfiguration.php', 133 133 'AphrontBarView' => 'view/widget/bars/AphrontBarView.php', 134 134 'AphrontBoolHTTPParameterType' => 'aphront/httpparametertype/AphrontBoolHTTPParameterType.php', 135 - 'AphrontCSRFException' => 'aphront/exception/AphrontCSRFException.php', 136 135 'AphrontCalendarEventView' => 'applications/calendar/view/AphrontCalendarEventView.php', 137 136 'AphrontController' => 'aphront/AphrontController.php', 138 137 'AphrontCursorPagerView' => 'view/control/AphrontCursorPagerView.php', ··· 4566 4565 'AphrontApplicationConfiguration' => 'Phobject', 4567 4566 'AphrontBarView' => 'AphrontView', 4568 4567 'AphrontBoolHTTPParameterType' => 'AphrontHTTPParameterType', 4569 - 'AphrontCSRFException' => 'AphrontException', 4570 4568 'AphrontCalendarEventView' => 'AphrontView', 4571 4569 'AphrontController' => 'Phobject', 4572 4570 'AphrontCursorPagerView' => 'AphrontView',
+21 -14
src/aphront/AphrontRequest.php
··· 261 261 // Add some diagnostic details so we can figure out if some CSRF issues 262 262 // are JS problems or people accessing Ajax URIs directly with their 263 263 // browsers. 264 - $more_info = array(); 264 + $info = array(); 265 + 266 + $info[] = pht( 267 + 'You are trying to save some data to Phabricator, but the request '. 268 + 'your browser made included an incorrect token. Reload the page '. 269 + 'and try again. You may need to clear your cookies.'); 265 270 266 271 if ($this->isAjax()) { 267 - $more_info[] = pht('This was an Ajax request.'); 272 + $info[] = pht('This was an Ajax request.'); 268 273 } else { 269 - $more_info[] = pht('This was a Web request.'); 274 + $info[] = pht('This was a Web request.'); 270 275 } 271 276 272 277 if ($token) { 273 - $more_info[] = pht('This request had an invalid CSRF token.'); 278 + $info[] = pht('This request had an invalid CSRF token.'); 274 279 } else { 275 - $more_info[] = pht('This request had no CSRF token.'); 280 + $info[] = pht('This request had no CSRF token.'); 276 281 } 277 282 278 283 // Give a more detailed explanation of how to avoid the exception 279 284 // in developer mode. 280 285 if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { 281 286 // TODO: Clean this up, see T1921. 282 - $more_info[] = pht( 287 + $info[] = pht( 283 288 "To avoid this error, use %s to construct forms. If you are already ". 284 289 "using %s, make sure the form 'action' uses a relative URI (i.e., ". 285 290 "begins with a '%s'). Forms using absolute URIs do not include CSRF ". ··· 299 304 'setRenderAsForm(true)'); 300 305 } 301 306 307 + $message = implode("\n", $info); 308 + 302 309 // This should only be able to happen if you load a form, pull your 303 310 // internet for 6 hours, and then reconnect and immediately submit, 304 311 // but give the user some indication of what happened since the workflow 305 312 // is incredibly confusing otherwise. 306 - throw new AphrontCSRFException( 307 - pht( 308 - 'You are trying to save some data to Phabricator, but the request '. 309 - 'your browser made included an incorrect token. Reload the page '. 310 - 'and try again. You may need to clear your cookies.')."\n\n". 311 - implode("\n", $more_info)); 313 + throw new AphrontMalformedRequestException( 314 + pht('Invalid Request (CSRF)'), 315 + $message, 316 + true); 312 317 } 313 318 314 319 return true; ··· 480 485 $configured_as = PhabricatorEnv::getEnvConfig('phabricator.base-uri'); 481 486 $accessed_as = $this->getHost(); 482 487 483 - throw new Exception( 488 + throw new AphrontMalformedRequestException( 489 + pht('Bad Host Header'), 484 490 pht( 485 491 'This Phabricator install is configured as "%s", but you are '. 486 492 'using the domain name "%s" to access a page which is trying to '. ··· 488 494 'domain or a configured alternate domain. Phabricator will not '. 489 495 'set cookies on other domains for security reasons.', 490 496 $configured_as, 491 - $accessed_as)); 497 + $accessed_as), 498 + true); 492 499 } 493 500 494 501 $base_domain = $base_domain_uri->getDomain();
-3
src/aphront/exception/AphrontCSRFException.php
··· 1 - <?php 2 - 3 - final class AphrontCSRFException extends AphrontException {}
+12 -2
src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php
··· 28 28 29 29 $viewer = $this->getViewer($request); 30 30 31 - // Always log the unhandled exception. 32 - phlog($ex); 31 + // Some types of uninteresting request exceptions don't get logged, usually 32 + // because they are caused by the background radiation of bot traffic on 33 + // the internet. These include requests with bad CSRF tokens and 34 + // questionable "Host" headers. 35 + $should_log = true; 36 + if ($ex instanceof AphrontMalformedRequestException) { 37 + $should_log = !$ex->getIsUnlogged(); 38 + } 39 + 40 + if ($should_log) { 41 + phlog($ex); 42 + } 33 43 34 44 $class = get_class($ex); 35 45 $message = $ex->getMessage();
+4 -2
src/applications/auth/provider/PhabricatorAuthProvider.php
··· 464 464 public function getAuthCSRFCode(AphrontRequest $request) { 465 465 $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID); 466 466 if (!strlen($phcid)) { 467 - throw new Exception( 467 + throw new AphrontMalformedRequestException( 468 + pht('Missing Client ID Cookie'), 468 469 pht( 469 470 'Your browser did not submit a "%s" cookie with client state '. 470 471 'information in the request. Check that cookies are enabled. '. 471 472 'If this problem persists, you may need to clear your cookies.', 472 - PhabricatorCookies::COOKIE_CLIENTID)); 473 + PhabricatorCookies::COOKIE_CLIENTID), 474 + true); 473 475 } 474 476 475 477 return PhabricatorHash::digest($phcid);