Skip to content

fix(gateway): block Hermes home media attachments - #35939

Open
zapabob wants to merge 2 commits into
NousResearch:mainfrom
zapabob:codex/gateway-media-hermes-home-block-20260601
Open

fix(gateway): block Hermes home media attachments#35939
zapabob wants to merge 2 commits into
NousResearch:mainfrom
zapabob:codex/gateway-media-hermes-home-block-20260601

Conversation

@zapabob

@zapabob zapabob commented May 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Treat non-cache files under the active Hermes home and shared Hermes root as denied for native media delivery.
  • Preserve delivery from Hermes-managed cache roots and explicit HERMES_MEDIA_ALLOW_DIRS operator roots.
  • Add regressions for OAuth/MCP/session files under HERMES_HOME, cache allowlist precedence, and operator allowlist precedence.

Why

Default media delivery mode blocked a few named Hermes credential files, but other non-cache Hermes state files such as OAuth refresh tokens, MCP token JSON, and session metadata could still be attached if a model emitted their path. Cache and explicit operator roots are already checked first, so denying the Hermes roots closes that leak without breaking generated artifact delivery.

Related

Tests

  • ruff check gateway/platforms/base.py tests/gateway/test_platform_base.py tests/gateway/test_extract_local_files.py
  • python -X utf8 -m pytest -p no:cacheprovider tests/gateway/test_platform_base.py::TestMediaDeliveryDefaultMode tests/gateway/test_extract_local_files.py -q --timeout-method=thread
  • ruff check .

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to closed #32090 and merged #35634 (both deny entire HERMES_HOME tree in media delivery). Also related to merged #30432 (original MEDIA path delivery fix). Verify this doesn't conflict with already-merged #35634.

@liuhao1024

Copy link
Copy Markdown
Contributor

I verified this change — the blanket denial of the entire HERMES_HOME directory is the correct security posture, and the allowlist override semantics are preserved.

Verification details:

  1. Allowlist beats denylist. In validate_media_delivery_path() (base.py:1052-1060), the cache/operator allowlist check runs before the denylist check. So hermes_dir/cache/documents/report.pdf still delivers correctly even when the entire hermes_dir is denied — confirmed by test_cache_allowlist_beats_hermes_home_denylist.

  2. Blocks the real attack surface. The old denylist missed OAuth tokens (.anthropic_oauth.json), MCP tokens (mcp-tokens/server.json), session databases (sessions.json), and any future credential files added under HERMES_HOME. Blanket denial is defense-in-depth — new files are blocked by default.

  3. USERPROFILE env var patching is correct. On Windows, os.path.expanduser("~") reads USERPROFILE (not HOME). The test patches both to ensure cross-platform coverage.

  4. Operator override preserved. HERMES_MEDIA_ALLOW_DIRS still works for explicit operator allowlists under HERMES_HOME (tested by test_operator_allowlist_beats_hermes_home_denylist).

Note: The existing merged PR #35634 denied specific Hermes root files. This PR supersedes that approach with a blanket denial — cleaner and more future-proof. The PR body should mention this supersedes #35634's per-file approach.

No issues found. Ready to merge.

@egilewski

Copy link
Copy Markdown
Contributor

Looks mergeable from this security review.

Checked current main 6f6eb871d83415fe2980f3483cc41a435ba22196 against PR head 7ba4e041c9d564ea81bd415cd4da13866abff205. I reproduced the default-mode media-delivery gap on current main with a synthetic HERMES_HOME: .hermes/.anthropic_oauth.json, auth/google_oauth.json, mcp-tokens/server.json, and sessions.json all returned deliverable paths. In the PR state, the same paths returned None, while cache delivery and explicit HERMES_MEDIA_ALLOW_DIRS roots under HERMES_HOME still returned resolved paths.

Validation:

  • python -m pytest -o addopts='' -p no:cacheprovider tests/gateway/test_platform_base.py -k 'denylist_blocks_non_cache_hermes_home_files or cache_allowlist_beats_hermes_home_denylist or operator_allowlist_beats_hermes_home_denylist' -q -> 3 passed, 150 deselected
  • git diff --cached --check HEAD -> clean
  • CodeRabbit on the uncommitted PR patch against 6f6eb871d83415fe2980f3483cc41a435ba22196 -> No findings
  • GitHub reports MERGEABLE / CLEAN, with checks successful or expected skipped

Signed: GPT-5.5-xhigh in Codex

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 21, 2026
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists and removed P1 High — major feature broken, no workaround labels Jun 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused media-delivery security review. Current main has moved to a deliberately targeted model rather than a whole-Hermes-home denial.

Problems

  • The added whole-root deny would break the current contract at gateway/platforms/base.py:1163-1165: non-credential skills, logs, and ad-hoc Hermes-home files remain deliverable. tests/gateway/test_platform_base.py:1165-1184 explicitly asserts a fresh ~/.hermes/adhoc_report.pdf is delivered and says it prevents reintroducing the rejected whole-tree deny.
  • The listed OAuth/MCP paths are now protected individually: .anthropic_oauth.json and auth/google_oauth.json are denied at gateway/platforms/base.py:1172-1179; mcp-tokens/ is denied at gateway/platforms/base.py:1185-1202 via 42d0174.

Suggested changes

  • Re-scope to a specifically unprotected sensitive store, preserving the current ad-hoc artifact behavior. Session/SQLite-store denial is already being handled by open PR #41071.

Automated hermes-sweeper review.

Comment thread gateway/platforms/base.py
denied.append(hermes_root / "config.yaml")
denied.append(hermes_root)
return denied

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole-root deny reverses current main's intentional targeted policy. Main explicitly preserves fresh non-credential files under Hermes home (tests/gateway/test_platform_base.py:1165-1184); please re-scope to a concrete sensitive path instead.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Two PRs address the issue's Hermes media-delivery path protection. #35634 adds shared-root blocking for named configuration and credential files, while #35939 broadens the deny rule to non-cache files under the active Hermes home and shared root to cover additional token and session state.

Related pull requests

Duplicates

#35634 and #35939 overlap in Hermes-root media-delivery protection, but they are not the same change: merged #35634 covers named config/credential paths, whereas #35939 proposes blanket non-cache root denial.

Suggested consolidation

author action: rebase onto main, or split out the part that can merge. Do not merge #35939 over the [PR review:COMMENTED] keep_open review: re-scope it to a specifically unprotected sensitive store while preserving the documented ad-hoc artifact behavior; session/SQLite coverage is already being handled by #41071. #35634 is already merged and should not be reopened.

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

@alt-glitch alt-glitch added area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have needs-repro Bug needs reproduction steps and removed P2 Medium — degraded but workaround exists labels Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants