From ef58220c0990da74bd0badd21954ca473f12bb54 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Mon, 4 May 2026 10:03:53 +0000
Subject: [PATCH] security: use persistent HMAC key, validate clean-history
 target, improve comments

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             | 12 +++++++++++-
 php/src/Controller/DockerController.php |  5 +++--
 php/src/Controller/LoginController.php  | 18 +++++++++++-------
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/php/public/clean-history.js b/php/public/clean-history.js
index 26b8f642..0a6bbc86 100644
--- a/php/public/clean-history.js
+++ b/php/public/clean-history.js
@@ -10,6 +10,16 @@
 // We replace with location.pathname only (no query string, no hash), which
 // intentionally strips the ?token=… parameter and any hash fragment from the
 // recorded history entry.
-const target = document.currentScript.dataset.target;
+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
+    : '/';
+
 history.replaceState(null, '', location.pathname);
 window.location.replace(target);
diff --git a/php/src/Controller/DockerController.php b/php/src/Controller/DockerController.php
index 8fe99ee8..6a76719f 100644
--- a/php/src/Controller/DockerController.php
+++ b/php/src/Controller/DockerController.php
@@ -167,8 +167,9 @@ readonly class DockerController {
 
         // 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.
-        // ConfigurationManager.set() overwrites the stored value with an empty string and
-        // immediately writes the updated config to disk, so the plaintext password is removed.
+        // 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
diff --git a/php/src/Controller/LoginController.php b/php/src/Controller/LoginController.php
index 41b86ae7..679aa8b3 100644
--- a/php/src/Controller/LoginController.php
+++ b/php/src/Controller/LoginController.php
@@ -34,19 +34,23 @@ readonly class LoginController {
         }
 
         // 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 rate-limit against a shared bucket when no IP is available.
+            // 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);
         }
 
-        // Use HMAC to avoid leaking which IPs are being tracked via predictable cache-key names.
-        $hmacKey = (string)(apcu_fetch('login_rate_limit_hmac_key') ?: '');
-        if ($hmacKey === '') {
-            $hmacKey = bin2hex(random_bytes(16));
-            apcu_add('login_rate_limit_hmac_key', $hmacKey);
-        }
+        // 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.
+        $hmacKey = $this->dockerActionManager->GetAndGenerateSecretWrapper('RATE_LIMIT_HMAC_KEY');
         $rateLimitKey = 'login_attempts_' . hash_hmac('sha256', $ip, $hmacKey);
         $attempts = (int)(apcu_fetch($rateLimitKey) ?: 0);