Merge pull request #46827 from nextcloud/build/psalm/named-attribute-…

…args build(psalm): Enforce named attribute arguments
nextcloud · Jul 29, 2024 · 3a7c18b · 3a7c18b
2 parents 1af3a66 + bc5c026
commit 3a7c18b
Show file tree

Hide file tree

Showing 9 changed files with 69 additions and 15 deletions.
diff --git a/build/psalm/AttributeNamedParameters.php b/build/psalm/AttributeNamedParameters.php
@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+use PhpParser\Node\Attribute;
+use Psalm\CodeLocation;
+use Psalm\FileSource;
+use Psalm\Issue\InvalidDocblock;
+use Psalm\IssueBuffer;
+use Psalm\Plugin\EventHandler\Event\AfterClassLikeVisitEvent;
+
+class AttributeNamedParameters implements Psalm\Plugin\EventHandler\AfterClassLikeVisitInterface {
+	public static function afterClassLikeVisit(AfterClassLikeVisitEvent $event): void {
+		$stmt = $event->getStmt();
+		$statementsSource = $event->getStatementsSource();
+
+		foreach ($stmt->attrGroups as $attrGroup) {
+			foreach ($attrGroup->attrs as $attr) {
+				self::checkAttribute($attr, $statementsSource);
+			}
+		}
+
+		foreach ($stmt->getMethods() as $method) {
+			foreach ($method->attrGroups as $attrGroup) {
+				foreach ($attrGroup->attrs as $attr) {
+					self::checkAttribute($attr, $statementsSource);
+				}
+			}
+		}
+	}
+
+	private static function checkAttribute(Attribute $stmt, FileSource $statementsSource): void {
+		if ($stmt->name->getLast() === 'Attribute') {
+			return;
+		}
+
+		foreach ($stmt->args as $arg) {
+			if ($arg->name === null) {
+				IssueBuffer::maybeAdd(
+					new InvalidDocblock(
+						'Attribute arguments must be named.',
+						new CodeLocation($statementsSource, $stmt)
+					)
+				);
+			}
+		}
+	}
+}
diff --git a/core/Controller/AppPasswordController.php b/core/Controller/AppPasswordController.php
@@ -168,7 +168,7 @@ public function rotateAppPassword(): DataResponse {
 	 * 403: Password confirmation failed
 	 */
 	#[NoAdminRequired]
-	#[BruteForceProtection('sudo')]
+	#[BruteForceProtection(action: 'sudo')]
 	#[UseSession]
 	#[ApiRoute(verb: 'PUT', url: '/apppassword/confirm', root: '/core')]
 	public function confirmUserPassword(string $password): DataResponse {

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
@@ -274,7 +274,7 @@ private function generateRedirect(?string $redirectUrl): RedirectResponse {
 	 */
 	#[NoCSRFRequired]
 	#[PublicPage]
-	#[BruteForceProtection('login')]
+	#[BruteForceProtection(action: 'login')]
 	#[UseSession]
 	#[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
 	#[FrontpageRoute(verb: 'POST', url: '/login')]
@@ -387,7 +387,7 @@ private function createLoginFailedResponse(
 	 * 403: Password confirmation failed
 	 */
 	#[NoAdminRequired]
-	#[BruteForceProtection('sudo')]
+	#[BruteForceProtection(action: 'sudo')]
 	#[UseSession]
 	#[NoCSRFRequired]
 	#[FrontpageRoute(verb: 'POST', url: '/login/confirm')]

diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
@@ -81,8 +81,8 @@ public function __construct(
 	 */
 	#[PublicPage]
 	#[NoCSRFRequired]
-	#[BruteForceProtection('passwordResetEmail')]
-	#[AnonRateLimit(10, 300)]
+	#[BruteForceProtection(action: 'passwordResetEmail')]
+	#[AnonRateLimit(limit: 10, period: 300)]
 	#[FrontpageRoute(verb: 'GET', url: '/lostpassword/reset/form/{token}/{userId}')]
 	public function resetform(string $token, string $userId): TemplateResponse {
 		try {
@@ -144,8 +144,8 @@ private function success(array $data = []): array {
 	}
 
 	#[PublicPage]
-	#[BruteForceProtection('passwordResetEmail')]
-	#[AnonRateLimit(10, 300)]
+	#[BruteForceProtection(action: 'passwordResetEmail')]
+	#[AnonRateLimit(limit: 10, period: 300)]
 	#[FrontpageRoute(verb: 'POST', url: '/lostpassword/email')]
 	public function email(string $user): JSONResponse {
 		if ($this->config->getSystemValue('lost_password_link', '') !== '') {
@@ -180,8 +180,8 @@ public function email(string $user): JSONResponse {
 	}
 
 	#[PublicPage]
-	#[BruteForceProtection('passwordResetEmail')]
-	#[AnonRateLimit(10, 300)]
+	#[BruteForceProtection(action: 'passwordResetEmail')]
+	#[AnonRateLimit(limit: 10, period: 300)]
 	#[FrontpageRoute(verb: 'POST', url: '/lostpassword/set/{token}/{userId}')]
 	public function setPassword(string $token, string $userId, string $password, bool $proceed): JSONResponse {
 		if ($this->encryptionManager->isEnabled() && !$proceed) {

diff --git a/core/Controller/OCSController.php b/core/Controller/OCSController.php
@@ -77,7 +77,7 @@ public function getCapabilities(): DataResponse {
 	}
 
 	#[PublicPage]
-	#[BruteForceProtection('login')]
+	#[BruteForceProtection(action: 'login')]
 	#[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
 	#[ApiRoute(verb: 'POST', url: '/check', root: '/person')]
 	public function personCheck(string $login = '', string $password = ''): DataResponse {

diff --git a/core/Controller/ProfileApiController.php b/core/Controller/ProfileApiController.php
@@ -53,7 +53,7 @@ public function __construct(
 	 */
 	#[NoAdminRequired]
 	#[PasswordConfirmationRequired]
-	#[UserRateLimit(40, 600)]
+	#[UserRateLimit(limit: 40, period: 600)]
 	#[ApiRoute(verb: 'PUT', url: '/{targetUserId}', root: '/profile')]
 	public function setVisibility(string $targetUserId, string $paramId, string $visibility): DataResponse {
 		$requestingUser = $this->userSession->getUser();

diff --git a/core/Controller/TranslationApiController.php b/core/Controller/TranslationApiController.php
@@ -62,8 +62,8 @@ public function languages(): DataResponse {
 	 * 412: Translating is not possible
 	 */
 	#[PublicPage]
-	#[UserRateLimit(25, 120)]
-	#[AnonRateLimit(10, 120)]
+	#[UserRateLimit(limit: 25, period: 120)]
+	#[AnonRateLimit(limit: 10, period: 120)]
 	#[ApiRoute(verb: 'POST', url: '/translate', root: '/translation')]
 	public function translate(string $text, ?string $fromLanguage, string $toLanguage): DataResponse {
 		try {

diff --git a/core/Controller/WipeController.php b/core/Controller/WipeController.php
@@ -40,7 +40,7 @@ public function __construct(
 	 */
 	#[PublicPage]
 	#[NoCSRFRequired]
-	#[AnonRateLimit(10, 300)]
+	#[AnonRateLimit(limit: 10, period: 300)]
 	#[FrontpageRoute(verb: 'POST', url: '/core/wipe/check')]
 	public function checkWipe(string $token): JSONResponse {
 		try {
@@ -69,7 +69,7 @@ public function checkWipe(string $token): JSONResponse {
 	 */
 	#[PublicPage]
 	#[NoCSRFRequired]
-	#[AnonRateLimit(10, 300)]
+	#[AnonRateLimit(limit: 10, period: 300)]
 	#[FrontpageRoute(verb: 'POST', url: '/core/wipe/success')]
 	public function wipeDone(string $token): JSONResponse {
 		try {

diff --git a/psalm.xml b/psalm.xml
@@ -16,6 +16,7 @@
 >
 	<plugins>
 		<plugin filename="build/psalm/AppFrameworkTainter.php" />
+		<plugin filename="build/psalm/AttributeNamedParameters.php" />
 	</plugins>
 	<projectFiles>
 		<directory name="apps/admin_audit"/>