fix(server): refuse passwordless public bind (fail-closed) — salvage of #3758

[nesquena-hermes]

Salvage of #3758 — public-bind-requires-auth hardening

Salvaged from #3758 (thanks @fantasticsquirrel) — this takes only the public-bind-requires-auth guard. The other parts of the original PR were handled separately: the onboarding-XFF and update-check-split hardenings already shipped to master, and the /tmp media-root removal was intentionally dropped (shared screenshots render via the per-session MEDIA: token grant, so it wasn't worth the breakage risk).

What it does

Refuses to start (fail-closed sys.exit(1)) when the server would bind to a public/non-loopback address with no authentication configured, instead of only printing a warning — so a passwordless WebUI can't be exposed to the network by accident.

• Loopback (127.0.0.1 / ::1 / localhost) never trips it.
• Auth-enabled (password or passkey) never trips it.
• Bare-metal dev keeps the existing warn-only behavior (no hard exit) unless explicitly opted in.
• Containers are protected by default (Dockerfile sets HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1).
• Operators who terminate auth at another layer (reverse proxy / VPN / private net) opt out with HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=0.

Crystal-clear refusal message

On the dangerous case it prints a bordered, scannable block: what happened + why it's a risk + three concrete fixes (set a password [recommended] / bind to 127.0.0.1 / explicit opt-out), echoing the detected host. Designed so an existing user hard-stopped on upgrade instantly understands how to recover.

Structure note

The predicate + message live in a new thin module api/bind_guard.py (re-exported as server._public_bind_requires_auth) to keep server.py under its 750-line architectural guard — behavior and API surface are identical to an inline version.

Tests

9 new cases in tests/test_security_review_fixes.py (container-public-passwordless blocks; loopback/auth-enabled/bare-metal-dev exempt; explicit flag on/off; Dockerfile default; message clarity + dynamic host). Full suite: 10,793 passed, only the 4 known pre-existing flakes; test_v050260_docker_invariants.py 53/53.

Co-authored-by: fantasticsquirrel

[greptile-apps]

Greptile Summary

This PR hardens the public-bind safety check from warn-only to fail-closed: a passwordless WebUI about to bind a non-loopback address inside a container now prints an actionable refusal message and calls sys.exit(1) instead of continuing. Loopback binds, auth-enabled servers, and bare-metal dev hosts are unaffected; containers are protected by default via a new Dockerfile env var, and operators with external auth can opt out with HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=0.

• api/bind_guard.py (new): contains the _public_bind_requires_auth predicate (auth→loopback→env-var→container-default priority chain) and the crystal-clear multi-line refusal message; re-exported from server.py to maintain a stable server._public_bind_requires_auth reference used by tests.
• server.py: calls the guard before the existing warn block; the existing [!!] WARNING block is left in place for the warn-only paths, but it also fires when an operator has explicitly opted out via the flag (see comment).
• Nine new tests in test_security_review_fixes.py cover the predicate and message; the Dockerfile invariant is pinned by a dedicated test case.

Confidence Score: 4/5

Safe to merge — the guard itself is correct and containers are protected by default; the only rough edge is cosmetic warning output in the explicit-opt-out path.

The new guard predicate, message, Dockerfile default, and tests are all well-structured and cover the intended scenarios. The one gap is that the pre-existing [!!] WARNING block still fires when an operator has explicitly set HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=0, printing suppression advice that contradicts the flag they already used. This does not affect runtime correctness — the server starts — but could confuse operators who opted out and still see a scary warning with incomplete suppression instructions.

server.py lines 599-605 — the warn-only block needs to be aware of the explicit opt-out flag so it does not fire after an operator has already acknowledged the risk.

Important Files Changed

Filename	Overview
api/bind_guard.py	New module implementing the fail-closed guard predicate and refusal message. Logic is clear, env-var normalization is correct (strip+lower), loopback set is accurate for common cases, and all three fix paths are documented in the message.
server.py	Wires the guard into main() correctly, but the existing warn-only block (lines 599-605) still fires when the operator explicitly sets the flag to 0, producing misleading output that omits the opt-out flag from its own suppression instructions.
Dockerfile	Adds ENV HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1 to enable fail-closed guard by default in containers. Placement and comment are appropriate.
tests/test_security_review_fixes.py	Nine new tests cover the predicate (container/loopback/auth-enabled/bare-metal/flag-on/flag-off), Dockerfile invariant, and message clarity. No test covers the combined main()-level behavior when opt-out is set (the warning-fires-anyway path).
CHANGELOG.md	Adds changelog entry describing the fail-closed guard change with accurate detail.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[server.main startup] --> D[call is_auth_enabled]
    D --> E[call _public_bind_requires_auth]
    E --> F{auth_enabled}
    F -->|yes| ALLOW[return False - no block]
    F -->|no| G{host is loopback}
    G -->|yes| ALLOW
    G -->|no| H{REQUIRE_AUTH env var}
    H -->|0 or false| ALLOW
    H -->|1 or true| BLOCK[return True - block]
    H -->|unset| I{within_container}
    I -->|yes| BLOCK
    I -->|no| ALLOW
    BLOCK --> J[print refusal message]
    J --> K[sys.exit 1]
    ALLOW --> L{non-loopback AND no auth}
    L -->|yes| M[WARNING: NO PASSWORD SET - fires even on explicit opt-out]
    L -->|no| N{no auth at all}
    N -->|yes| O[tip: no password set]
    N -->|no| P[continue startup]
    M --> P
    O --> P

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[server.main startup] --> D[call is_auth_enabled]
    D --> E[call _public_bind_requires_auth]
    E --> F{auth_enabled}
    F -->|yes| ALLOW[return False - no block]
    F -->|no| G{host is loopback}
    G -->|yes| ALLOW
    G -->|no| H{REQUIRE_AUTH env var}
    H -->|0 or false| ALLOW
    H -->|1 or true| BLOCK[return True - block]
    H -->|unset| I{within_container}
    I -->|yes| BLOCK
    I -->|no| ALLOW
    BLOCK --> J[print refusal message]
    J --> K[sys.exit 1]
    ALLOW --> L{non-loopback AND no auth}
    L -->|yes| M[WARNING: NO PASSWORD SET - fires even on explicit opt-out]
    L -->|no| N{no auth at all}
    N -->|yes| O[tip: no password set]
    N -->|no| P[continue startup]
    M --> P
    O --> P

Loading

_{Reviews (1): Last reviewed commit: "Fail closed on passwordless public bind ..." | Re-trigger Greptile}

[greptile-apps]

Warning fires even after explicit operator opt-out

When an operator sets HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=0 on a container host, _public_bind_requires_auth correctly returns False and the hard exit is skipped — but then this warning block fires anyway because its condition knows nothing about the explicit opt-out. The operator then sees [!!] WARNING: Binding to 0.0.0.0 with NO PASSWORD SET. and, worse, To suppress: bind to 127.0.0.1 or set a password., which omits the flag they already used and implies their opt-out didn't work. Reading the env var here (or exposing a helper from api.bind_guard) and skipping the warning when an explicit opt-out is present would fix the misleading output. The container note on line 605 suffers the same issue.

[nesquena-hermes]

⚠️ Gate finding — the Dockerfile default-on guard bricks passwordless containers (Docker smoke RED)

The Docker smoke tests (single / two-container / three-container) fail on this branch, while browser-smoke passes. Root cause from the smoke log:

container hermes-smoke-single-...-hermes-webui-1 is unhealthy
hermes-webui-1  |   REFUSING TO START: passwordless server would be exposed on a public address
hermes-webui-1  |   == Environment variable HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND [1] already set
hermes-webui-1  | !! ERROR: hermes-webui failed or exited with an error

So the guard is working exactly as designed — but the Dockerfile default HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1 means every passwordless container bind now fails closed, including the smoke tests and, more importantly, any existing user who runs the Docker image without a password set would be hard-stopped on upgrade. That's a much larger blast radius than "prevent accidental exposure" — it's "passwordless Docker is now unbootable by default."

The design decision this surfaces (for @nesquena)

The security intent is right, but defaulting the guard ON in the Dockerfile is too aggressive. Options:

1. Don't default it on in the Dockerfile — keep the guard opt-in everywhere (the warn path still fires for passwordless public binds; users who want fail-closed set the env var). Lowest breakage, still ships the capability. (my lean)
2. Keep default-on but make the smoke tests + the shipped Docker quickstart set HERMES_WEBUI_PASSWORD — and accept that existing passwordless Docker users get hard-stopped on upgrade (needs a prominent migration note).
3. Default-on only when a public bind is actually detected AND no reverse-proxy signal — more nuance, more complexity.

This is parked for your call regardless (security PR). Flagging that as-is it would brick passwordless Docker deployments — I'd recommend option 1 (drop the Dockerfile default; keep the guard opt-in) before this ships. Happy to make that one-line Dockerfile change + re-gate once you pick a direction.

[nesquena]

Review — changes requested 🔶 (the security design is right; the container rollout breaks CI and the default deployment)

Independent review of the salvage of #3758 (@fantasticsquirrel): refuse to start (fail-closed sys.exit(1)) when the WebUI would bind to a public/non-loopback address with no auth. I strongly support the intent — silently exposing a passwordless WebUI (filesystem + agent + memory) to the network is exactly the footgun worth closing, and the implementation is clean. But as it stands it can't merge: all three Docker smoke jobs are red, and for a real reason that also affects the default deployment.

What this ships

api/bind_guard.py (+91, new — predicate + refusal message), server.py (+ wiring/sys.exit(1)), Dockerfile (sets HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1), tests/test_security_review_fixes.py (+9 cases), CHANGELOG.md. Agent-authored (nesquena-hermes). Currently CONFLICTING (CHANGELOG) and CI-red on Smoke single / two-container / three-container.

The guard logic is correct

_public_bind_requires_auth(host, within_container, auth_enabled) (api/bind_guard.py:21): auth configured → never block; loopback → never block; explicit …REQUIRE_AUTH_FOR_PUBLIC_BIND off/on honored; else default to within_container. Clean precedence, fails closed only for the genuinely dangerous case. The refusal message is genuinely excellent — states what/why + three concrete recovery paths. The 9 unit cases pass (15/15 in the bind-subset locally). No correctness issue with the predicate itself. ✓

The blocker — the guard refuses the smoke container, and that mirrors the default deployment

The smoke log is unambiguous:

container hermes-smoke-single-…-hermes-webui-1 is unhealthy
hermes-webui-1 |   REFUSING TO START: passwordless server would be exposed on a public address.
hermes-webui-1 |     Hermes WebUI was about to bind to host '0.0.0.0' with NO password
hermes-webui-1 | !! ERROR: hermes-webui failed or exited with an error

The Dockerfile sets HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1, and all three smoke compose files run passwordless on 0.0.0.0 (HERMES_WEBUI_PASSWORD is commented out in docker-compose.yml:46, docker-compose.two-container.yml:121, docker-compose.three-container.yml:142; each sets HERMES_WEBUI_HOST=0.0.0.0). So the guard does exactly what it's designed to do and the containers refuse to boot → all three smoke jobs fail.

This isn't a flaky CI or a test that needs a trivial tweak — those compose files are the templates users copy, so the same refusal hits:

1. the default docker compose up out-of-box experience, and
2. every existing passwordless container deployment on upgrade — their container will stop booting.

That's a deliberate, defensible security posture, but it's a breaking change to the default container behavior, and right now nothing in the rollout accounts for it.

What I'd like resolved before approving

1. Get the smoke CI green by reconciling the compose templates with the new default. Whichever way the posture goes, the three smoke compose files (and any other passwordless 0.0.0.0 deployment templates) need to either set HERMES_WEBUI_PASSWORD, set HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=0 with an explanatory comment, or bind 127.0.0.1. A red gate can't merge.
2. Decide + document the breaking-change rollout (maintainer call): is breaking passwordless-container-on-upgrade acceptable for the security win (I'd lean yes, fail-closed is the right default), and if so —
   • flag it as a BREAKING change prominently in the CHANGELOG (the current entry reads as a feature, not a breaking default change), and
   • consider whether the shipped docker-compose*.yml templates should now ship with a password placeholder that's required, a generated password, or a 127.0.0.1 default, so docker compose up works out of the box again.
   • optionally, a one-release warn-then-enforce migration to soften the upgrade break.
3. Rebase onto master (only CHANGELOG conflicts, same ### Added/Fixed collision as the rest of the batch).

I deliberately did not push a CI-greening patch myself: making smoke pass by editing the compose/workflow would mask the user-facing default-deployment break without the maintainer explicitly accepting that posture — and choosing between "ship a password," "default to localhost," or "warn-then-enforce" is a product/security decision that's yours, not mine.

Tests

• tests/test_security_review_fixes.py bind-guard cases — 15/15 locally (predicate correctness: container-public-passwordless blocks; loopback / auth-enabled / bare-metal-dev exempt; explicit flag on/off; message clarity + dynamic host). The unit coverage is solid.
• CI: Smoke single / two-container / three-container all FAIL (the guard refusing the passwordless smoke containers — see above). Lint + unit shards + browser-smoke pass.

Recommendation

🔶 Request changes — not on the guard (the predicate, message, and unit tests are right and I want this protection in), but on the rollout: the three smoke jobs are red because the new container default refuses the passwordless compose templates, which also breaks the default docker compose up and existing passwordless deployments on upgrade. Please green the smoke CI by reconciling the compose templates with the new default, flag the breaking change in the CHANGELOG, and (maintainer call) decide whether to soften the upgrade path. Happy to re-review immediately once CI is green — this is close, and the security win is worth landing.