From 79e05f33cd3cbad60dc0c7f3e2cdaef0a5e2fba1 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Mon, 4 May 2026 10:06:14 +0000
Subject: [PATCH] security: enforce APCu availability, fix fixed-window rate
 limiting, tighten URL validation

Agent-Logs-Url: https://github.com/nextcloud/all-in-one/sessions/f1016d36-0771-46e0-992c-95ce22594414

Co-authored-by: szaimen <42591237+szaimen@users.noreply.github.com>
---
 php/public/clean-history.js            | 11 +++--------
 php/src/Controller/LoginController.php | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/php/public/clean-history.js b/php/public/clean-history.js
index 0a6bbc86..b4634d55 100644
--- a/php/public/clean-history.js
+++ b/php/public/clean-history.js
@@ -12,14 +12,9 @@
 // recorded history entry.
 const rawTarget = document.currentScript.dataset.target;
 
-// Validate that the redirect target is a safe relative path (starts with '.' or '/').
-// This guards against hypothetical injection (e.g. 'javascript:…') even though the
-// value is server-set.
-const safePattern = /^[./]/;
-const unsafePattern = /^\/\//;
-const target = (typeof rawTarget === 'string' && safePattern.test(rawTarget) && !unsafePattern.test(rawTarget))
-    ? rawTarget
-    : '/';
+// Only accept the exact relative path we set server-side to prevent any
+// potential open-redirect via a manipulated data-target value.
+const target = rawTarget === '../../' ? rawTarget : '/';
 
 history.replaceState(null, '', location.pathname);
 window.location.replace(target);
diff --git a/php/src/Controller/LoginController.php b/php/src/Controller/LoginController.php
index 679aa8b3..51567a5f 100644
--- a/php/src/Controller/LoginController.php
+++ b/php/src/Controller/LoginController.php
@@ -46,10 +46,20 @@ readonly class LoginController {
             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 so it survives cache clears
-        // and container restarts, preserving rate-limit state across APCu evictions.
+        // 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);
         $attempts = (int)(apcu_fetch($rateLimitKey) ?: 0);
@@ -68,8 +78,13 @@ readonly class LoginController {
             return $response->withHeader('Location', '.')->withStatus(201);
         }
 
-        // Increment the failed-attempts counter (expires after RATE_LIMIT_WINDOW_SEC seconds).
-        apcu_store($rateLimitKey, $attempts + 1, self::RATE_LIMIT_WINDOW_SEC);
+        // 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($rateLimitKey, 1, self::RATE_LIMIT_WINDOW_SEC)) {
+            apcu_inc($rateLimitKey);
+        }
 
         // Punish failed auth attempts with a delay, as a very simple means against bots.
         sleep(5);