fix(mcp): restrict catalog environment writes

[unsupportedpastels]

Summary

• restrict MCP catalog credential submissions to environment variables declared by the selected catalog entry
• block Hermes runtime and approval-control variables at the shared environment writer
• prevent generic persistence from replacing the MCP catalog trust root
• prevent generic persistence from acquiring Copilot ACP executable/argv authority
• apply denylist and .env key matching with native Windows case semantics while preserving POSIX behavior
• validate the complete catalog environment map before writing any values
• add endpoint-level regression coverage for valid credentials, undeclared keys, reserved controls, and secret-safe errors

Security

POST /api/mcp/catalog/install previously accepted an arbitrary env mapping and persisted each non-empty value to the selected profile's .env file before installation. A request could therefore attach a Hermes runtime control such as HERMES_YOLO_MODE to an otherwise valid MCP credential submission.

The route now treats each catalog entry's auth.env list as a closed schema. Any undeclared name returns HTTP 400 before the first write or install action. The complete submitted name set is also checked against the shared writer denylist, so a malformed catalog entry cannot authorize itself to persist approval or session controls.

Errors report rejected names only; submitted values are not included.

Compatibility

The Desktop already renders and submits credential fields from the selected catalog entry's auth.env declaration, so valid catalog installs retain their current request shape. Existing manually configured MCP servers are unchanged.

The denylist remains name-specific rather than blocking all HERMES_* variables. Integration credentials and settings such as HERMES_LANGFUSE_PUBLIC_KEY, HERMES_SPOTIFY_CLIENT_ID, HERMES_QWEN_BASE_URL, and HERMES_MAX_ITERATIONS remain writable. Dedicated CLI, config, and session controls continue to manage approval behavior.

HERMES_OPTIONAL_MCPS is blocked only through generic persistence writes. Existing .env values and package-manager/process-supplied catalog roots continue to resolve. Windows comparisons are case-insensitive, matching the host environment model; POSIX comparisons remain case-sensitive.

HERMES_COPILOT_ACP_COMMAND and HERMES_COPILOT_ACP_ARGS follow the same write-only restriction. Existing operator or package-manager supplied values remain readable by the ACP client; rejected /api/env writes neither persist nor change the live command/argv resolvers.

Test plan

• scripts/run_tests.sh tests/hermes_cli/test_config.py tests/hermes_cli/test_mcp_catalog.py tests/hermes_cli/test_mcp_catalog_env_boundary.py tests/hermes_cli/test_credential_lifecycle.py tests/hermes_cli/test_web_server_profile_unification.py tests/agent/test_copilot_acp_client.py tests/agent/test_copilot_acp_deprecation.py -q — 188 passed, 4 Windows-only cases skipped locally
• scripts/run_tests.sh tests/hermes_cli/test_mcp_catalog_env_boundary.py -q — 10 passed
• Ruff on all changed Python files
• git diff --check

Verification note

A broader scripts/run_tests.sh tests/hermes_cli/ -q run reached unrelated update/doctor tests that detected the developer machine's live gateway and triggered the suite's live-system guard. The guard blocked attempts to signal those external gateway PIDs, producing 13 failures in update/doctor modules. None of the affected MCP/config/profile modules failed; those modules were rerun separately against the final diff as listed above.

Closes #91565.

[andrexibiza]

Blocking review findings at exact head 859a0995e4836dfad66934ba30c9d46777d16a27. The catalog-key closed schema is directionally correct and exact-head CI is green, but the shared writer boundary remains bypassable in two independent ways: omitted execution-control/trust-root variables and case-insensitive Windows aliases. Both preserve arbitrary command or bootstrap execution through the same authenticated environment-writing surface, so this does not yet close the vulnerability class.

[andrexibiza]

[P1] Block the complete execution-control class, not selected names. HERMES_OPTIONAL_MCPS is still writable here; mcp_catalog._catalog_root() trusts it as the catalog root, and a manifest from that directory can supply install.bootstrap, which _run_bootstrap() executes with shell=True. The shared writer also still accepts HERMES_COPILOT_ACP_COMMAND / HERMES_COPILOT_ACP_ARGS, which agent/copilot_acp_client.py uses to construct a subprocess invocation. Because save_env_value() updates os.environ immediately, these are live in the current dashboard process as well as future children. Define one authoritative non-writable set covering catalog/skill trust roots and the behavioral keys already identified by _PROFILE_MANAGED_ENV_KEYS, then add public /api/env regressions. Otherwise the new catalog schema blocks one route while equivalent execution paths remain open.

[unsupportedpastels]

Thanks — I agree with the MCP-specific trust-root gap. Commit 6818ea0e4f adds HERMES_OPTIONAL_MCPS to the shared writer denylist, so authenticated /api/env writes can no longer replace the catalog root in either the current process or a later restart. The restriction is deliberately write-only: pre-existing .env values still load, and package-manager/process-supplied HERMES_OPTIONAL_MCPS still resolves. The public endpoint and both compatibility paths now have regression coverage (test_generic_env_endpoint_rejects_protected_key, test_preexisting_optional_mcps_override_still_loads, and test_process_supplied_catalog_root_remains_supported).

I did not fold all of _PROFILE_MANAGED_ENV_KEYS into this MCP PR. That set includes documented Copilot ACP command/path/base-URL overrides; blocking all of them through the generic writer would change an unrelated provider configuration surface and widen the blast radius for MCP users. The catalog route is closed against undeclared keys, and this follow-up closes its alternate MCP trust-root path. The Copilot/shared-writer execution-control class should be handled separately with its own compatibility analysis rather than being bundled into this MCP fix.

[andrexibiza]

Re-review at exact head 6818ea0e4fb1820916a1f76bbc018cdb521cfadc: the MCP trust-root half is fixed, but this P1 is only partially closed. HERMES_OPTIONAL_MCPS is now protected and the Windows-name bypass is handled, but the same shared writer still accepts HERMES_COPILOT_ACP_COMMAND / HERMES_COPILOT_ACP_ARGS; agent/copilot_acp_client.py reads those values directly to choose the executable/argv for a subprocess. Because save_env_value() also updates the live process environment, the generic authenticated env-writing surface still has an execution-control path.

If that class is intentionally split out of this MCP patch, it needs an explicit interlock to a concrete follow-up rather than treating the original finding as closed. I am leaving this thread unresolved until either the shared boundary is closed here or that follow-up exists and this PR is scoped/proven as an MCP-only slice.

[andrexibiza]

Resolved on exact head 6b94252f4226bed24f0a3152b5165e4f9486ec66. The PR now absorbs #91565 directly: HERMES_COPILOT_ACP_COMMAND and HERMES_COPILOT_ACP_ARGS are blocked by the shared writer, Windows aliases are covered, rejected public /api/env writes cannot change the live ACP command/argv resolvers, and pre-existing operator configuration remains readable. The original P1 execution-control finding is closed on this head.

[andrexibiza]

[P1] Canonicalize the name before the policy lookup. _reject_denylisted_env_var() uses exact-string membership, but Windows environment names are case-insensitive: Path or Hermes_Yolo_Mode pass this validation and then os.environ[key] = value aliases them to PATH / HERMES_YOLO_MODE in the current process and child processes. Normalize before denylist membership (and before matching/updating existing .env assignments), or reject case-insensitive collisions, with mixed-case Windows-semantics regression coverage.

[unsupportedpastels]

Agreed. Commit 6818ea0e4f now canonicalizes names for policy lookup with native host semantics: uppercase on Windows, identity on POSIX. The same rule is applied when matching existing .env assignments, so Windows cannot create case-variant duplicates such as Path beside PATH, while Linux/macOS retain case-sensitive behavior.

Coverage includes pure Windows/POSIX policy tests without faking the host OS, export and plain assignment matching, and a native @pytest.mark.windows_only writer regression for Hermes_Yolo_Mode. The focused affected suite passes 154 tests locally (the one native Windows test is running on the Windows CI lane), plus Ruff and git diff --check.

[andrexibiza]

Resolved on exact head 6b94252f4226bed24f0a3152b5165e4f9486ec66. Policy lookup and existing .env assignment matching use Windows-case-insensitive semantics while POSIX remains case-sensitive; mixed-case aliases now cover the MCP trust root, security controls, and Copilot ACP execution controls. This P1 finding is closed.

[andrexibiza]

Topology closure for the residual execution-control class: #91565 now explicitly owns the generic-writer → ACP subprocess executable/argv boundary (HERMES_COPILOT_ACP_COMMAND / _ARGS) with public /api/env, live-os.environ, Windows-case, and compatibility acceptance witnesses.

That makes this PR's scope concrete: #91139 owns the MCP catalog trust-root/schema boundary and Windows env-name semantics; #91565 owns the provider-wide subprocess-control class. The remaining ACP work is no longer an implicit/unowned exception in this PR.

[andrexibiza]

Topology follow-up at exact head 6818ea0e4fb1820916a1f76bbc018cdb521cfadc: the condition I left on the remaining execution-control finding now has a concrete owner in #91565 (security: remove ACP subprocess controls from generic environment persistence). That issue explicitly owns HERMES_COPILOT_ACP_COMMAND / _ARGS, the live os.environ mutation path, Windows aliases, compatibility for legitimate ACP configuration, and public /api/env witnesses.

With that interlock in place, I no longer treat the Copilot/shared-writer class as an unowned blocker inside this MCP-scoped PR. The MCP-specific trust-root path and Windows case-normalization finding remain substantively fixed on this head; #91565 must remain open until the provider-wide execution-control boundary is actually closed.

I attempted to mark the now-satisfied formal thread resolved, but the current review-thread mutation identity cannot perform that bookkeeping operation. That does not change the source disposition above.

[andrexibiza]

I found one residual in the same generic env-writer authority class while reviewing this head. Please absorb it here rather than creating a second implementation PR.

agent/copilot_acp_client.py currently consumes HERMES_COPILOT_ACP_COMMAND as the executable and HERMES_COPILOT_ACP_ARGS as argv. The authenticated dashboard PUT /api/env path reaches the shared writer through save_provider_env_credential(), so either name can currently cross generic persistence into live os.environ and become subprocess execution authority.

This PR already owns the correct choke point and Windows case semantics. The narrow completion is therefore:

• add HERMES_COPILOT_ACP_COMMAND and HERMES_COPILOT_ACP_ARGS to the shared generic-writer denylist;
• reject Windows mixed-case aliases through the existing _env_var_policy_name() path;
• preserve existing operator/package-manager supplied values as readable/usable compatibility state — this is a write-policy change, not an env-loader migration;
• add a public /api/env regression proving a rejected write neither persists nor changes the executable/argv subsequently resolved by the ACP client;
• keep the MCP catalog closed-schema work and this ACP execution-control residual as one writer-boundary object rather than duplicating the policy in the ACP client.

Topology owner for the residual is #91565. If this is absorbed here with exact-head proof, #91565 can close against this PR instead of opening another delivery surface.

[unsupportedpastels]

@andrexibiza Absorbed into this PR at exact head 6b94252f42.

The shared generic-writer denylist now includes exactly HERMES_COPILOT_ACP_COMMAND and HERMES_COPILOT_ACP_ARGS; no policy was duplicated in agent/copilot_acp_client.py. Because the existing _env_var_policy_name() path owns the lookup, Windows mixed-case aliases are rejected as well.

The public /api/env regression starts from a trusted live command/argv, attempts attacker-controlled writes for both names, asserts HTTP 400, verifies neither value reaches .env, and then re-reads _resolve_command() / _resolve_args() to prove live execution authority did not change. A separate compatibility regression writes pre-existing operator values to the profile .env, loads them through load_hermes_dotenv(), and proves the ACP resolvers still consume them unchanged. This keeps the change write-policy-only and preserves operator/package-manager supplied state.

Local exact-head verification: 188 affected tests passed across config, MCP/profile/credential lifecycle, and both Copilot ACP modules; the 10 endpoint boundary cases pass; Ruff and git diff --check pass. The PR body now includes Closes #91565 so the topology owner can close against this delivery surface once merged. CI is running on the new head.

[andrexibiza]

Re-review complete at exact head 6b94252f4226bed24f0a3152b5165e4f9486ec66: no remaining blocker.

The residual P1 execution-control class is now closed in this PR rather than merely interlocked:

• HERMES_COPILOT_ACP_COMMAND and HERMES_COPILOT_ACP_ARGS are rejected by the shared generic writer before .env persistence or live os.environ mutation;
• Windows mixed-case aliases consume the same native case-insensitive policy boundary;
• pre-existing operator/package-manager values remain readable compatibility state;
• the public /api/env witness drives both rejected writes and proves _resolve_command() / _resolve_args() remain unchanged afterward.

The MCP trust-root and Windows-name findings remain closed. Exact-head hosted evidence is green: CI 32496484581, Docker 32496483561, and Nix 32496483680.