Skip to content

fix(cli): scan MCP configs and skills in directories - #752

Open
dc-tw wants to merge 1 commit into
Agent-Threat-Rule:mainfrom
dc-tw:fix/directory-scan-575
Open

dc-tw wants to merge 1 commit into
Agent-Threat-Rule:mainfrom
dc-tw:fix/directory-scan-575

Conversation

@dc-tw

@dc-tw dc-tw commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • treat a directory as a collection of scan targets instead of passing it to readFileSync
  • recursively scan both SKILL.md files and recognized MCP config files (.mcp.json, mcp.json, mcp-config.json, and claude_desktop_config.json)
  • aggregate both paths into one JSON, SARIF, or terminal report
  • skip .git and node_modules, retain the existing per-file size limits, and report skipped config files

This fixes the directory portion of #575. In particular, the Action's default atr scan . --sarif can now inspect both nested MCP configs and skills instead of choosing one input type for the whole directory or passing the directory to readFileSync.

It does not change action.yml; the independent scan-exit handling is covered by #743. It also leaves all single-file MCP config, event-log, tools/list, and skill behavior unchanged.

Refs #575

Validation

  • npm run typecheck
  • npm run build
  • npm run validate — 844/844 rules
  • focused CLI regression suite — 28/28 tests
  • npm test -- --maxWorkers=4 — 153 files passed, 2 skipped; 2,608 tests passed, 14 skipped
  • git diff --check

@eeee2345 eeee2345 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.

@dc-tw thanks, this covers the directory half of #575. I reproduced the false clean on main (a .cursor/mcp.json with a NODE_OPTIONS --require payload next to a SKILL.md comes back with 0 threats) and the PR fixes it. Typecheck is clean and the CLI suites pass, plus the advisory, tool-definition-scope and enforcement suites. Symlink loops terminate, .git and node_modules are skipped, an empty dir and an all-skipped dir both exit 1, and an oversized SKILL.md now exits 1 instead of reporting one clean skill as main does.

Two things before merge, both because this scans repositories we do not control:

  1. The walker reads entries that are not regular files inside the scanned tree. A committed SKILL.md or mcp.json that is a symlink to something outside the root, or to /dev/zero, a FIFO or /proc/self/environ, is followed and read, which can hang or exhaust the CI job or pull outside content into the report. Since readdirSync(..., { withFileTypes: true }) gives lstat semantics, checking entry.isFile() first and counting symlinks and other non-regular entries as skipped (with a warning) would close it; a test with a symlink pointing outside the root would pin it.
  2. Skipped files do not reach SARIF. --json reports files_skipped, but the SARIF that the Action and the Security tab consume has nothing, and the run exits 0, so an MCP config that was not parsed shows up as clean. One toolExecutionNotifications entry per skipped file (level warning, with the reason) in runs[0].invocations[0] would make it visible.

For a follow-up rather than this PR: a tools/list document or an event log inside the directory is skipped without any message, and .vscode/mcp.json uses a servers key, so it is recognized by name but skipped as not a config. We'll note the scan_type: "directory" JSON shape in the changelog.

@eeee2345

Copy link
Copy Markdown
Contributor

@dc-tw thanks. A follow-up to the earlier review: I ran the branch against a few more directory shapes and found things I should have caught in the first pass. I am sending them now, before your next push, so everything can go in one round. Besides the two earlier points, item 1 is the only one I would hold the merge on; 2 is mostly tests for behaviour you already implemented, and 3 is optional.

  1. A single unreadable or broken entry aborts the whole scan. A subdirectory without read permission (readdirSync at src/cli/scan-handler.ts:606), a SKILL.md without read permission (readFileSync at :244), or a dangling symlink named SKILL.md or mcp.json (statSync at :213 / :239) each end the run with exit 1 and nothing on stdout, so with --sarif there is no document and the findings in every other file are lost. In the same tree an unparseable config is skipped with a warning (:218-224), so the two target types behave differently. The isFile() check from the earlier review would cover the dangling symlink, but not the two permission cases. Reporting these as skipped, the way configs already are, would keep one bad entry from hiding the rest, and with the SARIF notifications from the earlier point 2 they would show up there as well.

  2. The new tests cover the mixed-directory JSON and SARIF path. The ones that matter most would pin the earlier two points and item 1: a symlink pointing outside the root, an unreadable entry, and a skipped file appearing in SARIF. The other behaviours the description promises are not pinned yet: skipping .git and node_modules, files_skipped for an oversized or unparseable file, exit 1 for an empty or all-skipped directory, and --fail-on in directory mode. Those would be good to have here too; if that makes the PR too large, a follow-up is fine.

  3. Optional, here or later: a clean directory scan in terminal mode prints the summary but not the "No threats detected." line the other modes print (terminal branch at :265-270), and the summary leaves out the skipped count that --json reports. The help text at src/cli.ts:59 still calls scan-skill an alias for scan, which is no longer true for directories.

Not for this PR: when the scanned directory is outside the current working directory, toArtifactUri (src/converters/sarif.ts:77-85) reduces each path to its basename, so with several SKILL.md files every SARIF result points at "SKILL.md". That predates this change and the Action runs from the repository root, but directory mode makes it the common case, so it may be worth its own issue.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants