Skip to content

fix(mcp): reject mutable catalog refs at runtime - #38018

Open
coygeek wants to merge 1 commit into
NousResearch:mainfrom
coygeek:fix/optional-mcp-catalog-installs-a-floating-git-ref-whose-bootstrap-executes-attacker-controlled-code-in-the-agent-venv-with-access-to-operator-credentials
Open

fix(mcp): reject mutable catalog refs at runtime#38018
coygeek wants to merge 1 commit into
NousResearch:mainfrom
coygeek:fix/optional-mcp-catalog-installs-a-floating-git-ref-whose-bootstrap-executes-attacker-controlled-code-in-the-agent-venv-with-access-to-operator-credentials

Conversation

@coygeek

@coygeek coygeek commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The shipped-catalog exposure reported in #38017 was independently fixed by #64463 / 9df5f879b: n8n now pins the same full commit SHA proposed here, and a catalog-wide CI contract rejects mutable Git refs in every shipped manifest.

This PR is narrowed to the remaining runtime defense-in-depth boundary. It enforces the documented full-SHA policy while parsing install manifests, so malformed or stale packaged catalog data is rejected before clone or bootstrap.

  • Require install.ref to be a string containing exactly 40 hexadecimal characters.
  • Reject branches, tags, abbreviated SHAs, whitespace-padded values, and non-string values during manifest parsing.
  • Retain the plain-clone plus exact-checkout path for valid commit objects.
  • Remove the now-unreachable branch/tag clone fallback.
  • Do not duplicate the n8n manifest pin or the shipped-catalog contract already on main.

Linked context

Follow-up defense in depth for #38017. The original shipped-catalog condition is fixed by #64463.

Current behavior proof

  1. Rebased onto current main at 477c08b44766ace8b890faa72bf82ecbcf2b3ba8.
  2. Current main documents that Git catalog installs require full commit SHAs and tests every shipped manifest against that invariant.
  3. Current _parse_manifest() still accepts any non-empty install.ref, including branches and tags.
  4. Current _do_git_install() still contains the mutable git clone --branch path and will bootstrap it.
  5. This patch rejects the mutable value during parsing, before the install entry can reach clone or bootstrap.

Files changed

  • hermes_cli/mcp_catalog.py
  • tests/hermes_cli/test_mcp_catalog.py

Tests and validation

  • scripts/run_tests.sh tests/hermes_cli/test_mcp_catalog.py -q
  • Result: 1 file, 45 tests passed, 0 failed (100% complete) in 2.9s
  • .venv/bin/ruff check hermes_cli/mcp_catalog.py tests/hermes_cli/test_mcp_catalog.py
  • Result: all checks passed
  • git diff --check
  • Result: passed

Source state

  • Base: 477c08b44766ace8b890faa72bf82ecbcf2b3ba8
  • Head: 41634e3a44d743fb5ba6b28272826a623c459a05
  • The head has current main as its sole parent.

Risk

This intentionally makes the documented full-SHA catalog policy fail closed at runtime. Package-manager-provided catalog data that still contains a branch, tag, abbreviated SHA, padded value, or non-string ref will be diagnosed as invalid instead of being cloned and bootstrapped.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cli CLI entry point, hermes_cli/, setup wizard tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists labels Jun 3, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused hardening change. The premise is confirmed on current main: optional-mcps/n8n/manifest.yaml:31 still uses ref: main; hermes_cli/mcp_catalog.py:397-421 clones a non-SHA ref and then runs bootstrap commands, while _run_bootstrap() uses shell=True at hermes_cli/mcp_catalog.py:359-371.

The PR's parse-time full-SHA validation prevents the mutable-ref path before installation, updates the only git-install manifest, and retains the clone-then-checkout mechanism appropriate for commit objects. The proposed n8n SHA is currently available as the upstream main head (git ls-remote https://github.com/CyberSamuraiX/hermes-n8n-mcp.git 7a9ae00795593aa1fdb4e61ecd640e8bfd0c3841 refs/heads/main). The patch also applies cleanly to current main via gh pr diff 38018 --repo NousResearch/hermes-agent | git apply --check.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 area/install-update Installer, updater, packaging, wheels, doctor labels Jul 14, 2026
The shipped catalog is now protected by the exact-pin contract added in 9df5f87. Enforce the same full-commit-SHA policy while parsing install manifests so packaged catalog data fails closed before clone or bootstrap if it is malformed or stale.
@coygeek
coygeek force-pushed the fix/optional-mcp-catalog-installs-a-floating-git-ref-whose-bootstrap-executes-attacker-controlled-code-in-the-agent-venv-with-access-to-operator-credentials branch from 2554b69 to 41634e3 Compare July 21, 2026 03:15
@coygeek coygeek changed the title fix(mcp): pin optional catalog installs before bootstrap fix(mcp): reject mutable catalog refs at runtime Jul 21, 2026

coygeek commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased and narrowed this PR after #64463 landed. Commit 9df5f879b already pinned n8n to 7a9ae00795593aa1fdb4e61ecd640e8bfd0c3841 and added the catalog-wide shipped-manifest exact-pin contract, so this branch no longer duplicates either change. The remaining two-file diff enforces the same policy at runtime parsing for packaged catalog data, rejecting mutable or malformed refs before clone/bootstrap. Current head is 41634e3a44d743fb5ba6b28272826a623c459a05, one commit atop current main; all 45 MCP catalog tests and Ruff pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists 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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/mcp MCP client and OAuth type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants