Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

User: enforce pingLimiter() expiry time

Browse source 
This makes User::pingLimiter() include the expiry time in the payload of
the cache key that holds the current count. This allows us to ignore
stale counts.

Until now, we have been relying on the cache implementation to expire
the relevant keys in time. This however seems to fail sometimes.

Bug: T246991
Change-Id: Ifa3c558b4449f1ca133d0064781f26ac1bf59425
This commit is contained in:
daniel 2020-08-26 13:18:46 +02:00 committed by Krinkle
parent 2e6b188f94
commit f50240fe76
4 changed files with 170 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/libs/objectcache/MediumSpecificBagOStuff.php
View file

@ -296,7 +296,7 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
}
// Derive the new value from the old value
$value = call_user_func( $callback, $this, $key, $currentValue, $exptime );
$value = $callback( $this, $key, $currentValue, $exptime );
$keyWasNonexistant = ( $currentValue === false );
$valueMatchesOldValue = ( $value === $currentValue );
unset( $currentValue ); // free RAM in case the value is large

105
includes/user/User.php
View file

@ -1640,6 +1640,8 @@ class User implements IDBAccessObject, UserIdentity {
* @throws MWException
*/
public function pingLimiter( $action = 'edit', $incrBy = 1 ) {
$logger = \MediaWiki\Logger\LoggerFactory::getInstance( 'ratelimit' );
// Call the 'PingLimiter' hook
$result = false;
if ( !$this->getHookRunner()->onPingLimiter( $this, $action, $result, $incrBy ) ) {
@ -1661,6 +1663,8 @@ class User implements IDBAccessObject, UserIdentity {
return false;
}
$logger->debug( __METHOD__ . ": limiting $action rate for {$this->getName()}" );
$keys = [];
$id = $this->getId();
$isNewbie = $this->isNewbie();
@ -1740,7 +1744,7 @@ class User implements IDBAccessObject, UserIdentity {
// that $userLimit is also a bool here.
// @phan-suppress-next-line PhanTypeInvalidExpressionArrayDestructuring
list( $max, $period ) = $userLimit;
wfDebug( __METHOD__ . ": effective user limit: $max in {$period}s" );
$logger->debug( __METHOD__ . ": effective user limit: $max in {$period}s" );
$keys[$cache->makeKey( 'limiter', $action, 'user', $id )] = $userLimit;
}
@ -1768,28 +1772,89 @@ class User implements IDBAccessObject, UserIdentity {
}
}
// XXX: We may want to use $cache->getCurrentTime() here, but that would make it
// harder to test for T246991. Also $cache->getCurrentTime() is documented
// as being for testing only, so it apparently should not be called here.
$now = MWTimestamp::time();
$clockFudge = 3; // avoid log spam when a clock is slightly off
$triggered = false;
foreach ( $keys as $key => $limit ) {
// phan is confused because &can-bypass's value is a bool, so it assumes
// that $userLimit is also a bool here.
// @phan-suppress-next-line PhanTypeInvalidExpressionArrayDestructuring
list( $max, $period ) = $limit;
$summary = "(limit $max in {$period}s)";
$count = $cache->get( $key );
// Already pinged?
if ( $count && $count >= $max ) {
wfDebugLog( 'ratelimit', "User '{$this->getName()}' " .
"(IP {$this->getRequest()->getIP()}) tripped $key at $count $summary" );
$triggered = true;
} else {
wfDebug( __METHOD__ . ": adding record for $key $summary" );
if ( $incrBy > 0 ) {
$cache->add( $key, 0, intval( $period ) ); // first ping
// Do the update in a merge callback, for atomicity.
// To use merge(), we need to explicitly track the desired expiry timestamp.
// This tracking was introduced to investigate T246991. Once it is no longer needed,
// we could go back to incrWithInit(), though that has more potential for race
// conditions between the get() and incrWithInit() calls.
$cache->merge(
$key,
function ( $cache, $key, $data, &$expiry )
use ( $action, $logger, &$triggered, $now, $clockFudge, $limit, $incrBy )
{
// phan is confused because &can-bypass's value is a bool, so it assumes
// that $userLimit is also a bool here.
// @phan-suppress-next-line PhanTypeInvalidExpressionArrayDestructuring
list( $max, $period ) = $limit;
$expiry = $now + (int)$period;
$count = 0;
// Already pinged?
if ( $data ) {
// NOTE: in order to investigate T246991, we write the expiry time
// into the payload, along with the count.
$fields = explode( '|', $data );
$storedCount = (int)( $fields[0] ?? 0 );
$storedExpiry = (int)( $fields[1] ?? PHP_INT_MAX );
// Found a stale entry. This should not happen!
if ( $storedExpiry < ( $now + $clockFudge ) ) {
$logger->info(
'User::pingLimiter: '
. 'Stale rate limit entry, cache key failed to expire (T246991)',
[
'action' => $action,
'user' => $this->getName(),
'limit' => $max,
'period' => $period,
'count' => $storedCount,
'key' => $key,
'expiry' => MWTimestamp::convert( TS_DB, $storedExpiry ),
]
);
} else {
// NOTE: We'll keep the original expiry when bumping counters,
// resulting in a kind of fixed-window throttle.
$expiry = min( $storedExpiry, $now + (int)$period );
$count = $storedCount;
}
}
// Limit exceeded!
if ( $count >= $max ) {
if ( !$triggered ) {
$logger->info(
'User::pingLimiter: User tripped rate limit',
[
'action' => $action,
'user' => $this->getName(),
'ip' => $this->getRequest()->getIP(),
'limit' => $max,
'period' => $period,
'count' => $count,
'key' => $key
]
);
}
$triggered = true;
}
$count += $incrBy;
$data = "$count|$expiry";
return $data;
}
}
if ( $incrBy > 0 ) {
$cache->incrWithInit( $key, (int)$period, $incrBy, $incrBy );
}
);
}
return $triggered;

2
tests/phpunit/includes/libs/objectcache/BagOStuffTestBase.php
View file

@ -71,7 +71,7 @@ abstract class BagOStuffTestBase extends MediaWikiIntegrationTestCase {
$calls = 0;
$casRace = false; // emulate a race
$callback = function ( BagOStuff $cache, $key, $oldVal ) use ( &$calls, &$casRace ) {
$callback = function ( BagOStuff $cache, $key, $oldVal, &$expiry ) use ( &$calls, &$casRace ) {
++$calls;
if ( $casRace ) {
// Uses CAS instead?

84
tests/phpunit/includes/user/UserTest.php
View file

@ -2290,7 +2290,7 @@ class UserTest extends MediaWikiIntegrationTestCase {
/**
* @covers User::pingLimiter
*/
public function testPingLimiter() {
public function testPingLimiterHook() {
$this->setMwGlobals( [
'wgRateLimits' => [
'edit' => [
@ -2333,6 +2333,88 @@ class UserTest extends MediaWikiIntegrationTestCase {
);
}
/**
* @covers User::pingLimiter
*/
public function testPingLimiterWithStaleCache() {
global $wgMainCacheType;
$this->setMwGlobals( [
'wgRateLimits' => [
'edit' => [
'user' => [ 1, 60 ],
],
],
] );
$cacheTime = 1600000000.0;
$appTime = 1600000000;
$cache = new HashBagOStuff();
// TODO: make the main object cache a service we can override, T243233
ObjectCache::$instances[$wgMainCacheType] = $cache;
$cache->setMockTime( $cacheTime ); // this is a reference!
MWTimestamp::setFakeTime( function () use ( &$appTime ) {
return (int)$appTime;
} );
$this->assertFalse( $this->user->pingLimiter(), 'limit not reached' );
$this->assertTrue( $this->user->pingLimiter(), 'limit reached' );
// Make it so that rate limits are expired according to MWTimestamp::time(),
// but not according to $cache->getCurrentTime(), emulating the conditions
// that trigger T246991.
$cacheTime += 10;
$appTime += 100;
$this->assertFalse( $this->user->pingLimiter(), 'limit expired' );
$this->assertTrue( $this->user->pingLimiter(), 'limit functional after expiry' );
}
/**
* @covers User::pingLimiter
*/
public function testPingLimiterRate() {
global $wgMainCacheType;
$this->setMwGlobals( [
'wgRateLimits' => [
'edit' => [
'user' => [ 3, 60 ],
],
],
] );
$fakeTime = 1600000000;
$cache = new HashBagOStuff();
// TODO: make the main object cache a service we can override, T243233
ObjectCache::$instances[$wgMainCacheType] = $cache;
$cache->setMockTime( $fakeTime ); // this is a reference!
MWTimestamp::setFakeTime( function () use ( &$fakeTime ) {
return (int)$fakeTime;
} );
// The limit is 3 per 60 second. Do 5 edits at an emulated 50 second interval.
// They should all pass. This tests that the counter doesn't just keeps increasing
// but gets reset in an appropriate way.
$this->assertFalse( $this->user->pingLimiter(), 'first ping should pass' );
$fakeTime += 50;
$this->assertFalse( $this->user->pingLimiter(), 'second ping should pass' );
$fakeTime += 50;
$this->assertFalse( $this->user->pingLimiter(), 'third ping should pass' );
$fakeTime += 50;
$this->assertFalse( $this->user->pingLimiter(), 'fourth ping should pass' );
$fakeTime += 50;
$this->assertFalse( $this->user->pingLimiter(), 'fifth ping should pass' );
}
private function newFakeUser( $name, $ip, $id ) {
$req = new FauxRequest();
$req->setIP( $ip );