fix(security): installed firewall no longer reports as absent

UFW installed cleanly, dpkg said ii, the install notification said so — and
/security/firewall still rendered "No Firewall Installed". fail2ban worked on
the same box, which is the clue: fail2ban-client lives in /usr/bin, ufw in
/usr/sbin, and the panel unit ships

    PATH=<venv>/bin:/usr/local/bin:/usr/bin:/bin

with no sbin. subprocess resolves argv[0] through $PATH alone, so `ufw status`
raised FileNotFoundError. _check_ufw wrapped both questions in one try/except,
so that exception discarded an `installed` already computed as True and
returned False. Measured on the affected host:

    is_command_available('ufw')        = True
    PackageManager.is_installed('ufw') = True
    get_status() -> ufw {'installed': False}, any_installed False

FirewallTab.jsx gates its empty state on any_installed. The Enable button was
broken identically — same bare-name exec.

Three fixes, each independent:
- resolve_command() gives argv[0] an absolute path, but ONLY when $PATH cannot
  resolve it, so working commands pass through byte-identical.
- probe failure can no longer unsay "installed" (same split for firewalld).
- the unit template gains /usr/local/sbin, /usr/sbin and /sbin. update.sh
  re-renders the unit, so existing installs pick this up on upgrade.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/app/services/firewall_service.py

@@ -46,43 +46,60 @@ class FirewallService:
         """Check firewalld status."""
         try:
             installed = PackageManager.is_installed('firewalld') or is_command_available('firewall-cmd')
+        except Exception:
+            installed = False
 
+        # Same separation as _check_ufw: a probe that fails must not be able to
+        # unsay an `installed` that was already determined correctly.
         running = False
         default_zone = None
-
         if installed:
+            try:
                 result = run_privileged(['firewall-cmd', '--state'])
-                running = 'running' in result.stdout.lower()
+                running = 'running' in (result.stdout or '').lower()
 
                 if running:
                     result = run_privileged(['firewall-cmd', '--get-default-zone'])
-                    default_zone = result.stdout.strip()
+                    default_zone = (result.stdout or '').strip()
+            except Exception:
+                running = False
+                default_zone = None
 
         return {
             'installed': installed,
             'running': running,
             'default_zone': default_zone
         }
-        except Exception:
-            return {'installed': False, 'running': False, 'default_zone': None}
 
     @classmethod
     def _check_ufw(cls) -> Dict:
-        """Check ufw status."""
+        """Check ufw status.
+
+        The two questions are answered independently on purpose. A single
+        try/except around both let a failure in the *status* probe discard an
+        ``installed`` that had already been determined correctly, so a working
+        install was reported as "No Firewall Installed" — the probe raised
+        FileNotFoundError because ufw lives in /usr/sbin, which is absent from
+        the panel unit's PATH. Not knowing whether it is running says nothing
+        about whether it is there.
+        """
         try:
             installed = PackageManager.is_installed('ufw') or is_command_available('ufw')
+        except Exception:
+            installed = False
 
         active = False
         if installed:
+            try:
                 result = run_privileged(['ufw', 'status'])
-                active = 'Status: active' in result.stdout
+                active = 'Status: active' in (result.stdout or '')
+            except Exception:
+                active = False
 
         return {
             'installed': installed,
             'active': active
         }
-        except Exception:
-            return {'installed': False, 'active': False}
 
     @classmethod
     def enable(cls, firewall: str = None) -> Dict:

backend/app/utils/system.py

@@ -50,6 +50,19 @@ def privileged_cmd(cmd: Union[List[str], str], *, user: Optional[str] = None) ->
         return cmd
 
     cmd = list(cmd)
+
+    # Give argv[0] an absolute path ONLY when $PATH cannot resolve it. subprocess
+    # searches $PATH and nothing else, so a tool that exists but sits outside it
+    # raises FileNotFoundError — which callers wrapping the probe in a broad
+    # `except` then report as "not installed". This is how a successfully
+    # installed ufw (in /usr/sbin, absent from the unit's PATH) surfaced in the
+    # panel as "No Firewall Installed". Left untouched when $PATH already works,
+    # so a resolvable command is passed through exactly as the caller wrote it.
+    if cmd and cmd[0] != 'sudo' and not os.path.isabs(cmd[0]) and not shutil.which(cmd[0]):
+        resolved = resolve_command(cmd[0])
+        if resolved:
+            cmd[0] = resolved
+
     if _needs_sudo() and cmd[0] != 'sudo':
         if user:
             return ['sudo', '-n', '-u', user] + cmd
@@ -108,23 +121,47 @@ def run_command(cmd: Union[List[str], str], *, timeout: int = 60,
     }
 
 
+# Searched when ``$PATH`` does not resolve a command. Ordered the way a login
+# shell would. ``/usr/sbin`` and ``/sbin`` matter most: the panel's systemd unit
+# ships a PATH of venv:/usr/local/bin:/usr/bin:/bin, so every sbin-resident tool
+# (ufw, iptables, nft) is unreachable by bare name from the service. ``/snap/bin``
+# is included because snap is the recommended install route for certbot and is
+# almost never on a service's PATH.
+_COMMAND_SEARCH_DIRS = ('/usr/local/sbin', '/usr/local/bin', '/usr/sbin',
+                        '/usr/bin', '/sbin', '/bin', '/snap/bin')
+
+
+def resolve_command(cmd: str) -> Optional[str]:
+    """Absolute path to *cmd*, or None when it cannot be found anywhere.
+
+    ``shutil.which`` first, then :data:`_COMMAND_SEARCH_DIRS`. Knowing a command
+    exists is not enough to run it: ``subprocess`` resolves argv[0] through
+    ``$PATH`` alone and raises ``FileNotFoundError`` when that misses, so a
+    caller that only checked availability would still fail to exec.
+    """
+    if os.path.isabs(cmd):
+        return cmd if os.path.exists(cmd) else None
+
+    found = shutil.which(cmd)
+    if found:
+        return found
+
+    for directory in _COMMAND_SEARCH_DIRS:
+        candidate = os.path.join(directory, cmd)
+        if os.path.exists(candidate):
+            return candidate
+
+    return None
+
+
 def is_command_available(cmd: str) -> bool:
     """Check whether *cmd* is available on the system.
 
-    Uses ``shutil.which`` first, then falls back to checking common sbin/local
-    paths that may not be on the current ``$PATH``.  ``/snap/bin`` is included
-    because snap is the recommended install method for tools like certbot, and
-    snap's bin dir is frequently missing from a service's ``$PATH``.
+    Uses ``shutil.which`` first, then falls back to the same sbin/local paths
+    :func:`resolve_command` searches — so "available" and "runnable" can never
+    disagree.
     """
-    if shutil.which(cmd):
-        return True
-
-    for directory in ('/usr/bin', '/usr/sbin', '/usr/local/bin',
-                      '/usr/local/sbin', '/snap/bin'):
-        if os.path.exists(os.path.join(directory, cmd)):
-            return True
-
-    return False
+    return resolve_command(cmd) is not None
 
 
 def sourced_result(lines: list, source: str, source_label: str) -> dict:

backend/tests/test_firewall_detection.py

@@ -0,0 +1,200 @@
+"""Firewall detection: an installed firewall must not report as absent.
+
+Measured on a live panel: `ufw` was installed and `dpkg -l` said `ii`, the
+install notification said "UFW installed successfully", and /security/firewall
+still rendered "No Firewall Installed". fail2ban worked perfectly on the same
+box, which is the clue — `fail2ban-client` lives in /usr/bin, `ufw` in
+/usr/sbin, and the panel's systemd unit ships:
+
+    Environment="PATH=/opt/serverkit/venv/bin:/usr/local/bin:/usr/bin:/bin"
+
+So subprocess could not exec `ufw` by bare name and raised FileNotFoundError.
+The service had already computed installed=True correctly; a single try/except
+around the whole body then threw that away and returned installed=False.
+
+Two independent defects, two independent guards:
+  1. a command outside $PATH must still be runnable (resolve_command)
+  2. a failed status probe must never unsay "installed"
+"""
+
+import subprocess
+from unittest.mock import patch
+
+import pytest
+
+from app.services.firewall_service import FirewallService
+from app.utils.system import is_command_available, privileged_cmd, resolve_command
+
+
+# --------------------------------------------------------------------------- #
+# 1. Commands outside $PATH
+# --------------------------------------------------------------------------- #
+
+class TestResolveCommand:
+
+    @patch('app.utils.system.shutil.which', return_value='/usr/bin/nginx')
+    def test_path_hit_is_used_as_is(self, _which):
+        assert resolve_command('nginx') == '/usr/bin/nginx'
+
+    @patch('app.utils.system.os.path.exists')
+    @patch('app.utils.system.shutil.which', return_value=None)
+    def test_sbin_is_searched_when_path_misses(self, _which, exists):
+        """The exact live failure: ufw in /usr/sbin, not on the unit's PATH."""
+        exists.side_effect = lambda p: p == '/usr/sbin/ufw'
+
+        assert resolve_command('ufw') == '/usr/sbin/ufw'
+        assert is_command_available('ufw') is True
+
+    @patch('app.utils.system.os.path.exists', return_value=False)
+    @patch('app.utils.system.shutil.which', return_value=None)
+    def test_genuinely_absent_command_resolves_to_none(self, _which, _exists):
+        assert resolve_command('nonexistent') is None
+        assert is_command_available('nonexistent') is False
+
+    @patch('app.utils.system.os.path.exists', return_value=False)
+    def test_absolute_path_that_does_not_exist_is_not_invented(self, _exists):
+        assert resolve_command('/opt/nope/tool') is None
+
+
+class TestPrivilegedCmdResolution:
+
+    @patch('app.utils.system.os.name', 'posix')
+    @patch('app.utils.system.os.geteuid', return_value=0, create=True)
+    @patch('app.utils.system.os.path.exists')
+    @patch('app.utils.system.shutil.which', return_value=None)
+    def test_unresolvable_command_is_given_an_absolute_path(self, _which, exists,
+                                                            _euid):
+        """Without this, subprocess raises FileNotFoundError on argv[0]."""
+        exists.side_effect = lambda p: p == '/usr/sbin/ufw'
+
+        assert privileged_cmd(['ufw', 'status']) == ['/usr/sbin/ufw', 'status']
+
+    @patch('app.utils.system.os.name', 'posix')
+    @patch('app.utils.system.os.geteuid', return_value=0, create=True)
+    @patch('app.utils.system.shutil.which', return_value='/usr/bin/systemctl')
+    def test_resolvable_command_is_passed_through_untouched(self, _which, _euid):
+        """A working PATH must not have its argv rewritten underneath callers."""
+        assert privileged_cmd(['systemctl', 'restart', 'nginx']) == [
+            'systemctl', 'restart', 'nginx'
+        ]
+
+    @patch('app.utils.system.os.name', 'posix')
+    @patch('app.utils.system.os.geteuid', return_value=0, create=True)
+    @patch('app.utils.system.os.path.exists', return_value=False)
+    @patch('app.utils.system.shutil.which', return_value=None)
+    def test_unfindable_command_is_left_alone(self, _which, _exists, _euid):
+        """Nothing to resolve to — do not fabricate a path."""
+        assert privileged_cmd(['ghost', '--x']) == ['ghost', '--x']
+
+    @patch('app.utils.system.os.name', 'posix')
+    @patch('app.utils.system.os.geteuid', return_value=1000, create=True)
+    @patch('app.utils.system.os.path.exists')
+    @patch('app.utils.system.shutil.which')
+    def test_resolution_composes_with_sudo(self, which, exists, _euid):
+        # sudo itself must stay resolvable, or _needs_sudo() short-circuits and
+        # the test would pass for the wrong reason.
+        which.side_effect = lambda c: '/usr/bin/sudo' if c == 'sudo' else None
+        exists.side_effect = lambda p: p == '/usr/sbin/ufw'
+
+        assert privileged_cmd(['ufw', 'status']) == [
+            'sudo', '-n', '/usr/sbin/ufw', 'status'
+        ]
+
+
+# --------------------------------------------------------------------------- #
+# 2. A failed probe must not unsay "installed"
+# --------------------------------------------------------------------------- #
+
+class TestProbeFailureIsolation:
+
+    @patch('app.services.firewall_service.run_privileged')
+    @patch('app.services.firewall_service.is_command_available', return_value=True)
+    @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True)
+    def test_status_probe_raising_does_not_hide_the_install(self, _pkg, _avail, run):
+        """The live bug, reproduced exactly."""
+        run.side_effect = FileNotFoundError(2, 'No such file or directory', 'ufw')
+
+        ufw = FirewallService._check_ufw()
+
+        assert ufw['installed'] is True     # was False — the whole bug
+        assert ufw['active'] is False       # genuinely unknown, reported as off
+
+    @patch('app.services.firewall_service.run_privileged')
+    @patch('app.services.firewall_service.is_command_available')
+    @patch('app.services.firewall_service.PackageManager.is_installed')
+    def test_page_would_render_the_firewall_not_the_empty_state(self, pkg, avail, run):
+        """`any_installed` is what FirewallTab.jsx gates the empty state on.
+
+        Only ufw is present here — the live box has no firewalld, and saying
+        "everything is installed" would let the assertion pass for free.
+        """
+        pkg.side_effect = lambda name: name == 'ufw'
+        avail.side_effect = lambda name: name == 'ufw'
+        run.side_effect = FileNotFoundError(2, 'No such file or directory', 'ufw')
+
+        status = FirewallService.get_status()
+
+        assert status['any_installed'] is True
+        assert status['active_firewall'] == 'ufw'
+        assert status['firewalld']['installed'] is False
+
+    @patch('app.services.firewall_service.run_privileged')
+    @patch('app.services.firewall_service.is_command_available', return_value=True)
+    @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True)
+    def test_installed_but_inactive_is_reported_faithfully(self, _pkg, _avail, run):
+        """builditdesign's real state: installed, `Status: inactive`."""
+        run.return_value = subprocess.CompletedProcess([], 0, stdout='Status: inactive\n',
+                                                       stderr='')
+
+        status = FirewallService.get_status()
+
+        assert status['any_installed'] is True
+        assert status['any_active'] is False
+        assert status['ufw'] == {'installed': True, 'active': False}
+
+    @patch('app.services.firewall_service.run_privileged')
+    @patch('app.services.firewall_service.is_command_available', return_value=True)
+    @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True)
+    def test_active_firewall_is_detected(self, _pkg, _avail, run):
+        run.return_value = subprocess.CompletedProcess([], 0, stdout='Status: active\n',
+                                                       stderr='')
+
+        status = FirewallService.get_status()
+
+        assert status['any_active'] is True
+        assert status['ufw']['active'] is True
+
+    @patch('app.services.firewall_service.is_command_available', return_value=False)
+    @patch('app.services.firewall_service.PackageManager.is_installed', return_value=False)
+    def test_genuinely_absent_firewall_still_reports_absent(self, _pkg, _avail):
+        """The guard must not make everything look installed."""
+        status = FirewallService.get_status()
+
+        assert status['any_installed'] is False
+        assert status['active_firewall'] is None
+
+    @patch('app.services.firewall_service.run_privileged')
+    @patch('app.services.firewall_service.is_command_available', return_value=True)
+    @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True)
+    def test_firewalld_probe_failure_is_isolated_too(self, _pkg, _avail, run):
+        run.side_effect = FileNotFoundError(2, 'No such file or directory', 'firewall-cmd')
+
+        firewalld = FirewallService._check_firewalld()
+
+        assert firewalld['installed'] is True
+        assert firewalld['running'] is False
+        assert firewalld['default_zone'] is None
+
+    @pytest.mark.parametrize('failure', [
+        subprocess.TimeoutExpired(cmd=['ufw', 'status'], timeout=30),
+        PermissionError(13, 'Permission denied'),
+        OSError(8, 'Exec format error'),
+    ])
+    @patch('app.services.firewall_service.run_privileged')
+    @patch('app.services.firewall_service.is_command_available', return_value=True)
+    @patch('app.services.firewall_service.PackageManager.is_installed', return_value=True)
+    def test_any_probe_failure_keeps_installed_true(self, _pkg, _avail, run, failure):
+        """A sudo timeout or permission denial is not evidence of absence."""
+        run.side_effect = failure
+
+        assert FirewallService._check_ufw()['installed'] is True

templates/serverkit-backend.service.in

@@ -8,7 +8,13 @@ Type=simple
 User=@USER@
 Group=@USER@
 WorkingDirectory=@SERVERKIT_DIR@/backend
-Environment="PATH=@SERVERKIT_VENV_DIR@/bin:/usr/local/bin:/usr/bin:/bin"
+# sbin dirs are mandatory, not cosmetic: the panel shells out to ufw, iptables
+# and nft, all of which live in /usr/sbin. Without them a bare-name exec raises
+# FileNotFoundError, which surfaced as "No Firewall Installed" on a host where
+# ufw was installed and working. app/utils/system.py resolves such commands to
+# an absolute path as a backstop; this keeps anything not routed through it
+# working too.
+Environment="PATH=@SERVERKIT_VENV_DIR@/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
 Environment="FLASK_ENV=production"
 Environment="SERVERKIT_INSTALL_DIR=@SERVERKIT_DIR@"
 EnvironmentFile=@SERVERKIT_DIR@/.env