RateLimiter: Fix peek mode

Why:
- Setting the increment to 0 should check the limit without bumping it.
- This was apparently broken by If3e66491306f22650.

What:
- Use LimitBatch::peek if the increment amount is 0

Bug: T381033
Change-Id: Ife76a1976a2063f051f00302e5adaebd701e6367
(cherry picked from commit e09606b3dc44711571cc6cf2d0d11bd7784d0cdd)

3 files changed, 40 insertions, 2 deletions

diff --git a/includes/Permissions/RateLimiter.php b/includes/Permissions/RateLimiter.php
index 82caca856d8c..a0d9cf321a82 100644
--- a/includes/Permissions/RateLimiter.php
+++ b/includes/Permissions/RateLimiter.php
@@ -210,7 +210,8 @@ class RateLimiter {
 
 		$conds = $this->getConditions( $action );
 		$limiter = $this->wrstatsFactory->createRateLimiter( $conds, [ 'limiter', $action ] );
-		$limitBatch = $limiter->createBatch( $incrBy );
+		$peekMode = $incrBy === 0;
+		$limitBatch = $limiter->createBatch( $incrBy ?: 1 );
 		$this->logger->debug( __METHOD__ . ": limiting $action rate for {$user->getName()}" );
 
 		$id = $user->getId();
@@ -311,7 +312,7 @@ class RateLimiter {
 			'ip' => $ip,
 		];
 
-		$batchResult = $limitBatch->tryIncr();
+		$batchResult = $peekMode ? $limitBatch->peek() : $limitBatch->tryIncr();
 		foreach ( $batchResult->getFailedResults() as $type => $result ) {
 			$this->logger->info(
 				'User::pingLimiter: User tripped rate limit',
diff --git a/includes/libs/WRStats/WRStatsReader.php b/includes/libs/WRStats/WRStatsReader.php
index be97fcfb232b..a25f9d28ad7b 100644
--- a/includes/libs/WRStats/WRStatsReader.php
+++ b/includes/libs/WRStats/WRStatsReader.php
@@ -87,6 +87,13 @@ class WRStatsReader {
 				break;
 			}
 		}
+
+		if ( !$seqSpec ) {
+			// This check exists to make Phan happy.
+			// It should never fail since we apply normalization in MetricSpec::__construct()
+			throw new WRStatsError( 'There should have been at least one sequence' );
+		}
+
 		$timeStep = $seqSpec->timeStep;
 		$firstBucket = (int)( $range->start / $timeStep );
 		$lastBucket = (int)ceil( $range->end / $timeStep );
diff --git a/tests/phpunit/integration/includes/Permissions/RateLimiterTest.php b/tests/phpunit/integration/includes/Permissions/RateLimiterTest.php
index 36d959bb6b6e..f943dce5079e 100644
--- a/tests/phpunit/integration/includes/Permissions/RateLimiterTest.php
+++ b/tests/phpunit/integration/includes/Permissions/RateLimiterTest.php
@@ -484,6 +484,36 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
 	}
 
 	/**
+	 * Test that setting the increment to 0 causes the RateLimiter to operate in
+	 * peek mode, checking a rate limit without setting it.
+	 *
+	 * Regression test for T381033.
+	 */
+	public function testPeek() {
+		$limits = [
+			'edit' => [
+				'user' => [ 1, 60 ],
+			],
+		];
+
+		$user = new RateLimitSubject( new UserIdentityValue( 7, 'Garth' ), '127.0.0.1', [] );
+
+		$limiter = $this->newRateLimiter( $limits, [] );
+
+		// initial peek should pass
+		$this->assertFalse( $limiter->limit( $user, 'edit', 0 ) );
+
+		// check that repeated peeking doesn't trigger the limit
+		$this->assertFalse( $limiter->limit( $user, 'edit', 0 ) );
+
+		// first increment should pass but trigger the limit
+		$this->assertFalse( $limiter->limit( $user, 'edit', 1 ) );
+
+		// peek should fail now
+		$this->assertTrue( $limiter->limit( $user, 'edit', 0 ) );
+	}
+
+	/**
 	 * Test that the most permissive limit is used when a limit is defined for
 	 * multiple groups a user belongs to.
 	 */