From 847b7977c1254e28c2106f03048f849b7f1fedad Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Apr 2014 11:22:38 -0700 Subject: [PATCH] Add semi-generic rate limiting infrastructure Summary: This adds a system which basically keeps a record of recent actions, who took them, and how many "points" they were worth, like: epriestley email.add 1 1233989813 epriestley email.add 1 1234298239 epriestley email.add 1 1238293981 We can use this to rate-limit actions by examining how many actions the user has taken in the past hour (i.e., their total score) and comparing that to an allowed limit. One major thing I want to use this for is to limit the amount of error email we'll send to an email address. A big concern I have with sending more error email is that we'll end up in loops. We have some protections against this in headers already, but hard-limiting the system so it won't send more than a few errors to a particular address per hour should provide a reasonable secondary layer of protection. This use case (where the "actor" needs to be an email address) is why the table uses strings + hashes instead of PHIDs. For external users, it might be appropriate to rate limit by cookies or IPs, too. To prove it works, I rate limited adding email addresses. This is a very, very low-risk security thing where a user with an account can enumerate addresses (by checking if they get an error) and sort of spam/annoy people (by adding their address over and over again). Limiting them to 6 actions / hour should satisfy all real users while preventing these behaviors. Test Plan: This dialog is uggos but I'll fix that in a sec: {F137406} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D8683 --- .../sql/autopatches/20140402.actionlog.sql | 12 ++ src/__phutil_library_map__.php | 13 ++ ...AphrontDefaultApplicationConfiguration.php | 17 +++ .../PhabricatorSettingsAddEmailAction.php | 20 +++ ...PhabricatorSettingsPanelEmailAddresses.php | 5 + .../system/action/PhabricatorSystemAction.php | 40 ++++++ .../engine/PhabricatorSystemActionEngine.php | 119 ++++++++++++++++++ ...bricatorSystemActionRateLimitException.php | 18 +++ ...habricatorSystemActionGarbageCollector.php | 21 ++++ .../storage/PhabricatorSystemActionLog.php | 22 ++++ .../system/storage/PhabricatorSystemDAO.php | 9 ++ .../patch/PhabricatorBuiltinPatchList.php | 1 + 12 files changed, 297 insertions(+) create mode 100644 resources/sql/autopatches/20140402.actionlog.sql create mode 100644 src/applications/settings/action/PhabricatorSettingsAddEmailAction.php create mode 100644 src/applications/system/action/PhabricatorSystemAction.php create mode 100644 src/applications/system/engine/PhabricatorSystemActionEngine.php create mode 100644 src/applications/system/exception/PhabricatorSystemActionRateLimitException.php create mode 100644 src/applications/system/garbagecollector/PhabricatorSystemActionGarbageCollector.php create mode 100644 src/applications/system/storage/PhabricatorSystemActionLog.php create mode 100644 src/applications/system/storage/PhabricatorSystemDAO.php diff --git a/resources/sql/autopatches/20140402.actionlog.sql b/resources/sql/autopatches/20140402.actionlog.sql new file mode 100644 index 0000000000..057542e92b --- /dev/null +++ b/resources/sql/autopatches/20140402.actionlog.sql @@ -0,0 +1,12 @@ +CREATE TABLE {$NAMESPACE}_system.system_actionlog ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + actorHash CHAR(12) NOT NULL COLLATE latin1_bin, + actorIdentity VARCHAR(255) NOT NULL COLLATE utf8_bin, + action CHAR(32) NOT NULL COLLATE utf8_bin, + score DOUBLE NOT NULL, + epoch INT UNSIGNED NOT NULL, + + KEY `key_epoch` (epoch), + KEY `key_action` (actorHash, action, epoch) + +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b1c6df7ff1..37e97aea66 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2043,6 +2043,7 @@ phutil_register_library_map(array( 'PhabricatorSearchWorker' => 'applications/search/worker/PhabricatorSearchWorker.php', 'PhabricatorSecurityConfigOptions' => 'applications/config/option/PhabricatorSecurityConfigOptions.php', 'PhabricatorSendGridConfigOptions' => 'applications/config/option/PhabricatorSendGridConfigOptions.php', + 'PhabricatorSettingsAddEmailAction' => 'applications/settings/action/PhabricatorSettingsAddEmailAction.php', 'PhabricatorSettingsAdjustController' => 'applications/settings/controller/PhabricatorSettingsAdjustController.php', 'PhabricatorSettingsMainController' => 'applications/settings/controller/PhabricatorSettingsMainController.php', 'PhabricatorSettingsPanel' => 'applications/settings/panel/PhabricatorSettingsPanel.php', @@ -2142,6 +2143,12 @@ phutil_register_library_map(array( 'PhabricatorSymbolNameLinter' => 'infrastructure/lint/hook/PhabricatorSymbolNameLinter.php', 'PhabricatorSyntaxHighlighter' => 'infrastructure/markup/PhabricatorSyntaxHighlighter.php', 'PhabricatorSyntaxHighlightingConfigOptions' => 'applications/config/option/PhabricatorSyntaxHighlightingConfigOptions.php', + 'PhabricatorSystemAction' => 'applications/system/action/PhabricatorSystemAction.php', + 'PhabricatorSystemActionEngine' => 'applications/system/engine/PhabricatorSystemActionEngine.php', + 'PhabricatorSystemActionGarbageCollector' => 'applications/system/garbagecollector/PhabricatorSystemActionGarbageCollector.php', + 'PhabricatorSystemActionLog' => 'applications/system/storage/PhabricatorSystemActionLog.php', + 'PhabricatorSystemActionRateLimitException' => 'applications/system/exception/PhabricatorSystemActionRateLimitException.php', + 'PhabricatorSystemDAO' => 'applications/system/storage/PhabricatorSystemDAO.php', 'PhabricatorTaskmasterDaemon' => 'infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php', 'PhabricatorTestCase' => 'infrastructure/testing/PhabricatorTestCase.php', 'PhabricatorTestController' => 'applications/base/controller/__tests__/PhabricatorTestController.php', @@ -4906,6 +4913,7 @@ phutil_register_library_map(array( 'PhabricatorSearchWorker' => 'PhabricatorWorker', 'PhabricatorSecurityConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorSendGridConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorSettingsAddEmailAction' => 'PhabricatorSystemAction', 'PhabricatorSettingsAdjustController' => 'PhabricatorController', 'PhabricatorSettingsMainController' => 'PhabricatorController', 'PhabricatorSettingsPanelAccount' => 'PhabricatorSettingsPanel', @@ -5006,6 +5014,11 @@ phutil_register_library_map(array( 'PhabricatorSubscriptionsUIEventListener' => 'PhabricatorEventListener', 'PhabricatorSymbolNameLinter' => 'ArcanistXHPASTLintNamingHook', 'PhabricatorSyntaxHighlightingConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorSystemActionEngine' => 'Phobject', + 'PhabricatorSystemActionGarbageCollector' => 'PhabricatorGarbageCollector', + 'PhabricatorSystemActionLog' => 'PhabricatorSystemDAO', + 'PhabricatorSystemActionRateLimitException' => 'Exception', + 'PhabricatorSystemDAO' => 'PhabricatorLiskDAO', 'PhabricatorTaskmasterDaemon' => 'PhabricatorDaemon', 'PhabricatorTestCase' => 'ArcanistPhutilTestCase', 'PhabricatorTestController' => 'PhabricatorController', diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index aea5d09e20..e81f77294f 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -111,6 +111,23 @@ class AphrontDefaultApplicationConfiguration $user = new PhabricatorUser(); } + if ($ex instanceof PhabricatorSystemActionRateLimitException) { + $error_view = id(new AphrontErrorView()) + ->setErrors(array(pht('You are being rate limited.'))); + + $dialog = id(new AphrontDialogView()) + ->setTitle(pht('Slow Down!')) + ->setUser($user) + ->appendChild($error_view) + ->appendParagraph($ex->getMessage()) + ->appendParagraph($ex->getRateExplanation()) + ->addCancelButton('/', pht('Okaaaaaaaaaaaaaay...')); + + $response = new AphrontDialogResponse(); + $response->setDialog($dialog); + return $response; + } + if ($ex instanceof PhabricatorPolicyException) { if (!$user->isLoggedIn()) { diff --git a/src/applications/settings/action/PhabricatorSettingsAddEmailAction.php b/src/applications/settings/action/PhabricatorSettingsAddEmailAction.php new file mode 100644 index 0000000000..764db7f543 --- /dev/null +++ b/src/applications/settings/action/PhabricatorSettingsAddEmailAction.php @@ -0,0 +1,20 @@ +setURI($uri); } + PhabricatorSystemActionEngine::willTakeAction( + array($user->getPHID()), + new PhabricatorSettingsAddEmailAction(), + 1); + if (!strlen($email)) { $e_email = pht('Required'); $errors[] = pht('Email is required.'); diff --git a/src/applications/system/action/PhabricatorSystemAction.php b/src/applications/system/action/PhabricatorSystemAction.php new file mode 100644 index 0000000000..1afe74ceb6 --- /dev/null +++ b/src/applications/system/action/PhabricatorSystemAction.php @@ -0,0 +1,40 @@ + $this->getScoreThreshold()); + } + + public function getLimitExplanation() { + return pht('You are performing too many actions too quickly.'); + } + + public function getRateExplanation($score) { + return pht( + 'The maximum allowed rate for this action is %s. You are taking '. + 'actions at a rate of %s.', + $this->formatRate($this->getScoreThreshold()), + $this->formatRate($score)); + } + + protected function formatRate($rate) { + if ($rate > 10) { + $str = pht('%d / second', $rate); + } else { + $rate *= 60; + if ($rate > 10) { + $str = pht('%d / minute', $rate); + } else { + $rate *= 60; + $str = pht('%d / hour', $rate); + } + } + + return phutil_tag('strong', array(), $str); + } + +} diff --git a/src/applications/system/engine/PhabricatorSystemActionEngine.php b/src/applications/system/engine/PhabricatorSystemActionEngine.php new file mode 100644 index 0000000000..4b38cf4840 --- /dev/null +++ b/src/applications/system/engine/PhabricatorSystemActionEngine.php @@ -0,0 +1,119 @@ += 0) { + $blocked = self::loadBlockedActors($actors, $action, $score); + if ($blocked) { + foreach ($blocked as $actor => $actor_score) { + throw new PhabricatorSystemActionRateLimitException( + $action, + $actor_score + ($score / self::getWindow())); + } + } + } + + self::recordAction($actors, $action, $score); + } + + public static function loadBlockedActors( + array $actors, + PhabricatorSystemAction $action) { + + $scores = self::loadScores($actors, $action); + + $blocked = array(); + foreach ($scores as $actor => $score) { + if ($action->shouldBlockActor($actor, $score)) { + $blocked[$actor] = $score; + } + } + + return $blocked; + } + + public static function loadScores( + array $actors, + PhabricatorSystemAction $action) { + + if (!$actors) { + return array(); + } + + $actor_hashes = array(); + foreach ($actors as $actor) { + $actor_hashes[] = PhabricatorHash::digestForIndex($actor); + } + + $log = new PhabricatorSystemActionLog(); + + $window = self::getWindow(); + + $conn_r = $log->establishConnection('r'); + $scores = queryfx_all( + $conn_r, + 'SELECT actorIdentity, SUM(score) totalScore FROM %T + WHERE action = %s AND actorHash IN (%Ls) + AND epoch >= %d GROUP BY actorHash', + $log->getTableName(), + $action->getActionConstant(), + $actor_hashes, + (time() - $window)); + + $scores = ipull($scores, 'totalScore', 'actorIdentity'); + + foreach ($scores as $key => $score) { + $scores[$key] = $score / $window; + } + + $scores = $scores + array_fill_keys($actors, 0); + + return $scores; + } + + private static function recordAction( + array $actors, + PhabricatorSystemAction $action, + $score) { + + $log = new PhabricatorSystemActionLog(); + $conn_w = $log->establishConnection('w'); + + $sql = array(); + foreach ($actors as $actor) { + $sql[] = qsprintf( + $conn_w, + '(%s, %s, %s, %f, %d)', + PhabricatorHash::digestForIndex($actor), + $actor, + $action->getActionConstant(), + $score, + time()); + } + + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn_w, + 'INSERT INTO %T (actorHash, actorIdentity, action, score, epoch) + VALUES %Q', + $log->getTableName(), + $chunk); + } + } + + private static function getWindow() { + // Limit queries to the last hour of data so we don't need to look at as + // many rows. We can use an arbitrarily larger window instead (we normalize + // scores to actions per second) but all the actions we care about limiting + // have a limit much higher than one action per hour. + return phutil_units('1 hour in seconds'); + } + +} diff --git a/src/applications/system/exception/PhabricatorSystemActionRateLimitException.php b/src/applications/system/exception/PhabricatorSystemActionRateLimitException.php new file mode 100644 index 0000000000..f75d800fcc --- /dev/null +++ b/src/applications/system/exception/PhabricatorSystemActionRateLimitException.php @@ -0,0 +1,18 @@ +action = $action; + $this->score = $score; + parent::__construct($action->getLimitExplanation()); + } + + public function getRateExplanation() { + return $this->action->getRateExplanation($this->score); + } + +} diff --git a/src/applications/system/garbagecollector/PhabricatorSystemActionGarbageCollector.php b/src/applications/system/garbagecollector/PhabricatorSystemActionGarbageCollector.php new file mode 100644 index 0000000000..9b9728ac8c --- /dev/null +++ b/src/applications/system/garbagecollector/PhabricatorSystemActionGarbageCollector.php @@ -0,0 +1,21 @@ +establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE epoch < %d LIMIT 100', + $table->getTableName(), + time() - $ttl); + + return ($conn_w->getAffectedRows() == 100); + } + +} diff --git a/src/applications/system/storage/PhabricatorSystemActionLog.php b/src/applications/system/storage/PhabricatorSystemActionLog.php new file mode 100644 index 0000000000..c7f76b7ba1 --- /dev/null +++ b/src/applications/system/storage/PhabricatorSystemActionLog.php @@ -0,0 +1,22 @@ + false, + ) + parent::getConfiguration(); + } + + public function setActorIdentity($identity) { + $this->setActorHash(PhabricatorHash::digestForIndex($identity)); + return parent::setActorIdentity($identity); + } + +} diff --git a/src/applications/system/storage/PhabricatorSystemDAO.php b/src/applications/system/storage/PhabricatorSystemDAO.php new file mode 100644 index 0000000000..ab4ae84ae1 --- /dev/null +++ b/src/applications/system/storage/PhabricatorSystemDAO.php @@ -0,0 +1,9 @@ + array(), 'db.phragment' => array(), 'db.dashboard' => array(), + 'db.system' => array(), '0000.legacy.sql' => array( 'legacy' => 0, ),