Merge pull request #7852 from nextcloud/strict_ratelimiting

Make OC\Security\RateLimiting strict

lib/private/Security/RateLimiting/Backend/IBackend.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *
@@ -39,9 +40,9 @@ interface IBackend {
 	 * @param int $seconds Seconds to look back at
 	 * @return int
 	 */
-	public function getAttempts($methodIdentifier,
-								$userIdentifier,
-								$seconds);
+	public function getAttempts(string $methodIdentifier,
+								string $userIdentifier,
+								int $seconds): int;
 
 	/**
 	 * Registers an attempt
@@ -50,7 +51,7 @@ interface IBackend {
 	 * @param string $userIdentifier Identifier for the user
 	 * @param int $period Period in seconds how long this attempt should be stored
 	 */
-	public function registerAttempt($methodIdentifier,
-									$userIdentifier,
-									$period);
+	public function registerAttempt(string $methodIdentifier,
+									string $userIdentifier,
+									int $period);
 }

lib/private/Security/RateLimiting/Backend/MemoryCache.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *
@@ -54,8 +55,8 @@ class MemoryCache implements IBackend {
 	 * @param string $userIdentifier
 	 * @return string
 	 */
-	private function hash($methodIdentifier,
-						  $userIdentifier) {
+	private function hash(string $methodIdentifier,
+						  string $userIdentifier): string {
 		return hash('sha512', $methodIdentifier . $userIdentifier);
 	}
 
@@ -63,9 +64,14 @@ class MemoryCache implements IBackend {
 	 * @param string $identifier
 	 * @return array
 	 */
-	private function getExistingAttempts($identifier) {
-		$cachedAttempts = json_decode($this->cache->get($identifier), true);
-		if(is_array($cachedAttempts)) {
+	private function getExistingAttempts(string $identifier): array {
+		$cachedAttempts = $this->cache->get($identifier);
+		if ($cachedAttempts === null) {
+			return [];
+		}
+
+		$cachedAttempts = json_decode($cachedAttempts, true);
+		if(\is_array($cachedAttempts)) {
 			return $cachedAttempts;
 		}
 
@@ -75,9 +81,9 @@ class MemoryCache implements IBackend {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function getAttempts($methodIdentifier,
-								$userIdentifier,
-								$seconds) {
+	public function getAttempts(string $methodIdentifier,
+								string $userIdentifier,
+								int $seconds): int {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		$existingAttempts = $this->getExistingAttempts($identifier);
 
@@ -96,9 +102,9 @@ class MemoryCache implements IBackend {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function registerAttempt($methodIdentifier,
-									$userIdentifier,
-									$period) {
+	public function registerAttempt(string $methodIdentifier,
+									string $userIdentifier,
+									int $period) {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		$existingAttempts = $this->getExistingAttempts($identifier);
 		$currentTime = $this->timeFactory->getTime();

lib/private/Security/RateLimiting/Exception/RateLimitExceededException.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *

lib/private/Security/RateLimiting/Limiter.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *
@@ -58,12 +59,12 @@ class Limiter {
 	 * @param int $limit
 	 * @throws RateLimitExceededException
 	 */
-	private function register($methodIdentifier,
-							  $userIdentifier,
-							  $period,
-							  $limit) {
-		$existingAttempts = $this->backend->getAttempts($methodIdentifier, $userIdentifier, (int)$period);
-		if ($existingAttempts >= (int)$limit) {
+	private function register(string $methodIdentifier,
+							  string $userIdentifier,
+							  int $period,
+							  int $limit) {
+		$existingAttempts = $this->backend->getAttempts($methodIdentifier, $userIdentifier, $period);
+		if ($existingAttempts >= $limit) {
 			throw new RateLimitExceededException();
 		}
 
@@ -79,10 +80,10 @@ class Limiter {
 	 * @param string $ip
 	 * @throws RateLimitExceededException
 	 */
-	public function registerAnonRequest($identifier,
-										$anonLimit,
-										$anonPeriod,
-										$ip) {
+	public function registerAnonRequest(string $identifier,
+										int $anonLimit,
+										int $anonPeriod,
+										string $ip) {
 		$ipSubnet = (new IpAddress($ip))->getSubnet();
 
 		$anonHashIdentifier = hash('sha512', 'anon::' . $identifier . $ipSubnet);
@@ -98,9 +99,9 @@ class Limiter {
 	 * @param IUser $user
 	 * @throws RateLimitExceededException
 	 */
-	public function registerUserRequest($identifier,
-										$userLimit,
-										$userPeriod,
+	public function registerUserRequest(string $identifier,
+										int $userLimit,
+										int $userPeriod,
 										IUser $user) {
 		$userHashIdentifier = hash('sha512', 'user::' . $identifier . $user->getUID());
 		$this->register($identifier, $userHashIdentifier, $userPeriod, $userLimit);

tests/lib/Security/RateLimiting/Backend/MemoryCacheTest.php

@@ -61,7 +61,7 @@ class MemoryCacheTest extends TestCase {
 			->expects($this->once())
 			->method('get')
 			->with('eea460b8d756885099c7f0a4c083bf6a745069ee4a301984e726df58fd4510bffa2dac4b7fd5d835726a6753ffa8343ba31c7e902bbef78fc68c2e743667cb4b')
-			->willReturn(false);
+			->willReturn(null);
 
 		$this->assertSame(0, $this->memoryCache->getAttempts('Method', 'User', 123));
 	}
@@ -97,7 +97,7 @@ class MemoryCacheTest extends TestCase {
 			->expects($this->once())
 			->method('get')
 			->with('eea460b8d756885099c7f0a4c083bf6a745069ee4a301984e726df58fd4510bffa2dac4b7fd5d835726a6753ffa8343ba31c7e902bbef78fc68c2e743667cb4b')
-			->willReturn(false);
+			->willReturn(null);
 		$this->cache
 			->expects($this->once())
 			->method('set')