fix(security): keep model caches owner-only

[mudrii]

Summary

• create OpenRouter metadata and model-catalog caches with owner-only 0600 permissions
• route model-catalog writes through the shared atomic JSON writer
• verify permissions on POSIX while retaining the existing content assertions on Windows

Why

These caches contain locally selected/provider-derived model metadata and previously inherited the process umask. Explicit owner-only modes align them with Hermes' other private state files.

Verification

• scripts/run_tests.sh tests/agent/test_model_metadata.py tests/hermes_cli/test_model_catalog.py
• 150 passed, 0 failed
• ruff check passed
• git diff --check passed

[Copilot]

Pull request overview

This PR hardens Hermes’ local on-disk caches for model metadata and the remote model-catalog manifest by ensuring they are written with owner-only permissions (0600) instead of inheriting a potentially permissive process umask. This aligns these caches with other private Hermes state files and reduces the risk of local information disclosure.

Changes:

• Write the CLI model-catalog cache via the shared atomic_json_write(...) helper and explicitly set mode=0o600.
• Ensure the OpenRouter model metadata disk cache is saved with mode=0o600.
• Extend tests to validate 0600 permissions on POSIX platforms (while keeping content assertions on Windows).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
hermes_cli/model_catalog.py	Routes cache writes through atomic_json_write and forces owner-only permissions.
agent/model_metadata.py	Sets owner-only permissions for the OpenRouter metadata disk cache.
tests/hermes_cli/test_model_catalog.py	Adds a POSIX-only assertion that the model-catalog cache file is written as 0600.
tests/agent/test_model_metadata.py	Adds a POSIX-only assertion that the OpenRouter metadata cache file is written as 0600.

---

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

[teknium1]

Thanks for hardening the two targeted cache writers. The proposed atomic_json_write(..., mode=0o600) mechanism is appropriate: utils.py:175-196 sets the temporary-file mode before replacement on POSIX and reapplies it to the resolved target.

Problems

• The hardening is incomplete for sibling persisted model caches. agent/models_dev.py:235, hermes_cli/models.py:2727, and hermes_cli/models.py:4011 use atomic_json_write without mode=0o600; hermes_cli/models.py:861-884 writes the Nous recommended-model cache through a default-mode temporary file. Those paths remain umask-dependent.

Suggested changes

• Apply the same owner-only mode to those writers, convert the Nous cache write to the shared atomic JSON helper, and add POSIX permission assertions for each persisted model-cache path.

This is an automated hermes-sweeper review.

[teknium1]

Please extend this owner-only write policy to the sibling persisted model caches: agent/models_dev.py:235, hermes_cli/models.py:2727, hermes_cli/models.py:4011, and the direct temporary-file write at hermes_cli/models.py:861-884 remain umask-dependent.

[GottZ]

This was generated by AI during triage.

Summary

Ten PRs were checked across the model-catalog cluster. #54737, #54793, and #54840 are the recorded best fixes for bounded provider/generic, manifest, and broad live-catalog reads respectively; #64202 instead hardens two persisted cache writers, while the other PRs address overlapping provider reads or separate validation, configuration, discovery, and picker behavior.

Related pull requests

• fix: validate model catalog schema version bounds #30853 related — (+25/-1) — n/a: Rejects pre-v1 and boolean manifest schema versions in _validate_manifest() but does not change response reads or cache permissions; its keep-open verdict concerns this separate centralized validation fix.
• fix: probe live Nous inference API for models missing from curated pi… #32075 related — (+150/-16) — n/a: Adds a cached live Nous /v1/models probe with a bare resp.read() and broadens the picker beyond its curated-plus-recommendations contract. Its keep-open verdict has low salvageability and asks the author to re-scope around a demonstrated recommendation-flow failure.
• fix(model-catalog): parse bool-ish config values #35469 related — (+63/-3) — n/a: Corrects bool-ish model_catalog.enabled parsing and robustly parses ttl_hours, including zero, invalid, non-finite, and negative values; its keep-open verdict and approval concern that distinct configuration defect.
• fix(cli): bound generic model catalog response reads #54737 best fix — (+166/-5) — n/a: Recorded best fix for Bound provider model catalog response reads #54735 and Provider live model catalog fetch buffers unbounded response bodies #56494: bounds both ProviderProfile.fetch_models() and the generic CLI probe with declared-length checks and capped reads. Consistent with its keep-open verdict, salvage must port the helper and tests to open_credentialed_url and _urlopen_model_catalog_request while preserving redirect security.
• fix(providers): bound model-catalog response reads (#54735) #54765 [closed] fixes — (+225/-5) — n/a: Bounds ProviderProfile.fetch_models() and the Anthropic override through a shared capped JSON reader; although closed, it remains relevant as a provider-side reference implementation and was closed in favor of fix(providers): bound the /models catalog response size in fetch_models #42930.
• fix(models): bound remote catalog manifest reads #54793 best fix — (+80/-2) — n/a: Recorded best fix for Remote model catalog manifest reads response bodies without a cap #54792: bounds the central _fetch_manifest() read and routes oversized bodies through the existing fallback, with focused normal-read and oversize tests. Its keep-open verdict supports retaining this manifest-specific salvage path.
• Bound model catalog response reads #54840 best fix — (+84/-18) — n/a: Recorded best fix for Bound live model catalog response reads #54838: routes the listed hermes_cli/models.py catalog, pricing, probe, and LM Studio reads through bounded helpers. The contributor keep-open review requires porting the diff to _urlopen_model_catalog_request, sharing the helper with model_catalog.py, and adding oversized-manifest fallback coverage.
• fix(providers): bound live model catalog reads #56495 [closed] fixes — (+72/-2) — n/a: Bounds ProviderProfile.fetch_models() with an 8 MiB cap and focused regression tests; although closed, it remains relevant as direct evidence for Provider live model catalog fetch buffers unbounded response bodies #56494 and was explicitly closed as a duplicate of the broader fix(cli): bound generic model catalog response reads #54737.
• fix(model): make picker discovery config-aware #60656 related — (+117/-13) — n/a: Makes picker provider discovery configuration-aware, filters env-only built-ins, preserves stored credential pools, and propagates empty provider context; it does not modify response bounds or cache modes.
• fix(security): keep model caches owner-only #64202 related — (+10/-7) — n/a: Applies owner-only 0600 writes to the OpenRouter metadata and model-catalog caches and adds POSIX permission assertions. The contributor keep-open review supports the mechanism but blocks the current scope pending equivalent hardening for agent/models_dev.py:235, hermes_cli/models.py:2727, hermes_cli/models.py:4011, and the Nous cache writer at hermes_cli/models.py:861-884.

Duplicates

Closed #54765 and #56495 substantially duplicate the ProviderProfile.fetch_models() portion of #54737; #56495 was explicitly closed as a duplicate of #54737, while #54765 was closed in favor of #42930. #54737 and #54840 overlap at the generic probe but are not full duplicates, and #54793 covers the complementary manifest path.

Suggested consolidation

Keep #64202 open with a salvage path: preserve its two owner-only atomic cache writes, then address the contributor keep-open review by extending 0600 handling and POSIX permission tests to the four named sibling cache writers. Keep recorded best-fix PRs #54737, #54793, and #54840 open on their separate conflict-aware salvage paths; #54765 and #56495 are already-closed provider-side duplicates and should remain closed.

Cross-PR triage: Reviewed 10 pull requests and 4 issues in this complex. Each diff was read against this issue; Assessment working set: 74 kB of PR diffs, 26 kB of issue/PR text, 22 kB of discussion (28 comments), 16 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[egilewski]

suggesting changes

The cache hardening is incomplete: fresh legacy cache files are accepted without an owner-only permission check. A pre-existing broad-permission catalog or OpenRouter metadata cache can remain readable and replaceable through fresh, stale/offline, or failed-refresh paths, allowing local peers to influence model metadata and selection.

• [P3] Fresh legacy cache reads bypass the owner-only invariant
  Both readers return valid legacy files before repairing or verifying permissions. Ensure every disk-cache read enforces owner-only access (or fails closed), and cover fresh, stale/offline, and failed-refresh cases with equivalent tests.

Security evidence:

• trust boundary: Remote catalog/metadata and on-disk caches are untrusted; local-user isolation requires owner-only cache access.
• source/sink/invariant: Successful writes pass mode=0o600, but fresh-read branches return existing files without a permission check or repair.
• current-main reproduction: The pre-fix writers preserved default broad permissions, so an existing cache could remain group/world-readable.
• PR-head or patch-replay validation: Refresh writes now produce 0600 on POSIX, while fresh legacy reads still leave broad modes unchanged.
• positive/negative cases: New or refreshed files are owner-only; valid pre-existing broad-permission catalog and metadata files are still served unchanged.
• residual bypass search: Both cache readers retain the same early-return bypass; unrelated cache files are outside this finding.
• reviewer validation: Source inspection and focused tests confirmed the write hardening and the fresh-read bypass.

Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub main because the submitted branch is stale or conflicted; this does not mean the submitted branch itself merges cleanly.

Not checked:

• Windows ACL behavior
• CodeRabbit review

Signed: GPT-5.6-luna-max in Codex

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(security): keep model caches owner-only

1. The permission tightening only applies at rewrite time: cache files already on disk with looser perms (created by previous versions under a permissive umask, e.g. 0o644) stay loose until the next successful refresh. Consider a chmod-on-load pass (or a startup sweep) so pre-existing caches are remediated immediately after upgrade.
2. Windows: mode=0o600 is effectively a no-op (no fchmod, and the tests correctly skip the assertion on nt). mkstemp restricts the temp file, but the final file's ACL on Windows is not governed by this change — the hardening is POSIX-only. Worth documenting that boundary so the security claim is accurate per platform.
3. Minor: the two cache writers now both use atomic_json_write(..., mode=0o600) (agent/model_metadata.py and hermes_cli/model_catalog.py) — a shared helper would keep the two paths in lockstep (format, mode, atomicity) if either is touched later.