diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -356,6 +356,7 @@ 'rsrc/js/application/conpherence/behavior-pontificate.js' => '53f6f2dd', 'rsrc/js/application/conpherence/behavior-widget-pane.js' => 'd8ef8659', 'rsrc/js/application/countdown/timer.js' => '889c96f3', + 'rsrc/js/application/dashboard/behavior-dashboard-async-panel.js' => '4398eabb', 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'f2441746', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => '533a187b', 'rsrc/js/application/differential/behavior-comment-jump.js' => '71755c79', @@ -546,6 +547,7 @@ 'javelin-behavior-conpherence-widget-pane' => 'd8ef8659', 'javelin-behavior-countdown-timer' => '889c96f3', 'javelin-behavior-dark-console' => 'e9fdb5e5', + 'javelin-behavior-dashboard-async-panel' => '4398eabb', 'javelin-behavior-device' => '03d6ed07', 'javelin-behavior-differential-add-reviewers-and-ccs' => '533a187b', 'javelin-behavior-differential-comment-jump' => '71755c79', @@ -1073,6 +1075,12 @@ 1 => 'javelin-dom', 2 => 'phortune-credit-card-form', ), + '4398eabb' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-dom', + 2 => 'javelin-workflow', + ), '441f2137' => array( 0 => 'javelin-behavior', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1257,6 +1257,7 @@ 'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', 'PhabricatorAuthTerminateSessionController' => 'applications/auth/controller/PhabricatorAuthTerminateSessionController.php', + 'PhabricatorAuthTryFactorAction' => 'applications/auth/action/PhabricatorAuthTryFactorAction.php', 'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php', 'PhabricatorAuthValidateController' => 'applications/auth/controller/PhabricatorAuthValidateController.php', 'PhabricatorAuthenticationConfigOptions' => 'applications/config/option/PhabricatorAuthenticationConfigOptions.php', @@ -4029,6 +4030,7 @@ 'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', 'PhabricatorAuthTerminateSessionController' => 'PhabricatorAuthController', + 'PhabricatorAuthTryFactorAction' => 'PhabricatorSystemAction', 'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController', 'PhabricatorAuthValidateController' => 'PhabricatorAuthController', 'PhabricatorAuthenticationConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/auth/action/PhabricatorAuthTryFactorAction.php b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php @@ -0,0 +1,21 @@ +issueHighSecurityToken($session, true); } + // Check for a rate limit without awarding points, so the user doesn't + // get partway through the workflow only to get blocked. + PhabricatorSystemActionEngine::willTakeAction( + array($viewer->getPHID()), + new PhabricatorAuthTryFactorAction(), + 0); + $validation_results = array(); if ($request->isHTTPPost()) { $request->validateCSRF(); if ($request->getExists(AphrontRequest::TYPE_HISEC)) { + // Limit factor verification rates to prevent brute force attacks. + PhabricatorSystemActionEngine::willTakeAction( + array($viewer->getPHID()), + new PhabricatorAuthTryFactorAction(), + 1); + $ok = true; foreach ($factors as $factor) { $id = $factor->getID(); @@ -268,6 +281,12 @@ } if ($ok) { + // Give the user a credit back for a successful factor verification. + PhabricatorSystemActionEngine::willTakeAction( + array($viewer->getPHID()), + new PhabricatorAuthTryFactorAction(), + -1); + $until = time() + phutil_units('15 minutes in seconds'); $session->setHighSecurityUntil($until); @@ -284,6 +303,9 @@ PhabricatorUserLog::ACTION_ENTER_HISEC); $log->save(); } else { + + + $log = PhabricatorUserLog::initializeNewLog( $viewer, $viewer->getPHID(), diff --git a/src/applications/system/engine/PhabricatorSystemActionEngine.php b/src/applications/system/engine/PhabricatorSystemActionEngine.php --- a/src/applications/system/engine/PhabricatorSystemActionEngine.php +++ b/src/applications/system/engine/PhabricatorSystemActionEngine.php @@ -2,6 +2,42 @@ final class PhabricatorSystemActionEngine extends Phobject { + /** + * Prepare to take an action, throwing an exception if the user has exceeded + * the rate limit. + * + * The `$actors` are a list of strings. Normally this will be a list of + * user PHIDs, but some systems use other identifiers (like email + * addresses). Each actor's score threshold is tracked independently. If + * any actor exceeds the rate limit for the action, this method throws. + * + * The `$action` defines the actual thing being rate limited, and sets the + * limit. + * + * You can pass either a positive, zero, or negative `$score` to this method: + * + * - If the score is positive, the user is given that many points toward + * the rate limit after the limit is checked. Over time, this will cause + * them to hit the rate limit and be prevented from taking further + * actions. + * - If the score is zero, the rate limit is checked but no score changes + * are made. This allows you to check for a rate limit before beginning + * a workflow, so the user doesn't fill in a form only to get rate limited + * at the end. + * - If the score is negative, the user is credited points, allowing them + * to take more actions than the limit normally permits. By awarding + * points for failed actions and credits for successful actions, a + * system can be sensitive to failure without overly restricting + * legitimate uses. + * + * If any actor is exceeding their rate limit, this method throws a + * @{class:PhabricatorSystemActionRateLimitException}. + * + * @param list List of actors. + * @param PhabricatorSystemAction Action being taken. + * @param float Score or credit, see above. + * @return void + */ public static function willTakeAction( array $actors, PhabricatorSystemAction $action, @@ -20,9 +56,11 @@ } } - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - self::recordAction($actors, $action, $score); - unset($unguarded); + if ($score != 0) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + self::recordAction($actors, $action, $score); + unset($unguarded); + } } public static function loadBlockedActors( @@ -35,9 +73,17 @@ $blocked = array(); foreach ($scores as $actor => $actor_score) { - $actor_score = $actor_score + ($score / $window); + // For the purposes of checking for a block, we just use the raw + // persistent score and do not include the score for this action. This + // allows callers to test for a block without adding any points and get + // the same result they would if they were adding points: we only + // trigger a rate limit when the persistent score exceeds the threshold. if ($action->shouldBlockActor($actor, $actor_score)) { - $blocked[$actor] = $actor_score; + // When reporting the results, we do include the points for this + // action. This makes the error messages more clear, since they + // more accurately report the number of actions the user has really + // tried to take. + $blocked[$actor] = $actor_score + ($score / $window); } }