Skip to content

Prevent skills manifests from hashing files outside the skill directory#3410

Merged
jlowin merged 1 commit intomainfrom
codex/fix-symlink-target-hash-vulnerability
Mar 7, 2026
Merged

Prevent skills manifests from hashing files outside the skill directory#3410
jlowin merged 1 commit intomainfrom
codex/fix-symlink-target-hash-vulnerability

Conversation

@jlowin
Copy link
Member

@jlowin jlowin commented Mar 6, 2026

Motivation

  • The skill scanner previously followed filesystem entries (including symlinks) and computed sizes/hashes unconditionally, which allowed a symlink inside a skill folder to disclose hash/size metadata for files outside the skill root via the generated manifest.
  • This is an information disclosure risk because later read-time path checks block access but the manifest already leaked an existence/content oracle (hashes), so discovery must enforce the skill directory boundary.

Description

  • Tighten scan_skill_files to resolve the skill root and each discovered file, and skip any discovered entry whose resolved target is not inside the resolved skill directory before computing stat() or hashing; sizes/hashes are now computed from the resolved in-skill path only. (See src/fastmcp/server/providers/skills/_common.py.)
  • Preserve deterministic discovery ordering and POSIX-style relative paths when including legitimate in-skill files.
  • Add a regression test that creates a symlink inside a skill pointing to a file outside the skill directory and asserts the manifest excludes that symlinked external target. (See tests/server/providers/test_skills_provider.py.)

Testing

  • Ran uv sync and dependency resolution succeeded.
  • Ran uv run ruff check and uv run ruff format for the modified files and fixed formatting; ruff check passed on the changed files.
  • Ran uv run pytest tests/server/providers/test_skills_provider.py -n auto and the skill-provider tests (including the new regression test) passed (43 passed).
  • Ran the full test suite with uv run pytest -n auto; unrelated existing tests in the environment showed several timeouts/failures (external to this change) while the modified skill-provider tests passed locally.
  • uv run prek run --all-files failed to initialize hooks due to network restrictions fetching a pre-commit hook, not due to code issues.

Codex Task

🤖 Generated with GPT-5.2-Codex
@marvin-context-protocol marvin-context-protocol bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality. high-priority labels Mar 6, 2026
@jlowin
Copy link
Member Author

jlowin commented Mar 7, 2026

Auto-reviewed and merging on behalf of @jlowin — CI is green (Windows OAuth proxy timeouts are pre-existing flaky tests).

@jlowin jlowin merged commit ea19a2a into main Mar 7, 2026
8 checks passed
@jlowin jlowin deleted the codex/fix-symlink-target-hash-vulnerability branch March 7, 2026 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. codex high-priority server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant