fix(security): close findings from a full security audit

.github/workflows/ci.yml

@@ -65,6 +65,7 @@ jobs:
           PVE2_TOKEN_SECRET: ci-dummy
           RACK1_ILO_PASSWORD: ci-dummy
           RACK2_IPMI_PASSWORD: ci-dummy
+          RACK3_IDRAC_PASSWORD: ci-dummy
           VPS2_PW: ci-dummy
         run: beaconmcp validate-config --config beaconmcp.yaml.example
 

beaconmcp.yaml.example

@@ -208,6 +208,18 @@ bmc:
       password: ${RACK2_IPMI_PASSWORD}
       # no jump_host = direct connection
 
+    - id: rack3-idrac
+      type: idrac            # idrac | supermicro | redfish -> Redfish over HTTPS
+      host: 192.168.10.22
+      user: root
+      password: ${RACK3_IDRAC_PASSWORD}
+      # Verify the BMC's TLS certificate on Redfish calls. Defaults to
+      # false because BMCs ship self-signed certs out of the factory; the
+      # admin password is sent on every request, so turn this on (after
+      # installing a cert the host trusts) for any BMC that is not on a
+      # fully trusted management VLAN.
+      verify_tls: false
+
 features:
   dashboard:
     enabled: true

deploy/beaconmcp.service

@@ -11,5 +11,13 @@ ExecStart=/opt/beaconmcp/.venv/bin/python -m beaconmcp serve
 Restart=on-failure
 RestartSec=5
 
+# Files the server creates at runtime (clients.json, tokens.db, dashboard.db,
+# audit.log) all hold secrets. Default umask 0022 would make them
+# world-readable before the explicit chmod calls land; 0077 closes that
+# window and covers anything not chmod'd individually.
+UMask=0077
+NoNewPrivileges=true
+PrivateTmp=true
+
 [Install]
 WantedBy=multi-user.target

deploy/install.sh

@@ -49,6 +49,11 @@ if [ ! -f "$INSTALL_DIR/.env" ]; then
 else
     echo "[*] Existing .env preserved."
 fi
+# The .env holds every secret the server has: Proxmox API tokens, BMC admin
+# passwords, SSH passwords, the dashboard session key. `cp` and the shell
+# create it with the default umask (0644), which leaves it readable by every
+# local user. Make it owner-only, on fresh and pre-existing installs alike.
+chmod 600 "$INSTALL_DIR/.env"
 
 # 5.b beaconmcp.yaml config file
 if [ ! -f "$INSTALL_DIR/beaconmcp.yaml" ]; then

docs/dashboard.md

@@ -68,16 +68,28 @@ Constraints:
 - **Thinking effort** — dropdown with `minimal` / `low` / `medium` / `high`. `gemini-2.5-pro` cannot disable thinking, so `minimal` is clamped to the 128-token floor automatically.
 - **Markdown rendering** — the client parses headings (`#`–`######`), ordered and unordered lists, blockquotes, horizontal rules, code fences (with `lang-*` class), inline code, bold/italic/strikethrough, and HTTP(S) links.
 
-## Mandatory confirmation for shell-capable tools
+## Mandatory confirmation for dangerous tools
 
-`ssh_run` and `proxmox_run` **never** run a command without manual approval from the chat UI. A call carrying only `exec_id=` is treated as read-only polling and skips the modal; any call that carries a `command` triggers the gate. When Gemini fires a gated call:
+The model's input is untrusted: a log line, a config file, or a web-search result can carry an injected instruction. So two classes of tool **never** run without manual approval from the chat UI.
+
+**Arbitrary code execution** — `ssh_run`, `proxmox_run`, and the primitives that become code execution in one hop (`proxmox_write_file`, `proxmox_upload_file`, `proxmox_download_file`, `proxmox_delete_transfer`). Writing `~/.ssh/authorized_keys` or a file under `/etc/cron.d` is exactly as good as a shell, which is why the file tools sit alongside the exec tools.
+
+**Destructive or irreversible** — `vm_bulk_action`, `proxmox_vm_stop`, `proxmox_vm_restart`, `proxmox_vm_migrate`, `proxmox_snapshot_rollback`, `proxmox_snapshot_delete`, `proxmox_backup_restore`, `bmc_power_off`, `bmc_power_reset`.
+
+Three call shapes are let through without a modal, because they don't change anything:
+
+- `ssh_run` / `proxmox_run` carrying only `exec_id=` — that's read-only polling of an already-approved session.
+- `proxmox_snapshot_create` / `_rollback` / `_delete` called with `dry_run=True` — they only report what they *would* do. The exemption is limited to those three by name, never inferred from the argument: an undeclared `dry_run` is silently dropped during argument validation, so trusting it would let `ssh_run(command=..., dry_run=True)` past the modal and then run for real.
+- `proxmox_vm_config` without `updates` — the read shape of a read-or-write tool.
+
+When Gemini fires a gated call:
 
 1. The tool card switches to an "approval required" state (orange badge, auto-expanded so arguments are visible).
 2. Two buttons: **Approve** / **Reject**.
 3. The Gemini turn blocks server-side until the decision is made (5-minute timeout).
 4. On rejection, Gemini receives a `FunctionResponse {"error": "user_rejected"}` and can revise its reply.
 
-The allow-list is hard-coded in `src/beaconmcp/dashboard/chat.py` (`_NEEDS_CONFIRMATION` + `_tool_call_requires_confirmation`). Only the integrated chat enforces this gate; external MCP clients (Assistant Desktop, Gemini CLI, ChatGPT MCP) must enable their own per-call approval mode (see the **Security** section of the root README).
+The allow-list is hard-coded in `src/beaconmcp/dashboard/chat.py` (`_NEEDS_CONFIRMATION`, `_CONFIRM_WHEN_ARG_PRESENT`, `_tool_call_requires_confirmation`). Only the integrated chat enforces this gate; external MCP clients (Assistant Desktop, Gemini CLI, ChatGPT MCP) must enable their own per-call approval mode (see the **Security** section of the root README).
 
 ## Stored data
 

docs/security.md

@@ -18,10 +18,16 @@ stop` was meant.
   `*_status`, `proxmox_get_logs`). They cannot break anything and are never gated behind a
   confirmation.
 
-The integrated chat at `/app/chat` already forces a human confirmation for every `ssh_run` /
-`proxmox_run` call that carries a `command`; polling-only calls that pass just an `exec_id` are
-read-only and skip the modal. Read the arguments on the confirmation card even when you're clicking
-through fast. No answer within 5 minutes counts as a refusal.
+The integrated chat at `/app/chat` forces a human confirmation for every code-execution tool
+(`ssh_run`, `proxmox_run`, `proxmox_write_file` and the transfer tools) and every destructive one
+(`vm_bulk_action`, `proxmox_vm_stop`, snapshot rollback/delete, backup restore, `bmc_power_off`,
+`bmc_power_reset`). Writing a guest file counts as code execution: `~/.ssh/authorized_keys` and
+`/etc/cron.d/` are one hop from a shell. Skipping the modal is reserved for calls that cannot
+change anything — polling by `exec_id` alone, `dry_run=True` on the snapshot tools that implement
+it, and the read shape of `proxmox_vm_config`. The full list is in
+[dashboard.md](dashboard.md#mandatory-confirmation-for-dangerous-tools). Read the arguments on the
+confirmation card even when you're clicking through fast. No answer within 5 minutes counts as a
+refusal.
 
 ## Tokens
 

src/beaconmcp/audit.py

@@ -32,8 +32,12 @@
 
 # Argument keys whose values are always masked before emission.
 _REDACT_KEYS = frozenset({
-    "password", "secret", "token", "token_secret", "client_secret",
-    "api_key", "authorization", "totp", "bearer",
+    "password", "passwd", "secret", "token", "token_secret", "client_secret",
+    "client_secret_hash", "api_key", "apikey", "authorization", "totp",
+    "totp_secret", "bearer", "access_token", "refresh_token",
+    # OAuth / session material: a code_verifier or a session_id is as good
+    # as a credential to whoever reads the log.
+    "code_verifier", "session_id", "session_key", "private_key",
 })
 
 

src/beaconmcp/auth.py

@@ -31,6 +31,7 @@
 from enum import Enum
 from pathlib import Path
 from typing import Any
+from urllib.parse import urlparse
 
 import pyotp
 
@@ -103,20 +104,35 @@ def current_client_id() -> str | None:
 CLIENTS_FILE = Path("/opt/beaconmcp/clients.json")
 
 
-# Redirect URI prefixes that are not CORS origins and therefore cannot be
-# modeled via ``server.allowed_origins``.
-TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES: tuple[str, ...] = (
-    # OS-level custom URI schemes used by desktop clients.
+# OS-level custom URI schemes used by desktop clients. Not CORS origins, so
+# they cannot be modeled via ``server.allowed_origins``. Prefix-matched: the
+# scheme itself is the authority (only the registering app can claim it).
+TRUSTED_CUSTOM_SCHEME_PREFIXES: tuple[str, ...] = (
     "vscode://",
     "vscode-insiders://",
     "cursor://",
-    # Local loopback for CLI / terminal clients (RFC 8252 style).
-    "http://localhost:",
-    "http://localhost/",
-    "http://127.0.0.1:",
-    "http://127.0.0.1/",
-    "http://[::1]:",
-    "http://[::1]/",
+)
+
+# Loopback callback hosts for CLI / terminal clients (RFC 8252 style).
+# Matched against the *parsed* hostname, never by string prefix: a prefix
+# test on "http://localhost:" also accepts
+# ``http://localhost:1@attacker.example/cb``, whose real host is
+# attacker.example -- a straight authorization-code exfiltration path.
+TRUSTED_LOOPBACK_HOSTS: frozenset[str] = frozenset(
+    {"localhost", "127.0.0.1", "::1"}
+)
+
+# Kept for backwards compatibility with anything importing the old name.
+TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES: tuple[str, ...] = (
+    TRUSTED_CUSTOM_SCHEME_PREFIXES
+    + (
+        "http://localhost:",
+        "http://localhost/",
+        "http://127.0.0.1:",
+        "http://127.0.0.1/",
+        "http://[::1]:",
+        "http://[::1]/",
+    )
 )
 
 
@@ -128,17 +144,31 @@ def is_trusted_redirect_uri(
 
     Web origins are sourced from ``allowed_origins`` (typically
     ``config.server.allowed_origins``), so redirect trust follows the same
-    operator-controlled allowlist as CORS. Non-origin redirect forms (custom
-    URI schemes and loopback callbacks) are covered by
-    :data:`TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES`.
+    operator-controlled allowlist as CORS. Non-origin redirect forms are
+    covered by :data:`TRUSTED_CUSTOM_SCHEME_PREFIXES` (desktop schemes) and
+    :data:`TRUSTED_LOOPBACK_HOSTS` (RFC 8252 loopback callbacks).
     """
     if not isinstance(redirect_uri, str):
         return False
     redirect_uri = redirect_uri.strip()
     if not redirect_uri:
         return False
-    if any(redirect_uri.startswith(p) for p in TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES):
+    if any(redirect_uri.startswith(p) for p in TRUSTED_CUSTOM_SCHEME_PREFIXES):
         return True
+    # Loopback: resolve the real host instead of trusting the string shape,
+    # so userinfo tricks (``http://localhost:1@evil.example/cb``) don't slip
+    # through as "starts with http://localhost:".
+    try:
+        parsed = urlparse(redirect_uri)
+    except ValueError:
+        return False
+    if parsed.scheme == "http":
+        hostname = (parsed.hostname or "").strip().lower()
+        if hostname in TRUSTED_LOOPBACK_HOSTS:
+            return True
+        # Any other plaintext-HTTP callback is untrusted, full stop --
+        # never fall through to the origin allowlist for it.
+        return False
     if allowed_origins:
         for origin in allowed_origins:
             if not isinstance(origin, str) or not origin:

src/beaconmcp/bmc/ipmi.py

@@ -9,6 +9,7 @@
 from __future__ import annotations
 
 import asyncio
+import os
 from typing import Any
 
 from ..config import BMCDevice, Config
@@ -25,22 +26,38 @@ def __init__(self, device: BMCDevice, config: Config) -> None:
         self._config = config
 
     def _argv(self, *extra: str) -> list[str]:
+        """Build the ipmitool argv.
+
+        The BMC password is deliberately NOT passed as ``-P <password>``:
+        argv is world-readable through ``/proc/<pid>/cmdline`` (``ps aux``),
+        so every local user on the host could read the BMC admin password
+        for as long as the call ran. ``-E`` makes ipmitool pick it up from
+        the ``IPMI_PASSWORD`` environment variable instead, which is only
+        readable by the process owner and root. See :meth:`_env`.
+        """
         return [
             "ipmitool",
             "-H", self._device.host,
             "-U", self._device.user,
-            "-P", self._device.password,
+            "-E",
             "-I", "lanplus",
             *extra,
         ]
 
+    def _env(self) -> dict[str, str]:
+        """Child environment carrying the BMC password out of band."""
+        env = dict(os.environ)
+        env["IPMI_PASSWORD"] = self._device.password or ""
+        return env
+
     async def _run(self, *extra: str) -> dict[str, Any]:
         argv = self._argv(*extra)
         try:
             proc = await asyncio.create_subprocess_exec(
                 *argv,
                 stdout=asyncio.subprocess.PIPE,
                 stderr=asyncio.subprocess.PIPE,
+                env=self._env(),
             )
             stdout_b, stderr_b = await proc.communicate()
         except FileNotFoundError:

src/beaconmcp/config.py

@@ -358,6 +358,11 @@ def _build(cls, raw: dict) -> Config:
                     user=_required(d, "user", f"bmc.devices[{device_id}]"),
                     password=_required(d, "password", f"bmc.devices[{device_id}]"),
                     jump_host=d.get("jump_host"),
+                    # Was declared on the dataclass but never read from the
+                    # YAML, so `verify_tls: true` silently did nothing and
+                    # every Redfish call went out with certificate
+                    # verification disabled. Parse it for real.
+                    verify_tls=_bool(d.get("verify_tls", False)),
                 )
             )
 
@@ -708,6 +713,7 @@ def mask(value: str) -> str:
                         "user": d.user,
                         "password": mask(d.password),
                         "jump_host": d.jump_host,
+                        "verify_tls": d.verify_tls,
                     }
                     for d in self.bmc_devices
                 ],

src/beaconmcp/dashboard/app.py

@@ -485,12 +485,12 @@ async def overview_get(request: Request) -> Response:
         if not _bearer_live(deps, session):
             return RedirectResponse("/app/refresh?next=/app/overview", status_code=302)
         return _render(
-            request,
             "overview.html",
-            {
-                "client_name": deps.client_store.get_name(session.client_id)
-                or "Unknown",
-            },
+            request,
+            client_name=(
+                deps.client_store.get_name(session.client_id)  # type: ignore[attr-defined]
+                or "Unknown"
+            ),
         )
 
     async def usage_get(request: Request) -> Response:
@@ -500,12 +500,12 @@ async def usage_get(request: Request) -> Response:
         if not _bearer_live(deps, session):
             return RedirectResponse("/app/refresh?next=/app/usage", status_code=302)
         return _render(
-            request,
             "usage_cost.html",
-            {
-                "client_name": deps.client_store.get_name(session.client_id)
-                or "Unknown",
-            },
+            request,
+            client_name=(
+                deps.client_store.get_name(session.client_id)  # type: ignore[attr-defined]
+                or "Unknown"
+            ),
         )
 
     async def tokens_get(request: Request) -> Response: