From 9544e707ed7f5acbd2a21ccdc99dc8144984b475 Mon Sep 17 00:00:00 2001
From: TheGeneralist <180094941+thegeneralist01@users.noreply.github.com>
Date: Mon, 29 Jun 2026 21:30:45 +0200
Subject: [PATCH] fix: address 6 code-review findings

NixOS module (4 fixes):
- Remove RestrictNamespaces=true: Chromium (launched by single-file for
  web captures) needs Linux user namespaces; blocking them broke captures
- Escape TOML string values: labels/paths with quotes or backslashes
  would produce invalid archivr-server.toml and prevent startup
- Bracket IPv6 listen addresses: ::1:8080 is rejected by Rust's
  SocketAddr parser; [::1]:8080 is the correct form (RFC 2732)
- Add optional storePath per archive: covers archives initialised with
  a custom store path outside the default sibling store/ directory

Rust (1 fix):
- extract_client_ip loopback branch: take last XFF value not first.
  When a proxy appends to X-Forwarded-For an attacker controls the
  first entry; the last entry is always set by the trusted local proxy

Frontend (1 fix):
- Gate fetchArchives on authState === 'authenticated'. The empty-dep
  useEffect fired on mount (before login), hit 401, and never retried
  after login because authState was not in the dependency array
---
 crates/archivr-server/src/routes.rs |  8 +++++---
 frontend/src/App.jsx                |  6 ++++--
 modules/nixos/archivr-server.nix    | 32 +++++++++++++++++++++++------
 3 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/crates/archivr-server/src/routes.rs b/crates/archivr-server/src/routes.rs
index 1d074f4..90d3a2f 100644
--- a/crates/archivr-server/src/routes.rs
+++ b/crates/archivr-server/src/routes.rs
@@ -174,13 +174,15 @@ fn extract_client_ip(req: &Request) -> IpAddr {
 
     match peer_ip {
         // Peer is a loopback address → the connection came from a local
-        // reverse proxy (nginx/caddy on the same host). Trust the first
-        // address in X-Forwarded-For as the real client IP.
+        // reverse proxy (nginx/caddy on the same host). Trust the last
+        // address in X-Forwarded-For as the real client IP — the last entry
+        // is always appended by the trusted proxy, even if the client sent a
+        // spoofed value earlier in the chain.
         Some(peer) if peer.is_loopback() => req
             .headers()
             .get("x-forwarded-for")
             .and_then(|v| v.to_str().ok())
-            .and_then(|s| s.split(',').next())
+            .and_then(|s| s.split(',').last())
             .and_then(|s| s.trim().parse().ok())
             .unwrap_or(peer),
 
diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx
index b49e053..1a3b758 100644
--- a/frontend/src/App.jsx
+++ b/frontend/src/App.jsx
@@ -70,8 +70,10 @@ export default function App() {
     }
   }, [])
 
-  // Mount: load archives, pick first, then parallel load
+  // Load archives once authenticated (re-runs when authState changes so
+  // it triggers correctly after first login or a session refresh).
   useEffect(() => {
+    if (authState !== 'authenticated') return
     fetchArchives().then(list => {
       setArchives(list)
       if (list.length > 0) {
@@ -79,7 +81,7 @@ export default function App() {
         setArchiveId(first)
       }
     })
-  }, [])
+  }, [authState])
 
   // Archive change: parallel load entries + runs + tags
   useEffect(() => {
diff --git a/modules/nixos/archivr-server.nix b/modules/nixos/archivr-server.nix
index 1c26c36..935ba46 100644
--- a/modules/nixos/archivr-server.nix
+++ b/modules/nixos/archivr-server.nix
@@ -6,8 +6,16 @@
 let
   cfg = config.services.archivr-server;
 
+  # Escape characters that would break TOML double-quoted strings.
+  escapeTOML = s: builtins.replaceStrings [ "\\" "\"" ] [ "\\\\" "\\\"" ] s;
+
   # Derived bind string from separate address + port options.
-  bindStr = "${cfg.listenAddress}:${toString cfg.port}";
+  # IPv6 addresses contain ":" and must be wrapped in brackets per RFC 2732.
+  bindStr =
+    let hasColon = builtins.match ".*:.*" cfg.listenAddress != null;
+    in if hasColon
+       then "[${cfg.listenAddress}]:${toString cfg.port}"
+       else "${cfg.listenAddress}:${toString cfg.port}";
 
   # Generate the TOML registry file from NixOS options.
   # The auth DB is pinned to the StateDirectory so it survives upgrades.
@@ -17,9 +25,9 @@ let
 
     ${lib.concatMapStrings (a: ''
       [[archives]]
-      id = "${a.id}"
-      label = "${a.label}"
-      archive_path = "${a.path}"
+      id = "${escapeTOML a.id}"
+      label = "${escapeTOML a.label}"
+      archive_path = "${escapeTOML a.path}"
 
     '') cfg.archives}
   '';
@@ -73,6 +81,16 @@ in
               The parent directory (which also contains the {file}`store/` blob directory)
               is whitelisted for read-write access under systemd hardening.'';
           };
+          storePath = lib.mkOption {
+            type = lib.types.nullOr lib.types.str;
+            default = null;
+            description = ''
+              Absolute path to the store directory for this archive. Only needed
+              when the archive was initialised with a custom store path outside
+              the default sibling {file}`store/` location. When null (the default),
+              the parent of {option}`path` already covers the standard layout.
+            '';
+          };
         };
       });
       default = [];
@@ -154,10 +172,12 @@ in
         NoNewPrivileges = true;
         PrivateTmp = true;
         ProtectSystem = "strict";
-        ReadWritePaths = [ "/var/lib/archivr-server" ] ++ (map (a: builtins.dirOf a.path) cfg.archives);
+        ReadWritePaths =
+          [ "/var/lib/archivr-server" ]
+          ++ (map (a: builtins.dirOf a.path) cfg.archives)
+          ++ (lib.concatMap (a: lib.optional (a.storePath != null) a.storePath) cfg.archives);
         RestrictAddressFamilies = [ "AF_INET" "AF_INET6" "AF_UNIX" ];
         LockPersonality = true;
-        RestrictNamespaces = true;
         RestrictSUIDSGID = true;
         RemoveIPC = true;
       };