Merge pull request #38268 from nextcloud/backport/38267/stable26

[stable26] fix(lostpassword): Also rate limit the setPassword endpoint
nextcloud · May 16, 2023 · e05da15 · e05da15
2 parents f7fdaf8 + 156cc8d
commit e05da15
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 13 deletions.
diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
@@ -223,16 +223,18 @@ public function email(string $user): JSONResponse {
 
 	/**
 	 * @PublicPage
+	 * @BruteForceProtection(action=passwordResetEmail)
+	 * @AnonRateThrottle(limit=10, period=300)
 	 */
-	public function setPassword(string $token, string $userId, string $password, bool $proceed): array {
+	public function setPassword(string $token, string $userId, string $password, bool $proceed): JSONResponse {
 		if ($this->encryptionManager->isEnabled() && !$proceed) {
 			$encryptionModules = $this->encryptionManager->getEncryptionModules();
 			foreach ($encryptionModules as $module) {
 				/** @var IEncryptionModule $instance */
 				$instance = call_user_func($module['callback']);
 				// this way we can find out whether per-user keys are used or a system wide encryption key
 				if ($instance->needDetailedAccessList()) {
-					return $this->error('', ['encryption' => true]);
+					return new JSONResponse($this->error('', ['encryption' => true]));
 				}
 			}
 		}
@@ -260,12 +262,16 @@ public function setPassword(string $token, string $userId, string $password, boo
 			$this->config->deleteUserValue($userId, 'core', 'lostpassword');
 			@\OC::$server->getUserSession()->unsetMagicInCookie();
 		} catch (HintException $e) {
-			return $this->error($e->getHint());
+			$response = new JSONResponse($this->error($e->getHint()));
+			$response->throttle();
+			return $response;
 		} catch (Exception $e) {
-			return $this->error($e->getMessage());
+			$response = new JSONResponse($this->error($e->getMessage()));
+			$response->throttle();
+			return $response;
 		}
 
-		return $this->success(['user' => $userId]);
+		return new JSONResponse($this->success(['user' => $userId]));
 	}
 
 	/**

diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php
@@ -447,7 +447,7 @@ public function testSetPasswordUnsuccessful() {
 
 		$response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true);
 		$expectedResponse = ['status' => 'error', 'msg' => ''];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testSetPasswordSuccessful() {
@@ -475,7 +475,7 @@ public function testSetPasswordSuccessful() {
 
 		$response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true);
 		$expectedResponse = ['user' => 'ValidTokenUser', 'status' => 'success'];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testSetPasswordExpiredToken() {
@@ -494,7 +494,7 @@ public function testSetPasswordExpiredToken() {
 			'status' => 'error',
 			'msg' => 'Could not reset password because the token is expired',
 		];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testSetPasswordInvalidDataInDb() {
@@ -514,7 +514,7 @@ public function testSetPasswordInvalidDataInDb() {
 			'status' => 'error',
 			'msg' => 'Could not reset password because the token is invalid',
 		];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testIsSetPasswordWithoutTokenFailing() {
@@ -533,7 +533,7 @@ public function testIsSetPasswordWithoutTokenFailing() {
 			'status' => 'error',
 			'msg' => 'Could not reset password because the token is invalid'
 		];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testSetPasswordForDisabledUser() {
@@ -563,7 +563,7 @@ public function testSetPasswordForDisabledUser() {
 			'status' => 'error',
 			'msg' => 'Could not reset password because the token is invalid'
 		];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testSendEmailNoEmail() {
@@ -599,7 +599,7 @@ public function testSetPasswordEncryptionDontProceedPerUserKey() {
 			}]]);
 		$response = $this->lostController->setPassword('myToken', 'user', 'newpass', false);
 		$expectedResponse = ['status' => 'error', 'msg' => '', 'encryption' => true];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testSetPasswordDontProceedMasterKey() {
@@ -627,7 +627,7 @@ public function testSetPasswordDontProceedMasterKey() {
 
 		$response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', false);
 		$expectedResponse = ['user' => 'ValidTokenUser', 'status' => 'success'];
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
 	public function testTwoUsersWithSameEmail() {