Fix what a review of the whole app found, end to end (0.67.1)

A pass over every feature against a real Docker daemon, then clicking
through the UI as admin and read-only user, on a desktop and a phone.

Security. /ws/deploy ran compose up for the read-only role; it is
admin-only now. A container's inspect view, a stack's compose file
(POSTGRES_PASSWORD: hunter2) and the viewer's .env tab no longer show
credentials to that role. Git branch and URL values that git would read
as options (-x, ext::) are refused. SFTP host keys are checked: pinned
on first use or entered by hand (key line or SHA256 fingerprint), a
different key is refused before the password is sent. A backup name
from the API can no longer reach outside the destination folder. The
file browser refuses to move or rename a browse root, as delete did.

Docker lifecycle. Closing the deploy console released the stack lock
while compose was still running and logged rc=None as success; deploys
and updates are now background runs that hold the lock to the end,
report the real exit code and can be re-attached to. Every closed log
view left a `compose logs -f` behind. A dozen open tabs exhausted the
default thread pool and hung the API; event and exec streams have their
own threads. Auto-update and the update endpoint ignored failed pulls.
A deleted git stack was recreated and started by the next webhook or
poll; deleting a stack now drops its git source, auto-update policy and
schedule entries. Deleting no longer walks into mounted shares, is
refused while a backup runs, and removes the containers even when the
compose file is broken. Stack folders compose has to normalise
(Nextcloud -> nextcloud) work everywhere and group correctly on the
volumes, networks and images pages.

Detection. Host port conflicts read the container's own network
namespace; /proc/1/net is read instead. Images referenced as docker.io/…
were never update-checked; local builds no longer report "needs
credentials". NVIDIA GPUs are read from the driver's procfs. The backup
dialog's "path mismatch" warning came from mountinfo's filesystem-relative
root and fired for any stacks folder on its own disk, ZFS dataset or
btrfs subvolume; the host path now comes from Docker's mounts.

Backups and destinations. The download ignored the folder/volume
selection (axios sent binds[]). Restoring under a new name kept the old
one. A report header past nginx's buffer turned a good backup into a
502. SFTP/S3 errors were bare 500s and "Test" did not try to write.
A destination used by a schedule could be deleted. Backup file names
are in the schedule's or the browser's time zone instead of UTC.

Editor and converter. `docker run` conversion produced invalid compose
for --network host, named volumes and -dit; --gpus and --mount were
dropped; a multi-line command pasted into the one-line field became
`image: ' '` (now a textarea, and flattened continuations are read).
Validation failed for env_file stacks. The editor overwrote unsaved
edits on every container event, lost them on a stray click (now asks),
threw when closing the diff, and its helper panel listed the starter
service "app" for every existing stack. Monaco is bundled instead of
loaded from jsdelivr, so the editor works offline (and monaco-editor
0.57 drops the dompurify advisory; axios and react-router updated).

UI. All times were two hours off: offset-less UTC from SQLite was read
as local time, and "next run" always said "just now". A token refresh
mid-session duplicated deploys, reset terminals and doubled logs. A
backend restart (update, self-update) signed everyone out on reload; the
page now waits for it. The editor was unusable on a phone, tables and
tabs overflowed, dialog text was black on the dark theme, and the file
browser opened on an error with the default roots. Read-only users get
"Admin only" pages instead of 403s; in-use networks offer no delete;
"Scan all" covers StackPilot's own stacks only and runs in the
background instead of timing out at 600s.

Deployment. nginx passes the port in Host (the OIDC redirect URI lost
:5009) and keeps an outer proxy's X-Forwarded-Proto. The frontend waits
for a healthy backend (healthcheck probes every 2s while starting). The
frontend image builds with npm ci. Secrets are 0644 in a 0700 folder so
non-root containers can read them; existing files are fixed at startup.
The test suite no longer starts real containers on a host with Docker.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fx7gcuy8d6nPd95g43rGHu

backend/services/backup_destination_service.py

@@ -76,24 +76,169 @@ def migrate_plaintext_configs(session) -> int:
 # --------------------------------------------------------------------------- #
 
 
-def _sftp_connect(cfg: dict):
+#: Seconds to wait for the SFTP server to accept the TCP connection.
+SFTP_CONNECT_TIMEOUT = 20
+
+
+def _check_file_name(name: str) -> None:
+    """A backup is addressed by its bare file name, never a path.
+
+    The name comes from the API (restore from / delete on a destination); with
+    ``../`` in it a delete reached files outside the backup folder — on SFTP
+    anything the account may write. NFS has always checked this.
+    """
+    if (
+        not name
+        or name in (".", "..")
+        or "/" in name
+        or "\\" in name
+        or "\0" in name
+    ):
+        raise DestinationError(f"Invalid backup file name '{name}'")
+
+
+def host_key_fingerprint(key) -> str:
+    """OpenSSH's form: ``SHA256:`` + unpadded base64 of the key's SHA-256."""
+    import base64
+    import hashlib
+
+    digest = hashlib.sha256(key.asbytes()).digest()
+    return "SHA256:" + base64.b64encode(digest).decode().rstrip("=")
+
+
+def fingerprint_of(pinned: str) -> str:
+    """The ``SHA256:`` fingerprint of a stored host key (as given, if it is
+    one already) — what the UI shows and ``ssh-keygen -lf`` prints."""
+    import base64
+    import hashlib
+
+    pinned = pinned.strip()
+    if pinned.startswith("SHA256:"):
+        return pinned
+    parts = pinned.split()
+    for i, part in enumerate(parts[:-1]):
+        if part.startswith(("ssh-", "ecdsa-", "sk-")):
+            try:
+                blob = base64.b64decode(parts[i + 1])
+            except ValueError:
+                break
+            return "SHA256:" + base64.b64encode(hashlib.sha256(blob).digest()).decode().rstrip("=")
+    return pinned
+
+
+def _pinned_type(pinned: str) -> Optional[str]:
+    """The key type of a pinned ``<type> <base64>`` line (a known_hosts line
+    with the host in front works too); None for a bare fingerprint."""
+    parts = pinned.split()
+    for part in parts[:-1]:
+        if part.startswith(("ssh-", "ecdsa-", "sk-")):
+            return part
+    return None
+
+
+#: What paramiko negotiates for a key of each type. An RSA key is offered
+#: under the SHA-2 signature names, not under its own.
+_HOST_KEY_ALGORITHMS = {
+    "ssh-rsa": ("rsa-sha2-512", "rsa-sha2-256", "ssh-rsa"),
+}
+
+
+def _host_key_matches(pinned: str, key) -> bool:
+    pinned = pinned.strip()
+    if pinned.startswith("SHA256:"):
+        return pinned.rstrip("=") == host_key_fingerprint(key)
+    parts = pinned.split()
+    for i, part in enumerate(parts[:-1]):
+        if part == key.get_name() and parts[i + 1] == key.get_base64():
+            return True
+    return False
+
+
+def _sftp_connect(cfg: dict, pin=None):
+    """Connect and authenticate, checking the server's host key.
+
+    Before 0.67.1 any host key was accepted, so whoever could answer on the
+    server's address (a spoofed DNS entry, a box that took its IP) received
+    the password and every backup. Now the key is pinned in the destination's
+    ``host_key``: entered by hand (a key line or its ``SHA256:`` fingerprint),
+    or — when empty — the one seen on the first connection, handed to ``pin``
+    to be stored. After that a different key refuses the connection.
+    """
+    import socket
+
     import paramiko
 
     host = cfg.get("host")
     if not host:
         raise DestinationError("SFTP host is required")
-    port = int(cfg.get("port") or 22)
-    username = cfg.get("username")
-    transport = paramiko.Transport((host, port))
     try:
+        port = int(cfg.get("port") or 22)
+    except (TypeError, ValueError) as exc:
+        raise DestinationError(f"Invalid SFTP port '{cfg.get('port')}'") from exc
+    username = cfg.get("username")
+    pinned = (cfg.get("host_key") or "").strip()
+    # Connected here, with a timeout, rather than by Transport((host, port)):
+    # that connects inside the constructor, outside any handler, so an
+    # unreachable host was a bare 500 instead of a message — and with no
+    # timeout it hung for the kernel's two minutes first.
+    try:
+        sock = socket.create_connection((host, port), timeout=SFTP_CONNECT_TIMEOUT)
+    except OSError as exc:
+        raise DestinationError(f"SFTP connection to {host}:{port} failed: {exc}") from exc
+    transport = paramiko.Transport(sock)
+    try:
+        key_type = _pinned_type(pinned) if pinned else None
+        if key_type:
+            # Ask for the pinned kind of key: a server has several, and the
+            # one negotiated by default need not be the one that was pinned.
+            wanted = _HOST_KEY_ALGORITHMS.get(key_type, (key_type,))
+            options = transport.get_security_options()
+            usable = tuple(k for k in wanted if k in options.key_types)
+            if usable:
+                options.key_types = usable
+        transport.start_client(timeout=SFTP_CONNECT_TIMEOUT)
+        server_key = transport.get_remote_server_key()
+        if pinned:
+            if not _host_key_matches(pinned, server_key):
+                raise DestinationError(
+                    f"SFTP host key of {host}:{port} does not match the stored one: the "
+                    f"server now presents {server_key.get_name()} "
+                    f"{host_key_fingerprint(server_key)}. If the server was reinstalled or "
+                    "its keys were rotated, clear the destination's host key to trust the "
+                    "new one — otherwise something else is answering on that address."
+                )
+        elif pin is not None:
+            pin(f"{server_key.get_name()} {server_key.get_base64()}")
         pkey = None
         if cfg.get("private_key"):
             pkey = _load_key(cfg["private_key"])
-        transport.connect(username=username, password=cfg.get("password") or None, pkey=pkey)
+        # What Transport.connect did: the key when there is one, else the password.
+        if pkey is not None:
+            transport.auth_publickey(username, pkey)
+        elif cfg.get("password"):
+            transport.auth_password(username, cfg["password"])
+        return paramiko.SFTPClient.from_transport(transport), transport
     except Exception as exc:  # noqa: BLE001
         transport.close()
+        if isinstance(exc, DestinationError):
+            raise
         raise DestinationError(f"SFTP connection failed: {exc}") from exc
-    return paramiko.SFTPClient.from_transport(transport), transport
+
+
+def _sftp_io(what: str):
+    """Turn what paramiko raises mid-transfer into a DestinationError."""
+    import contextlib
+
+    @contextlib.contextmanager
+    def wrapper():
+        try:
+            yield
+        except DestinationError:
+            raise
+        except Exception as exc:  # noqa: BLE001 - IOError, SSHException, EOFError …
+            raise DestinationError(f"SFTP {what} failed: {exc}") from exc
+
+    return wrapper()
 
 
 def _load_key(key_str: str):
@@ -120,22 +265,24 @@ def _sftp_makedirs(sftp, path: str) -> None:
             sftp.mkdir(cur)
 
 
-def _sftp_upload(cfg: dict, local_path: str, filename: str) -> str:
-    sftp, transport = _sftp_connect(cfg)
+def _sftp_upload(cfg: dict, local_path: str, filename: str, pin=None) -> str:
+    _check_file_name(filename)
+    sftp, transport = _sftp_connect(cfg, pin)
     try:
-        base = cfg.get("path") or "."
-        if base not in (".", ""):
-            _sftp_makedirs(sftp, base)
-        remote = posixpath.join(base, filename) if base not in (".", "") else filename
-        sftp.put(local_path, remote)
-        return remote
+        with _sftp_io("upload"):
+            base = cfg.get("path") or "."
+            if base not in (".", ""):
+                _sftp_makedirs(sftp, base)
+            remote = posixpath.join(base, filename) if base not in (".", "") else filename
+            sftp.put(local_path, remote)
+            return remote
     finally:
         sftp.close()
         transport.close()
 
 
-def _sftp_list(cfg: dict) -> list[dict]:
-    sftp, transport = _sftp_connect(cfg)
+def _sftp_list(cfg: dict, pin=None) -> list[dict]:
+    sftp, transport = _sftp_connect(cfg, pin)
     try:
         base = cfg.get("path") or "."
         out = []
@@ -155,23 +302,51 @@ def _sftp_list(cfg: dict) -> list[dict]:
         transport.close()
 
 
-def _sftp_download(cfg: dict, name: str, local_path: str) -> None:
-    sftp, transport = _sftp_connect(cfg)
+def _sftp_download(cfg: dict, name: str, local_path: str, pin=None) -> None:
+    _check_file_name(name)
+    sftp, transport = _sftp_connect(cfg, pin)
     try:
-        base = cfg.get("path") or "."
-        remote = posixpath.join(base, name) if base not in (".", "") else name
-        sftp.get(remote, local_path)
+        with _sftp_io(f"download of '{name}'"):
+            base = cfg.get("path") or "."
+            remote = posixpath.join(base, name) if base not in (".", "") else name
+            sftp.get(remote, local_path)
     finally:
         sftp.close()
         transport.close()
 
 
-def _sftp_delete(cfg: dict, name: str) -> None:
-    sftp, transport = _sftp_connect(cfg)
+def _sftp_delete(cfg: dict, name: str, pin=None) -> None:
+    _check_file_name(name)
+    sftp, transport = _sftp_connect(cfg, pin)
     try:
-        base = cfg.get("path") or "."
-        remote = posixpath.join(base, name) if base not in (".", "") else name
-        sftp.remove(remote)
+        with _sftp_io(f"delete of '{name}'"):
+            base = cfg.get("path") or "."
+            remote = posixpath.join(base, name) if base not in (".", "") else name
+            sftp.remove(remote)
+    finally:
+        sftp.close()
+        transport.close()
+
+
+_TEST_FILE = ".stackpilot-test"
+
+
+def _sftp_test(cfg: dict, pin=None) -> None:
+    """Connect, create the folder if needed, write and remove a file.
+
+    Listing alone said "OK" for a folder that does not exist or cannot be
+    written to — the first scheduled backup was then the one to find out.
+    """
+    sftp, transport = _sftp_connect(cfg, pin)
+    try:
+        with _sftp_io("write test"):
+            base = cfg.get("path") or "."
+            if base not in (".", ""):
+                _sftp_makedirs(sftp, base)
+            remote = posixpath.join(base, _TEST_FILE) if base not in (".", "") else _TEST_FILE
+            with sftp.open(remote, "w") as fh:
+                fh.write(b"ok")
+            sftp.remove(remote)
     finally:
         sftp.close()
         transport.close()
@@ -188,13 +363,16 @@ def _s3_client(cfg: dict):
     bucket = cfg.get("bucket")
     if not bucket:
         raise DestinationError("S3 bucket is required")
-    return boto3.client(
-        "s3",
-        endpoint_url=cfg.get("endpoint_url") or None,
-        region_name=cfg.get("region") or None,
-        aws_access_key_id=cfg.get("access_key") or None,
-        aws_secret_access_key=cfg.get("secret_key") or None,
-    )
+    try:
+        return boto3.client(
+            "s3",
+            endpoint_url=cfg.get("endpoint_url") or None,
+            region_name=cfg.get("region") or None,
+            aws_access_key_id=cfg.get("access_key") or None,
+            aws_secret_access_key=cfg.get("secret_key") or None,
+        )
+    except Exception as exc:  # noqa: BLE001 - e.g. a malformed endpoint URL
+        raise DestinationError(f"Invalid S3 settings: {exc}") from exc
 
 
 def _s3_key(cfg: dict, filename: str) -> str:
@@ -203,6 +381,7 @@ def _s3_key(cfg: dict, filename: str) -> str:
 
 
 def _s3_upload(cfg: dict, local_path: str, filename: str) -> str:
+    _check_file_name(filename)
     client = _s3_client(cfg)
     key = _s3_key(cfg, filename)
     try:
@@ -241,6 +420,7 @@ def _s3_list(cfg: dict) -> list[dict]:
 
 
 def _s3_download(cfg: dict, name: str, local_path: str) -> None:
+    _check_file_name(name)
     client = _s3_client(cfg)
     try:
         client.download_file(cfg["bucket"], _s3_key(cfg, name), local_path)
@@ -249,8 +429,24 @@ def _s3_download(cfg: dict, name: str, local_path: str) -> None:
 
 
 def _s3_delete(cfg: dict, name: str) -> None:
+    _check_file_name(name)
     client = _s3_client(cfg)
-    client.delete_object(Bucket=cfg["bucket"], Key=_s3_key(cfg, name))
+    try:
+        client.delete_object(Bucket=cfg["bucket"], Key=_s3_key(cfg, name))
+    except Exception as exc:  # noqa: BLE001
+        raise DestinationError(f"S3 delete failed: {exc}") from exc
+
+
+def _s3_test(cfg: dict) -> None:
+    """List, then write and delete a small object: proves the keys may write."""
+    _s3_list(cfg)
+    client = _s3_client(cfg)
+    key = _s3_key(cfg, _TEST_FILE)
+    try:
+        client.put_object(Bucket=cfg["bucket"], Key=key, Body=b"ok")
+        client.delete_object(Bucket=cfg["bucket"], Key=key)
+    except Exception as exc:  # noqa: BLE001
+        raise DestinationError(f"S3 write test failed: {exc}") from exc
 
 
 # --------------------------------------------------------------------------- #
@@ -354,6 +550,23 @@ def _nfs_volume(dest: BackupDestination, cfg: dict) -> str:
     return name
 
 
+def forget_nfs_volume(dest_id: int) -> None:
+    """Remove the Docker volume of a deleted NFS destination (best effort).
+
+    Left behind, it kept the old mount options under a name the next
+    destination with the same id would pick up again.
+    """
+    from docker_client import DockerError, get_client, safe_call
+
+    name = f"{_NFS_VOLUME_PREFIX}{dest_id}"
+    try:
+        client = get_client()
+        clear_nfs_leftovers(client, name)
+        safe_call(safe_call(client.volumes.get, name).remove)
+    except DockerError as exc:
+        logger.info("NFS volume %s not removed: %s", name, exc.detail or exc.error)
+
+
 def clear_nfs_leftovers(client, volume: Optional[str] = None) -> int:
     """Remove stopped containers holding an NFS destination volume.
 
@@ -566,10 +779,38 @@ def _nfs_test(dest: BackupDestination, cfg: dict) -> bool:
 # --------------------------------------------------------------------------- #
 
 
+def _pinner(dest: BackupDestination):
+    """Store the first host key an SFTP destination presents (trust on first
+    use). Written straight to the row — the callers are a scheduled run and a
+    "Test" click, and neither holds a session that will commit."""
+
+    def pin(host_key: str) -> None:
+        if dest.id is None:
+            return  # not saved yet: nothing to pin it to
+        from sqlmodel import Session
+
+        from database import engine
+
+        with Session(engine) as session:
+            row = session.get(BackupDestination, dest.id)
+            if row is None:
+                return
+            cfg = parse_config(row)
+            if (cfg.get("host_key") or "").strip():
+                return  # pinned meanwhile; the next connect compares against it
+            cfg["host_key"] = host_key
+            row.config = dump_config(cfg)
+            session.add(row)
+            session.commit()
+        logger.info("Destination '%s': pinned SFTP host key %s", dest.name, host_key.split()[0])
+
+    return pin
+
+
 def upload(dest: BackupDestination, local_path: str, filename: str) -> str:
     cfg = parse_config(dest)
     if dest.type == "sftp":
-        return _sftp_upload(cfg, local_path, filename)
+        return _sftp_upload(cfg, local_path, filename, _pinner(dest))
     if dest.type == "s3":
         return _s3_upload(cfg, local_path, filename)
     if dest.type == "nfs":
@@ -580,7 +821,7 @@ def upload(dest: BackupDestination, local_path: str, filename: str) -> str:
 def list_backups(dest: BackupDestination) -> list[dict]:
     cfg = parse_config(dest)
     if dest.type == "sftp":
-        return _sftp_list(cfg)
+        return _sftp_list(cfg, _pinner(dest))
     if dest.type == "s3":
         return _s3_list(cfg)
     if dest.type == "nfs":
@@ -591,7 +832,7 @@ def list_backups(dest: BackupDestination) -> list[dict]:
 def download(dest: BackupDestination, name: str, local_path: str) -> None:
     cfg = parse_config(dest)
     if dest.type == "sftp":
-        _sftp_download(cfg, name, local_path)
+        _sftp_download(cfg, name, local_path, _pinner(dest))
     elif dest.type == "s3":
         _s3_download(cfg, name, local_path)
     elif dest.type == "nfs":
@@ -603,7 +844,7 @@ def download(dest: BackupDestination, name: str, local_path: str) -> None:
 def delete(dest: BackupDestination, name: str) -> None:
     cfg = parse_config(dest)
     if dest.type == "sftp":
-        _sftp_delete(cfg, name)
+        _sftp_delete(cfg, name, _pinner(dest))
     elif dest.type == "s3":
         _s3_delete(cfg, name)
     elif dest.type == "nfs":
@@ -614,7 +855,13 @@ def delete(dest: BackupDestination, name: str) -> None:
 
 def test(dest: BackupDestination) -> bool:
     """Connectivity check — validates reachability, auth and write access."""
+    cfg = parse_config(dest)
     if dest.type == "nfs":
-        return _nfs_test(dest, parse_config(dest))
-    list_backups(dest)
-    return True
+        return _nfs_test(dest, cfg)
+    if dest.type == "sftp":
+        _sftp_test(cfg, _pinner(dest))
+        return True
+    if dest.type == "s3":
+        _s3_test(cfg)
+        return True
+    raise DestinationError(f"Unknown destination type '{dest.type}'")