Page MenuHomePhabricator

Add semi-generic rate limiting infrastructure
ClosedPublic

Authored by epriestley on Apr 3 2014, 12:12 AM.
Tags
None
Referenced Files
F13980301: D8683.diff
Sat, Oct 19, 9:41 AM
F13956653: D8683.id.diff
Mon, Oct 14, 6:35 AM
Unknown Object (File)
Oct 8 2024, 10:41 PM
Unknown Object (File)
Oct 5 2024, 5:12 PM
Unknown Object (File)
Oct 5 2024, 5:12 PM
Unknown Object (File)
Oct 5 2024, 5:10 PM
Unknown Object (File)
Oct 5 2024, 5:02 PM
Unknown Object (File)
Sep 15 2024, 2:56 PM
Subscribers

Details

Reviewers
btrahan
Commits
Restricted Diffusion Commit
rP847b7977c125: 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:

Screen_Shot_2014-04-02_at_5.05.25_PM.png (669×893 px, 91 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Add semi-generic rate limiting infrastructure.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.

Sweet. Inline about the mySQL which I think is okay, but just double checking.

src/applications/system/engine/PhabricatorSystemActionEngine.php
63–65

this ends up performing nicely I trust?

This revision is now accepted and ready to land.Apr 3 2014, 5:06 PM
src/applications/system/engine/PhabricatorSystemActionEngine.php
63–65

Yeah -- it can use the (actorHash, action, epoch) key to look at just the rows it needs to examine, and the window on the query prevents that from being a large number of rows:

mysql> explain SELECT actorIdentity, SUM(score) totalScore FROM `system_actionlog` WHERE action = 'email.add' AND actorHash IN ('5Z9iXDrDWIdw') AND epoch >= 1396541619 GROUP BY actorHash;
+----+-------------+------------------+------+----------------------+------------+---------+-------------+------+-------------+
| id | select_type | table            | type | possible_keys        | key        | key_len | ref         | rows | Extra       |
+----+-------------+------------------+------+----------------------+------------+---------+-------------+------+-------------+
|  1 | SIMPLE      | system_actionlog | ref  | key_epoch,key_action | key_action | 108     | const,const |    3 | Using where |
+----+-------------+------------------+------+----------------------+------------+---------+-------------+------+-------------+
1 row in set (0.00 sec)

I wouldn't want to add this all over the place to everything (we'll do cheaper generic rate limiting of requests across the board with T3923) since this is still heavy compared to a purpose-built service for handling it, but we can put it on troublesome/expensive/unusual stuff as the need arises, and generally have an extra tool in our belts for dealing with some of the quasi-security-ish DOS-like stuff that crops up occasionally.

epriestley updated this revision to Diff 20610.

Closed by commit rP847b7977c125 (authored by @epriestley).