Skip to content

fix: harden skill bundles and plugin API security - #19275

Open
abhinav11082001-stack wants to merge 2 commits into
NousResearch:mainfrom
abhinav11082001-stack:fix/skills-hub-binary-hash-clean
Open

fix: harden skill bundles and plugin API security#19275
abhinav11082001-stack wants to merge 2 commits into
NousResearch:mainfrom
abhinav11082001-stack:fix/skills-hub-binary-hash-clean

Conversation

@abhinav11082001-stack

@abhinav11082001-stack abhinav11082001-stack commented May 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • allow skill hub bundle hashing to handle both text and binary files
  • require dashboard session auth for plugin HTTP API routes and constrain plugin API imports to files under the plugin dashboard directory
  • replace Docker cleanup shell strings with argv-safe helper process cleanup that survives parent exit
  • update bundled plugin frontend/docs for session-aware plugin API calls
  • refresh Python/Node dependency locks to clear known audit findings
  • add regression coverage for binary bundle hashing, plugin API auth/path validation, session-aware plugin fetches, and argv-safe Docker cleanup

Test Plan

  • venv/bin/python -m pytest tests/hermes_cli/test_web_server_host_header.py tests/tools/test_docker_environment.py tests/tools/test_skills_hub_bundle_hash.py -q -o 'addopts=' (36 passed)
  • venv/bin/python - <<'PY' ... ast.parse(...) ... PY for modified Python files (OK)
  • git diff --check (clean)
  • uv audit --locked --no-progress --color never (no known vulnerabilities)
  • npm audit --package-lock-only --audit-level=low in ui-tui and website (0 vulnerabilities)

Notes

  • Full repository pytest was attempted with -q -o 'addopts=' but timed out after 600s with unrelated existing failures/skips before completion, so the gate is targeted regression tests plus Python/Node audits.
  • The messaging extra now depends on discord.py without the voice extra to avoid the vulnerable PyNaCl<1.6 transitive pin from discord.py[voice].

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) labels May 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #9925 — same fix for bundle_content_hash() crashing on binary files. Also duplicates #18367 and #19079.

@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #9925

@abhinav11082001-stack abhinav11082001-stack changed the title fix: hash binary skill bundle files fix: harden skill bundles and plugin API security May 3, 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 security-focused work. Most production fixes in this branch are already present on current main: binary bundle hashing is handled in tools/skills_hub.py:3573-3586; plugin API containment has discovery and mount-time checks in hermes_cli/web_server.py:16111-16145 and 16666-16775; Docker cleanup uses bounded argv-based subprocess.run calls in tools/environments/docker.py:1407-1459.

Problems

  • The changed pyproject.toml messaging line removes discord.py[voice]. Current main intentionally installs that extra in pyproject.toml:161 and tools/lazy_deps.py:162; voice-channel joining depends on those dependencies (gateway/run.py:12764-12803, plugins/platforms/discord/adapter.py:2852). This would regress an active supported feature.

Suggested changes

  • Re-scope any remaining dependency remediation onto current pins while retaining the voice extra, and verify the audit plus Discord voice dependency path.
  • Do not carry forward the runtime fixes already implemented on main.

This is an automated hermes-sweeper review.

Comment thread pyproject.toml
dev = ["debugpy>=1.8.0,<2", "pytest>=9.0.2,<10", "pytest-asyncio>=1.3.0,<2", "pytest-xdist>=3.0,<4", "mcp>=1.2.0,<2", "ty>=0.0.1a29,<0.0.22", "ruff"]
messaging = ["python-telegram-bot[webhooks]>=22.6,<23", "discord.py[voice]>=2.7.1,<3", "aiohttp>=3.13.3,<4", "slack-bolt>=1.18.0,<2", "slack-sdk>=3.27.0,<4", "qrcode>=7.0,<8"]
dev = ["debugpy>=1.8.0,<2", "pytest>=9.0.3,<10", "pytest-asyncio>=1.3.0,<2", "pytest-xdist>=3.0,<4", "mcp>=1.2.0,<2", "ty>=0.0.1a29,<0.0.22", "ruff"]
messaging = ["python-telegram-bot[webhooks]>=22.6,<23", "discord.py>=2.7.1,<3", "aiohttp>=3.13.4,<4", "slack-bolt>=1.18.0,<2", "slack-sdk>=3.27.0,<4", "qrcode>=7.0,<8"]

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.

Removing [voice] drops the dependency path needed for the current Discord voice-channel feature. Current main deliberately retains discord.py[voice] in both the messaging extra and tools/lazy_deps.py; please preserve voice support and address any remaining dependency concern with a compatible current-lock resolution.

@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 labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants