fix: harden WebUI security-sensitive defaults

[fantasticsquirrel]

Thinking Path

A security review found several high-risk default/path trust issues around first-run onboarding, Docker exposure, update checks, and media file serving. This PR keeps the changes scoped to those reviewed surfaces and adds regression coverage for the new security contracts.

What Changed

• Ignore X-Forwarded-For / X-Real-IP for unauthenticated onboarding local-network checks unless HERMES_WEBUI_TRUST_FORWARDED_FOR=1 is explicitly set.
• Add Docker/public-bind guard: Docker images set HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1, and startup refuses non-loopback passwordless binds unless explicitly disabled.
• Expand Docker init log masking to include password/secret/credential/cookie/session-like environment variable names.
• Make GET /api/updates/check cache-only and move git/network refreshes to POST /api/updates/check.
• Remove all-of-/tmp from default /api/media allowed roots; temporary files now require session MEDIA: grants or explicit MEDIA_ALLOWED_ROOTS.
• Update browser callers, tests, and changelog for the changed contracts.

Why It Matters

• Prevents spoofed forwarded headers from bypassing local-only unauthenticated onboarding gates.
• Prevents the Docker default 0.0.0.0 bind from exposing a passwordless WebUI by default.
• Reduces accidental credential disclosure in init logs.
• Keeps GET update status read-only/cache-only instead of causing git fetch side effects.
• Reduces arbitrary local file disclosure risk through broad /tmp media reads.

Verification

• pytest tests/test_security_review_fixes.py tests/test_update_checker.py tests/test_update_check_ui.py tests/test_api_timeout.py -q
• pytest tests/test_media_inline.py tests/test_security_review_fixes.py tests/test_update_checker.py tests/test_update_check_ui.py tests/test_api_timeout.py tests/test_v050260_docker_invariants.py tests/test_issue2453_agent_source_boundary.py -q
• python3 -m py_compile api/routes.py api/updates.py server.py

Risks / Follow-ups

• Bare /tmp media URLs are now denied unless represented by a session media grant or explicit MEDIA_ALLOWED_ROOTS. This is intentionally stricter and may require operators with custom scratch directories to configure an explicit allow-list.
• This PR does not attempt the larger supply-chain pinning work for installer scripts, base image digests, actions SHAs, or dependency lockfiles; that should be handled separately to avoid mixing deployment policy changes with runtime security fixes.

Contract Routing

Task type: security hardening
Touched areas: onboarding gates, Docker startup defaults/logging, update checks, media serving
Relevant public docs:

• AGENTS.md
• CONTRIBUTING.md
• docs/CONTRACTS.md
• CHANGELOG.md
  Scope boundaries: no UI layout changes, no installer pinning, no full dependency policy rewrite
  Evidence needed before claiming done: targeted regression tests plus Python compile checks

Model Used

AI-assisted: OpenAI gpt-5.5 via Hermes Agent/Codex tool workflow.

[greptile-apps]

Greptile Summary

This PR hardens several security-sensitive defaults across the WebUI: onboarding local-only gates now ignore spoofed forwarded headers by default, Docker startup refuses public passwordless binds, init-log masking is extended to cover secret-like env var names, GET /api/updates/check is made cache-only with network/git refreshes moved to POST /api/updates/check, and /tmp is removed from the default /api/media allowed roots.

• Forwarded-header trust is now opt-in (HERMES_WEBUI_TRUST_FORWARDED_FOR=1) and uses the rightmost X-Forwarded-For entry, eliminating the spoofed-loopback bypass in the original onboarding gates.
• The GET /api/updates/check → cached_update_status() / POST /api/updates/check → check_for_updates() split is clean and consistent with browser callers updated in boot.js and panels.js.
• Removing /tmp from default media roots is a meaningful hardening step; temporary artifacts now require an exact session MEDIA: grant (images only) or explicit MEDIA_ALLOWED_ROOTS.

Confidence Score: 5/5

Safe to merge; the security contracts are correctly implemented and all browser callers were updated to match the new GET/POST split.

The forwarded-header fix, Docker bind guard, log masking expansion, GET→cache-only routing, and /tmp removal are all correct. No functional regressions were introduced in the routes tested. The two observations are confined to a stale docstring and a test fixture that uses an unreachable initial state — neither affects runtime behaviour.

The _handle_media docstring in api/routes.py still advertises /tmp as an allowed root; and the test_cached_update_status_does_not_drop_agent_info_when_reenabled fixture in tests/test_security_review_fixes.py uses a state that normal code never produces.

Important Files Changed

Filename	Overview
api/routes.py	Adds _onboarding_gate_allows / _onboarding_request_is_local helpers replacing duplicated inline checks; moves update check to cached_update_status on GET; adds POST handler for force-refresh; removes /tmp from media allowed roots. Docstring for _handle_media still references /tmp as an allowed root after removal.
api/updates.py	Adds cached_update_status() for cache-only GET reads. Logic correctly suppresses agent info when include_agent=False and preserves existing cached agent data when re-enabling; but test fixture for the re-enable case uses a state unreachable by normal code flow.
server.py	Adds _public_bind_requires_auth() helper and wires it to sys.exit(1) on public passwordless bind in Docker. Logic is correct: loopback hosts bypass, env var opt-out honored, within_container is the final fallback.
tests/test_security_review_fixes.py	New regression suite covering all PR contracts. The test_cached_update_status_does_not_drop_agent_info_when_reenabled fixture is unrealistic — it stores real agent update data (behind: 2) under an include_agent: False cache, a state that check_for_updates(include_agent=False) never produces.
docker_init.bash	Extends ENV_OBFUSCATE_PART to mask PASSWORD, SECRET, CREDENTIAL, COOKIE, and SESSION variable names in addition to the existing TOKEN, API, KEY.
Dockerfile	Sets HERMES_WEBUI_REQUIRE_AUTH_FOR_PUBLIC_BIND=1 so the new startup guard in server.py fires for all Docker images by default.
static/boot.js	Switches on-boot update check from GET to POST with {force:false}; test-mode simulation path keeps GET with ?simulate=1.
static/panels.js	Switches the manual Check for updates button from GET ?force=1 to POST {force: true}, matching the new endpoint contract.
tests/test_media_inline.py	Updated to expect 403 for bare /tmp media requests (PNG, audio, HTML); test_nonexistent_file_returns_404 now accepts 403 as well since the path check fires before the existence check.

Sequence Diagram

sequenceDiagram
    participant Browser
    participant GET as GET /api/updates/check
    participant POST as POST /api/updates/check
    participant Cache as cached_update_status()
    participant Full as check_for_updates()
    participant Git as git fetch

    Note over Browser,Git: Normal page boot (boot.js)
    Browser->>POST: "POST {force: false}"
    POST->>Full: "force=False"
    alt cache fresh (TTL not expired)
        Full-->>POST: cached dict
    else cache stale
        Full->>Git: git fetch origin --tags
        Git-->>Full: tag refs
        Full-->>POST: updated dict
    end
    POST-->>Browser: "{webui, agent, checked_at}"

    Note over Browser,Git: Subsequent polls (boot.js sessionStorage guard)
    Browser->>GET: GET /api/updates/check
    GET->>Cache: "include_agent=True/False"
    Cache-->>GET: "shallow copy + cached=True"
    GET-->>Browser: "{webui, agent, cached: true}"

    Note over Browser,Git: Manual Check for updates button (panels.js)
    Browser->>POST: "POST {force: true}"
    POST->>Full: "force=True"
    Full->>Git: git fetch origin --tags (forced)
    Git-->>Full: tag refs
    Full-->>POST: updated dict
    POST-->>Browser: "{webui, agent, checked_at}"

Loading

_{Reviews (2): Last reviewed commit: "fix: address security review follow-ups" | Re-trigger Greptile}

[fantasticsquirrel]

Addressed both Greptile findings in follow-up commit 54c69a4.

Changes:

• Trusted X-Forwarded-For mode now reads the rightmost comma-separated entry, matching proxy-appended client IP behavior and preventing client-supplied leftmost spoofing.
• cached_update_status(include_agent=True) no longer overwrites an existing cached agent payload with None when the cached include_agent flag differs; it only replaces agent status with the ignored payload when include_agent=False is requested.

Regression tests added:

• test_onboarding_trusted_forwarded_for_uses_proxy_appended_rightmost_ip
• test_cached_update_status_does_not_drop_agent_info_when_reenabled

Verification:

• pytest tests/test_security_review_fixes.py -q → 9 passed
• pytest tests/test_media_inline.py tests/test_update_checker.py tests/test_update_check_ui.py tests/test_api_timeout.py tests/test_v050260_docker_invariants.py tests/test_issue2453_agent_source_boundary.py -q → 96 passed
• python3 -m py_compile api/routes.py api/updates.py server.py
• git diff --check

[nesquena-hermes]

Pulled the branch (54c69a45) into a read-only worktree and read the full diff across server.py, api/routes.py, api/updates.py, Dockerfile, docker_init.bash, and the three test files. Four independent hardening changes, all scoped tightly and each backed by a regression test. This is a clean PR — a few confirmations and one design note on the forwarded-header parsing.

What I verified

1. Onboarding gate refactor is the real win. The three onboarding POST handlers (/api/onboarding/oauth/start, /setup, /probe) previously inlined the same IP check using X-Forwarded-For.split(",")[0] — the leftmost value, which a direct client can spoof (X-Forwarded-For: 127.0.0.1, <attacker>). The new _onboarding_request_is_local (routes.py:1573) ignores forwarded headers entirely unless HERMES_WEBUI_TRUST_FORWARDED_FOR=1, and reads .split(",")[-1] when it does:

candidates = []
if _truthy_env("HERMES_WEBUI_TRUST_FORWARDED_FOR"):
    candidates.extend([
        handler.headers.get("X-Forwarded-For", "").split(",")[-1].strip(),
        handler.headers.get("X-Real-IP", "").strip(),
    ])
candidates.append(_request_client_ip(handler))

The loop returns on the first parseable candidate and falls through to return False (fail-closed deny) when nothing parses — correct for a security gate. Consolidating three copies into one helper is also a maintainability win.

2. GET/POST update-check split is correctly CSRF-protected. GET /api/updates/check now calls the new cached_update_status() (updates.py:689) which reads _update_cache under the lock with no git/network side effects, and the git-fetching check_for_updates(force=...) moved to a new POST branch at routes.py:6535. That branch sits after the global CSRF gate at routes.py:6493 (and /api/updates/check is not in _csrf_exempt_path), so the network-triggering path requires a valid token. Both static callers were updated — boot.js:1844 POSTs {force:false} for the banner check, panels.js:7574 POSTs {force:true} for the manual "check now". test_get_update_check_returns_cache_without_fetch asserts the GET path throws if it ever calls check_for_updates, which pins the contract. Cold-cache is safe: _update_cache initializes to {'webui': None, 'agent': None, ...} (updates.py:35) and the frontend guards with d.webui && d.webui.behind.

3. Public-bind guard. _public_bind_requires_auth (server.py:525) returns False for loopback/localhost and for auth-enabled, honors an explicit on/off env flag, and otherwise defaults to within_container. The Dockerfile sets the flag to enable-by-default, so a passwordless 0.0.0.0 container bind now sys.exit(1)s instead of just warning. within_container is detected via the /.within_container marker (server.py:602), so bare-metal dev hosts keep the old warn-only behavior — good, this won't surprise local users.

4. /tmp media root removal. allowed_roots (routes.py:9715) drops Path("/tmp"), leaving HERMES_HOME, ~/.hermes, and the active workspace, plus opt-in MEDIA_ALLOWED_ROOTS. Session MEDIA: token grants still work via _session_media_token_allows_image_path, so legitimate chat artifacts keep rendering. The test rewrites in test_media_inline.py correctly flip the bare-/tmp cases from 200 to 403 and relax the nonexistent-file case to {403, 404}.

One design note (non-blocking)

The .split(",")[-1] choice in _onboarding_request_is_local assumes a single trusted proxy. With exactly one proxy that's the right call — the proxy appends the real client IP it observed, and [-1] can't be spoofed by the client. But in a multi-proxy chain (client, proxy1, proxy2), the rightmost entry is an intermediate hop, not the original client, so the gate could misjudge locality. Given this is gated behind an explicit opt-in and is strictly safer than the old spoofable [0], I'd just document the single-trusted-proxy assumption next to HERMES_WEBUI_TRUST_FORWARDED_FOR rather than block on it. Operators with deeper chains are the ones who'd need a trusted-hop-count knob, and that can be a follow-up.

Test plan

Per the execution ban I didn't run anything from the worktree. tests/test_security_review_fixes.py covers the public-bind helper (container blocks, loopback allows, auth disables), the GET-no-fetch / POST-forces contract, and the cached_update_status agent-info-preservation edge. Recommend the maintainer run pytest tests/test_security_review_fixes.py tests/test_media_inline.py tests/test_update_checker.py tests/test_update_check_ui.py tests/test_v050260_docker_invariants.py -q plus a node --check on the two touched JS files before merge.

[nesquena-hermes]

Thanks for this — it was a genuinely clean, well-tested security PR, and it's having real impact even though we're closing the PR itself (it went stale + conflicting after the review).

Status of the four hardenings:

• ✅ Onboarding X-Forwarded-For spoof fix (ignore forwarded headers unless HERMES_WEBUI_TRUST_FORWARDED_FOR, read rightmost entry) — shipped to master.
• ✅ GET/POST update-check split (cached_update_status for the no-fetch GET; CSRF-gated POST for the git-fetching path) — shipped to master.
• ✅ test_security_review_fixes.py — in master.
• 🔜 Public-bind-requires-auth guard — we're bringing this in now via a fresh branch, with an expanded, very explicit startup message (what happened + exactly how to fix: set a password / bind localhost / explicit opt-out if another layer enforces auth), so an existing passwordless-public deployment that gets stopped on upgrade understands immediately. Credit to you preserved.
• ⏸️ /tmp media-root removal — we're holding this one: shared screenshots render via the per-session MEDIA: token grant (independent of allowed_roots), so removing /tmp is sound hardening, but we want to verify no legit image flow relies on the blanket /tmp allow before changing it.

Closing the PR as superseded/absorbed — three parts already in master, the public-bind guard landing now with credit, and the /tmp change deferred for verification. Really appreciate the careful work and the regression tests; they made all of this easy to act on.