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

[Ailcope]

Stacked on #23. This branch forks from fix/reliability-cloudflare-ci, so the diff currently shows that PR's 6 commits plus one security commit. It collapses to the single security commit once #23 merges. Stacking is not cosmetic: the suite does not collect on main (test_bmc_registry.py still imports the removed iDRAC/Supermicro stubs -- fixed in #23), and the path-segment guard below conflicts with #23's ProxmoxClient lock.

Why

A full read of src/ -- auth, dashboard (routes + chat engine + templates + JS), proxmox, ssh, bmc, plus deploy/ and the Dockerfile -- rather than a diff review of one branch. Most of the surface held up: SQL is parameterised everywhere, client_id isolation is correct on conversations / usage / tokens / DCR slugs, PKCE S256 is mandatory, the TOTP replay guard from #23 is sound, Jinja autoescaping is on, and the hand-rolled markdown renderer in chat.js escapes before it formats. No eval / pickle / yaml.load / shell=True, no hardcoded secrets.

What it did turn up was one hole that mattered and a set of smaller ones, mostly in seams between PRs that each did their part correctly.

The one that mattered: the chat's human-approval gate (introduced with the unified run tools, tightened in #9) covered ssh_run and proxmox_run -- but not proxmox_write_file. Writing a guest file is code execution in one hop: ~/.ssh/authorized_keys, /etc/cron.d/x. The model's input is not trusted -- a log line pulled by proxmox_read_file, a config file, or a Google Search result can carry an injected instruction -- so a single detoured turn could plant a key on a production VM with no modal, while the equivalent ssh_run call was blocked. Exec through the front door was gated; exec through the side door was not.

The rest are seams:

• BMCDevice.verify_tls was added with Epic B: Universal Hardware & Redfish #17 (Epic B: Universal Hardware & Redfish) and RedfishBackend reads it -- but Config._build() never parsed it from the YAML. verify_tls: true was silently a no-op, so every Redfish call ran with certificate verification off while sending the BMC admin password in Basic Auth on each request.
• fix: review follow-ups — per-host SSH host-key, transfer guards, durable tokens + audit wiring #22 chmod'd clients.json and tokens.db to 0600. dashboard.db was not in that pass, and it stores mcp_bearer in plaintext (only client_secret is AES-GCM encrypted). Any local user could lift a live bearer out of it.
• The redirect-URI allowlist that unified CORS and OAuth trust matched loopback callbacks by string prefix, so http://localhost:1@attacker.example/cb passed is_trusted_redirect_uri. /oauth/authorize re-checked the parsed hostname downstream and caught it; the DCR register endpoint added later validated only through that helper, and did not.

Changes

Confirmation gate (dashboard/chat.py)

• _NEEDS_CONFIRMATION now covers the code-execution primitives (proxmox_write_file alongside the existing exec and transfer tools) and the destructive ones (vm_bulk_action, proxmox_vm_stop / _restart / _migrate, proxmox_snapshot_rollback / _delete, proxmox_backup_restore, bmc_power_off, bmc_power_reset).
• New _CONFIRM_WHEN_ARG_PRESENT gates proxmox_vm_config only when updates is set, so reading a config does not raise a modal. dry_run=True calls and exec_id-only polling still skip it.
• docs/dashboard.md and the README security section rewritten to match.

BMC

• verify_tls parsed for real, surfaced in validate-config, documented with a worked example in beaconmcp.yaml.example. Default stays false -- BMCs ship self-signed.
• ipmitool no longer receives the password as -P <pw>; argv is world-readable through /proc/<pid>/cmdline for the duration of the call. Now -E + IPMI_PASSWORD in the child environment.

File permissions

• dashboard.db and its -wal / -shm sidecars chmod 0600, re-asserted on each connection because WAL recreates the sidecars per worker thread.
• chmod 600 on the .env written by deploy/install.sh and appended by beaconmcp init -- it holds every Proxmox token, BMC password, SSH password and the session key.
• UMask=0077, NoNewPrivileges=true, PrivateTmp=true on the systemd unit, to close the window before each explicit chmod lands.

Input validation

• is_trusted_redirect_uri resolves the parsed hostname for loopback callbacks instead of prefix-matching the string. Custom schemes (vscode://, cursor://) stay prefix-matched -- there the scheme is the authority. Plain-HTTP callbacks no longer fall through to the origin allowlist.
• ProxmoxClient.api_call rejects path segments that are not plain segments before issuing the request. Every endpoint is an f-string with caller-supplied values spliced in (.../snapshot/{snapname}/rollback, .../storage/{storage}/content); node was already constrained to a configured node, snapname / storage / archive were not.

Smaller

• audit._REDACT_KEYS gains session_id, code_verifier, totp_secret, session_key, access_token, private_key, passwd, client_secret_hash. Non-secret identifiers such as client_id stay readable.
• /app/overview and /app/usage passed _render's arguments in the wrong order and 500'd on every request. Fixed in passing.

Known and left alone

Deliberately not changed here, each for a stated reason:

• /metrics unauthenticated -- documented in-code as network-ACL-controlled, no label leaks a secret, and changing it breaks existing scrapes.
• beaconmcp.service runs as User=root -- needed to write /opt/beaconmcp as things stand. Moving to a dedicated user is a permissions migration, not a patch.
• mcp_bearer stored in plaintext in dashboard.db -- mitigated by the 0600 change above; encrypting it like client_secret is a schema change.
• _issuer() trusts Host / X-Forwarded-Host -- OAuth discovery URLs are poisonable in theory, but the client sets its own Host and server.allowed_hosts already covers /mcp. A real fix anchors the issuer on explicit config.
• _validate_authorize_params rejects custom schemes (parsed.scheme not in ("https", "http")), which makes the vscode:// / cursor:// entries in the trust list dead. That is a functional bug; fixing it loosens security, so it is flagged rather than changed.

Related

Builds on the security work in #11 (SSH host-key verification, per-IP auth rate limiting), #12 (structured audit log), #22 (owner-only clients.json / tokens.db, audit redaction) and #23 (thread-safe TokenStore, TOTP replay protection). Fixes seams left by #17 (verify_tls) and #22 (dashboard.db).

Tests

341 passed (297 before, +44 added), ruff check src/ tests/ clean, Python 3.12.

New tests/test_security_hardening.py pins each finding so a refactor that reopens one fails there: loopback prefix evasion, traversal-shaped API path segments (with the legitimate shapes real tools build asserted as still-accepted), the confirmation gate in both directions, verify_tls reaching RedfishBackend, dashboard.db permissions, and audit redaction. tests/test_bmc_ipmi.py gains an assertion that the BMC password never appears in argv.

[Copilot]

Pull request overview

This PR hardens BeaconMCP against several security-audit findings across OAuth redirect validation, chat-tool approval gating, Proxmox API path construction, secrets handling (file permissions + audit redaction), and BMC/IPMI credential exposure. It also adds regression tests and CI/linting to prevent these issues from resurfacing.

Changes:

• Expands the dashboard chat confirmation gate to cover additional code-execution/destructive tools and adds arg-shape-based gating for read-or-write tools.
• Tightens OAuth redirect trust (loopback host parsing), adds Proxmox API path-segment validation, and improves secret-handling (audit redaction, ipmitool password via env, owner-only DB/.env permissions, systemd hardening).
• Adds comprehensive regression tests plus CI (ruff + pytest) and supporting documentation updates.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_utils.py	Removes unused import.
tests/test_totp_replay.py	Adds replay-protection tests for TOTP verification.
tests/test_token_store.py	Adds TokenStore behavior + thread-safety tests.
tests/test_snapshots.py	Removes unused pytest import in snapshot tool tests.
tests/test_security_hardening.py	Adds security regression suite covering redirect validation, path guards, confirmation gate, verify_tls, db perms, and audit redaction.
tests/test_proxmox_client.py	Adds concurrency tests for ProxmoxClient connection-cache locking.
tests/test_integration.py	Fixes several string formatting / argument issues in integration test output.
tests/test_dashboard_chat.py	Removes unused import and normalizes inline imports.
tests/test_cloudflare_diagnostic.py	Adds tests for Cloudflare-specific 401 diagnostic hint behavior.
tests/test_bmc_registry.py	Updates registry expectations to use RedfishBackend and adds unreachable-backend error-path test.
tests/test_bmc_ipmi.py	Verifies IPMI password never appears on argv; asserts env-based passing.
src/beaconmcp/wizard.py	Ensures .env produced/updated by beaconmcp init is chmod 0600.
src/beaconmcp/server.py	Wraps all tools to run sync functions in worker threads while preserving metrics/audit emission.
src/beaconmcp/proxmox/system.py	Removes unused import.
src/beaconmcp/proxmox/client.py	Adds API path-segment validation and connection-cache locking.
src/beaconmcp/dashboard/db.py	Enforces owner-only permissions for dashboard.db and WAL/SHM sidecars.
src/beaconmcp/dashboard/chat.py	Expands confirmation gating and adds arg-shape-based gating + dry_run bypass.
src/beaconmcp/dashboard/app.py	Fixes _render call argument passing for overview/usage routes.
src/beaconmcp/config.py	Parses and surfaces bmc.devices[].verify_tls correctly.
src/beaconmcp/bmc/redfish.py	Minor cleanup (removes unused import).
src/beaconmcp/bmc/ipmi.py	Switches ipmitool password passing from argv (-P) to env (-E + IPMI_PASSWORD).
src/beaconmcp/auth.py	Fixes redirect-uri loopback trust by parsing hostname; adds TOTP replay protection; improves TokenStore locking.
src/beaconmcp/audit.py	Expands redaction key set for OAuth/session/token material.
src/beaconmcp/main.py	Adds Cloudflare-edge diagnostic hint in unauthorized bodies and preserves OAuth discovery headers.
README.md	Updates security guidance and Cloudflare deployment note.
pyproject.toml	Adds anyio dependency, dev extras, and ruff/pytest configuration.
docs/troubleshooting.md	Adds Cloudflare-specific 401/403 troubleshooting entry.
docs/dashboard.md	Updates and expands “dangerous tool” confirmation documentation.
docs/cloudflare.md	Adds detailed Cloudflare deployment guidance (WAF/Access/caching).
deploy/install.sh	Ensures .env is chmod 0600 during install.
deploy/beaconmcp.service	Adds UMask=0077 and additional systemd hardening flags.
beaconmcp.yaml.example	Documents and exemplifies verify_tls for Redfish-capable BMCs.
.github/workflows/ci.yml	Adds CI workflow running ruff + pytest on Python 3.11/3.12.

Comments suppressed due to low confidence (1)

src/beaconmcp/proxmox/client.py:47

• The exception message hard-codes the allowed character-class as [A-Za-z0-9._@+-]*. If _SAFE_PATH_SEGMENT is updated to allow underscores (to match Proxmox reality), update this message too so operators see accurate guidance when a segment is rejected.

            raise UnsafePathSegmentError(
                f"illegal Proxmox API path segment {part!r} in {path!r}: "
                "segments must match [A-Za-z0-9][A-Za-z0-9._@+-]* "
                "(no slashes, no '..')"
            )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Ailcope]

Not reproducible -- _ is already in the class. [A-Za-z0-9._@+-] is dot, underscore, at, plus, hyphen; it reads as if the underscore were missing, but it is the second character.

>>> from beaconmcp.proxmox.client import _SAFE_PATH_SEGMENT as R
>>> [(s, bool(R.match(s))) for s in ("pre-upgrade_2024", "local_lvm", "snap_1", "_leading")]
[("pre-upgrade_2024", True), ("local_lvm", True), ("snap_1", True), ("_leading", False)]

So underscores are accepted anywhere after the first character, and only a leading underscore is rejected -- deliberately, because api_call walks the path with getattr() and a leading _ would reach proxmoxer's private attributes rather than building a URL segment.

test_legitimate_paths_are_not_rejected in tests/test_security_hardening.py already pins exactly this with nodes/pve1/lxc/200/snapshot/pre-upgrade_2024/rollback, and it passes -- had the class actually excluded _, that assertion would be red rather than green.

The error message is accurate as written for the same reason, so no change here.

[Showdown76py]

Relu en entier. Le gros du travail tient : la regex de segments Proxmox ne casse aucun chemin réellement construit (j'ai vérifié que les UPID et les volid du type local:backup/... passent en kwargs, jamais en segment), le passage au hostname parsé pour les callbacks loopback gère bien http://[::1]:... et la casse, ipmitool -E, verify_tls, les chmod 0600 avec la ré-assertion pour les sidecars WAL, la redaction d'audit, tout est correct. Le fix _render sur /app/overview et /app/usage est réel, l'ordre des arguments faisait passer request comme nom de template.

Un point bloquant par contre, que j'ai corrigé directement sur la branche (dd0cfd6).

L'échappatoire dry_run dans _tool_call_requires_confirmation s'appliquait à tous les outils gardés, alors que seuls les trois outils snapshot déclarent le paramètre. Et FastMCP valide les arguments avec un BaseModel pydantic nu, donc extra='ignore' : un dry_run non déclaré est jeté silencieusement côté serveur. Vérifié :

call_fn_with_arg_validation(ssh_run, False,
    {'host': 'h1', 'command': 'rm -rf /', 'dry_run': True}, None)
→ EXECUTED rm -rf / on h1

Donc ssh_run(command="...", dry_run=True) passait la modale, l'argument disparaissait, et la commande s'exécutait pour de vrai. Même chose pour proxmox_write_file, vm_bulk_action, bmc_power_off, proxmox_backup_restore. C'est une régression sur une garde qui existait avant la PR, et elle est atteignable par exactement l'instruction injectée que la garde est là pour arrêter : il suffit d'un mot de plus.

Le correctif restreint l'échappatoire à un _DRY_RUN_AWARE explicite. Deux tests le figent : la forme de contournement sur six outils qui n'ont pas le paramètre, et une vérification que chaque outil de l'allow-list le déclare vraiment, pour que la liste ne dérive pas au fil des renommages.

J'ai aussi mis un skip Windows sur test_dashboard_db_is_owner_only, où os.chmod ne gère que le bit lecture seule. Note au passage : test_audit_log_wiring et test_token_persistence ont le même souci et échouent déjà sur main, à traiter séparément.

Deux remarques mineures, rien qui bloque :

• Tout callback en HTTP simple non-loopback est maintenant rejeté même s'il figure dans allowed_origins. C'est le bon choix, mais ça casse un déploiement qui aurait une origine http:// interne, ça mérite une ligne dans les notes de version.
• La description mentionne archive parmi les valeurs splicées en segment de chemin. Il part en kwarg dans proxmox_backup_restore, il n'a jamais été concerné.

Merci pour l'audit, la partie verify_tls en particulier était un vrai angle mort.

[Showdown76py]

Le bypass dry_run est corrigé et figé par des tests (dd0cfd6), le job config a récupéré son stub d'env (6e56320). Suite complète verte en local sous mcp 1.27 : 346 passés, ruff clean. Le job test reste rouge à cause de mcp 2.0.0 qui a supprimé mcp.server.fastmcp, cassure déjà présente sur main et traitée à part.