From bbafd6ff8c4bd6d757d52d14b44e44a5ec7c41ed Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 10:02:28 +0000 Subject: [PATCH] revert: remove borgRestorePassword clearing and rate limiting (to be addressed in separate PRs) Agent-Logs-Url: https://github.com/nextcloud/all-in-one/sessions/d7eb7ba7-23d8-4082-8255-09f1338de24b Co-authored-by: szaimen <42591237+szaimen@users.noreply.github.com> --- php/src/Controller/DockerController.php | 7 --- php/src/Controller/LoginController.php | 68 ------------------------- 2 files changed, 75 deletions(-) diff --git a/php/src/Controller/DockerController.php b/php/src/Controller/DockerController.php index 6a76719f..c14b1616 100644 --- a/php/src/Controller/DockerController.php +++ b/php/src/Controller/DockerController.php @@ -165,13 +165,6 @@ readonly class DockerController { $id = 'nextcloud-aio-borgbackup'; $this->PerformRecursiveContainerStart($id, true, $addToStreamingResponseBody); - // The password has been passed to the borgbackup container's environment. - // Clear it from the persistent config so it is not kept at rest longer than needed. - // The borgRestorePassword property setter calls ConfigurationManager::set() under - // the hood (via PHP 8.4+ property hooks), which immediately overwrites the stored - // value and writes the updated config to disk. - $this->configurationManager->borgRestorePassword = ''; - // End streaming response $this->finalizeStreamingResponse($nonbufResp); return $nonbufResp; diff --git a/php/src/Controller/LoginController.php b/php/src/Controller/LoginController.php index 6b2163e9..0d8d42f2 100644 --- a/php/src/Controller/LoginController.php +++ b/php/src/Controller/LoginController.php @@ -17,85 +17,17 @@ readonly class LoginController { ) { } - /** - * Maximum number of failed login attempts allowed within the rate-limit window. - */ - private const int MAX_FAILED_ATTEMPTS = 10; - - /** - * Duration in seconds during which failed attempts are counted (and for which a lockout lasts). - */ - private const int RATE_LIMIT_WINDOW_SEC = 300; - public function TryLogin(Request $request, Response $response, array $args) : Response { if (!$this->dockerActionManager->isLoginAllowed()) { $response->getBody()->write("The login is blocked since Nextcloud is running."); return $response->withHeader('Location', '.')->withStatus(422); } - - // Per-IP rate limiting: block after MAX_FAILED_ATTEMPTS failures within RATE_LIMIT_WINDOW_SEC. - // - // REMOTE_ADDR is set by Caddy (the reverse proxy that sits in front of PHP-FPM inside - // the mastercontainer), which passes the real client IP. In environments where an - // additional upstream proxy forwards traffic to Caddy, operators should configure Caddy - // with `trusted_proxies` so that REMOTE_ADDR reflects the actual client. - $ip = $request->getServerParams()['REMOTE_ADDR'] ?? ''; - if ($ip === '') { - // Refuse to fall back to a shared bucket when no IP is available. - $response->getBody()->write("Unable to determine client IP. Login refused."); - return $response->withStatus(403); - } - - // Require APCu to be available: without it rate limiting cannot be enforced, so we - // refuse logins rather than silently allowing unlimited attempts. - if (!apcu_enabled()) { - error_log('APCu is not available. Login rejected to enforce rate limiting.'); - $response->getBody()->write("Login temporarily unavailable. Please try again later."); - return $response->withStatus(503); - } - - // Use HMAC so the cache keys are not predictable from IP addresses alone, preventing - // enumeration of which IPs have attempted logins via APCu key inspection. - // The HMAC key is persisted via the configuration manager (in configuration.json) so it - // survives cache clears and container restarts. Note: anyone with read access to - // configuration.json could derive cache keys, but that file already contains the master - // password and other secrets, so it must be kept confidential regardless. - $hmacKey = $this->dockerActionManager->GetAndGenerateSecretWrapper('RATE_LIMIT_HMAC_KEY'); - $rateLimitKey = 'login_attempts_' . hash_hmac('sha256', $ip, $hmacKey); - // Use the $success parameter to distinguish "key not found" from an APCu error. - // Since apcu_enabled() confirmed APCu is operational, false here reliably means - // the key is absent (no prior failed attempts in the current window). - $fetchedValue = apcu_fetch($rateLimitKey, $fetchSuccess); - $attempts = ($fetchSuccess === true) ? (int)$fetchedValue : 0; - - if ($attempts >= self::MAX_FAILED_ATTEMPTS) { - // Return 429 immediately; the rate limit itself is sufficient protection. - $response->getBody()->write("Too many failed login attempts. Please try again later."); - return $response->withStatus(429); - } - $password = $request->getParsedBody()['password'] ?? ''; if($this->authManager->CheckCredentials($password)) { - // Clear the counter on success. - apcu_delete($rateLimitKey); $this->authManager->SetAuthState(true); return $response->withHeader('Location', '.')->withStatus(201); } - // Increment the failed-attempts counter using a fixed-window approach: - // - apcu_add() creates the key with a TTL only on the FIRST failure in a window. - // - apcu_inc() atomically increments on subsequent failures WITHOUT resetting the TTL, - // ensuring the window always expires RATE_LIMIT_WINDOW_SEC after the first failure. - // If apcu_add() fails for a reason other than the key already existing (e.g., cache full), - // apcu_inc() will also fail and return false. In that case, fail safe by refusing the request. - if (!apcu_add($rateLimitKey, 1, self::RATE_LIMIT_WINDOW_SEC)) { - if (apcu_inc($rateLimitKey) === false) { - error_log('APCu rate limit increment failed; refusing login attempt to fail safe.'); - $response->getBody()->write("Login temporarily unavailable. Please try again later."); - return $response->withStatus(503); - } - } - // Punish failed auth attempts with a delay, as a very simple means against bots. sleep(5);