Skip to content

fix(gateway): deny sibling-profile credential paths in media delivery - #47220

Open
JiraiyaETH wants to merge 1 commit into
NousResearch:mainfrom
JiraiyaETH:fix/sibling-profile-config-denial
Open

fix(gateway): deny sibling-profile credential paths in media delivery#47220
JiraiyaETH wants to merge 1 commit into
NousResearch:mainfrom
JiraiyaETH:fix/sibling-profile-config-denial

Conversation

@JiraiyaETH

Copy link
Copy Markdown

_media_delivery_denied_paths() blocks the active profile's control files (auth.json/.env/config.yaml/credentials) but not other profiles' on a multi-profile host — a message referencing ~/.hermes/profiles/<other>/.env passes through. Fix: iterate the profiles root and deny all profile credential files. Closes a multi-profile credential-exfiltration vector.

Runtime-only carry from 4950298476af; tests/support kept in external overlay artifact, not live checkout.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Jun 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the sibling-profile delivery gap. The issue remains present on current main: _media_delivery_denied_paths() applies credential entries only to _HERMES_HOME and _HERMES_ROOT (gateway/platforms/base.py:1198-1202), while default mode returns any non-denied regular file (gateway/platforms/base.py:1327-1330).

Problems

  • The proposed four-file sibling list is now incomplete. Current main additionally protects OAuth/token stores, auth.lock, webhook secrets, cache/bws_cache.json, pairing/, and mcp-tokens/ (gateway/platforms/base.py:1166-1202).
  • No regression test exercises a sibling profile under an otherwise permitted Hermes root. The existing profile credential test uses a separately denied parent prefix (tests/gateway/test_platform_base.py:1330-1355).

Suggested changes

  • Enumerate profile directories as _profile_cache_roots() already does (gateway/platforms/base.py:1050-1072) and apply the full current credential file/directory policy to each profile root.
  • Add a non-strict-mode sibling-profile regression test, including a newer credential class and a non-credential control case.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 14, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs address the sibling-profile credential-exfiltration gap in default media delivery. #47220 adds a narrow four-entry denylist, while #70144 applies the full current credential policy to every live profile directory and adds regression coverage for both blocked credentials and permitted non-credential files.

Related pull requests

  • #47220 related — (+14/-0) — superseded by #70144: This identifies the correct cause and denies four sibling-profile control paths, but its list omits current OAuth/token stores, auth.lock, webhook secrets, cache/bws_cache.json, pairing/, and mcp-tokens/, and it adds no regression test. Despite the keep_open review on #47220, that review explicitly requires the full current policy and sibling-profile test coverage, both of which #70144 supplies.
  • #70144 duplicate — (+133/-12) — preferred fix: This reuses one live profile-directory enumeration path, applies the complete existing file-and-directory credential policy to active, shared-root, and sibling profiles, and deduplicates resolved roots. Its tests cover the reported non-strict-mode exfiltration path, root-home mode, the full sensitive-path set, and continued delivery of sibling non-credential files.

Duplicates

#47220 and #70144 address the same sibling-profile media-delivery vulnerability; #70144 is the comprehensive superset of #47220.

Suggested consolidation

Merge #70144 because its diff closes the full policy gap and provides targeted regression coverage. Close #47220 as superseded by #70144; this does not disregard its keep_open review, because the review's blocking concerns—an incomplete credential list and missing sibling-profile tests—are addressed by #70144 rather than by #47220's current diff.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup47220 ["PRs duplicating each other"]
        P47220["PR #47220 (open)"]
        P70144["PR #70144 (open)"]
    end
    class P47220 open
    class P70144 open
    class P47220 target
    click P47220 "https://github.com/NousResearch/hermes-agent/pull/47220"
    click P70144 "https://github.com/NousResearch/hermes-agent/pull/70144"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

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, 4 kB of issue/PR text, 2 kB of discussion (2 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Delta since our previous triage comment

@egilewski identified a fail-open residual bypass in #70144 when the profiles directory cannot be enumerated, correcting our earlier assumption that enumeration alone was sufficient. @fangliquanflq then changed the implementation to classify sibling-profile credential paths structurally without directory listing and added a regression test for the enumeration-failure case; the supplied current diff confirms both changes. @teknium1’s new keep_open review remains appropriate until the contributor-requested fix is re-reviewed and the updated head receives passing verification.

Changed pull requests

  • #70144 duplicate — (+266/-53) — preferred fix, pending re-review and verification: The current diff now denies the full credential policy under <root>/profiles/<name>/ structurally, including when profile enumeration fails, while preserving delivery of non-credential sibling files and testing that boundary. This addresses @egilewski’s blocking fail-open objection in code, but does not justify merging over the outstanding changes-requested review until that resolution is confirmed; it is also consistent with @teknium1’s keep_open review.

Suggested consolidation

The recommendation is unchanged: keep #70144 open for contributor re-review and passing verification, then merge it and close #47220 as superseded.

Complex graph unchanged since our previous triage comment.

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

@egilewski

Copy link
Copy Markdown
Contributor

not enough evidence

I could not form a coherent review tree against current GitHub main at 9fc12bf7a4bc232698a14acfb18621523a711ceb: the run-owned merge-tree confirmed a content conflict, and the deterministic patch replay failed with patch_replay_conflict. The stale/conflict status is informational and was not treated as a standalone blocker. Without a coherent integrated tree, I cannot determine how this patch should compose with the expanded credential-file and credential-directory sets on current main, so I cannot reach a security conclusion on the intended integrated change.

Please rebase or resolve the patch onto current main and include runnable sibling-profile tests showing that direct and symlink-resolved access to every canonical credential file and credential directory is denied in both default and strict modes, while ordinary noncredential files and documented generated-media roots retain their intended behavior. The PR head only enumerates .env, auth.json, credentials, and config.yaml under existing profiles, whereas current main also protects additional credential files plus the pairing and mcp-tokens directories; the integrated implementation should reuse the canonical definitions rather than preserve this stale four-name subset.

Read-only inspection confirmed that current main has no sibling-profile traversal and that the PR head adds the 14-line scan against the older four-name set; python -B -m compileall gateway/platforms/base.py succeeded at the PR head, but the tests referenced by the commit were not present in the reviewed checkout and no behavioral test was run.

Signed: GPT-5.6-sol-xhigh in Codex

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 area/profiles Multi-profile isolation, HERMES_HOME scoping comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

5 participants