Skip to content

fix: remove shell usage from plugin check#1367

Merged
aaronpowell merged 2 commits intostagedfrom
fix/check-plugin-structure-command-injection
Apr 10, 2026
Merged

fix: remove shell usage from plugin check#1367
aaronpowell merged 2 commits intostagedfrom
fix/check-plugin-structure-command-injection

Conversation

@aaronpowell
Copy link
Copy Markdown
Contributor

Summary

  • replace the shell-based symlink scan in check-plugin-structure.yml with a filesystem walk
  • prevent command injection through malicious plugin directory names in pull requests
  • preserve the existing workflow behavior for detecting symlinks in plugin directories

Context

This addresses the command injection reported in github/next#264 by removing the interpolated shell invocation entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 10, 2026 04:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the plugin-structure GitHub Actions workflow to eliminate shell execution when scanning plugin directories for symlinks, mitigating command-injection risk from malicious plugin names in PRs.

Changes:

  • Replaces the find shell invocation with a JavaScript filesystem walk to detect symlinks.
  • Keeps the existing “materialized files” checks for agents/commands/skills subdirectories intact.
Show a summary per file
File Description
.github/workflows/check-plugin-structure.yml Removes shell-based symlink detection and replaces it with an in-process directory traversal to avoid command injection.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the check-plugin-structure GitHub Actions workflow by removing a shell-based find invocation and replacing it with a Node.js filesystem walk to detect symlinks, addressing a potential command-injection vector from untrusted plugin directory names in PRs.

Changes:

  • Removed child_process.execSync usage in the workflow’s symlink detection logic.
  • Added a findSymlinks() filesystem traversal using readdirSync + lstatSync to identify symlinks without invoking a shell.
  • Updated symlink error reporting and added error handling for directory/entry inspection failures.
Show a summary per file
File Description
.github/workflows/check-plugin-structure.yml Replaces shell-based symlink scanning with a Node-based directory walk to eliminate command injection risk.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +106 to +108
if (symlinkPaths.length > 0) {
const formattedPaths = symlinkPaths.map(filePath => `\`${filePath}\``).join(', ');
errors.push(`${pluginPath} contains symlinks: ${formattedPaths}`);
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

formattedPaths wraps user-controlled filesystem names in Markdown inline code without escaping. A crafted symlink filename containing backticks/newlines (or @mentions) can break formatting and potentially inject unwanted Markdown into the PR review body. Consider escaping Markdown special characters in filePath (at least backticks/newlines) or rendering the symlink list inside a fenced code block with safe escaping.

Copilot uses AI. Check for mistakes.
@aaronpowell aaronpowell merged commit 7df3657 into staged Apr 10, 2026
12 checks passed
@aaronpowell aaronpowell deleted the fix/check-plugin-structure-command-injection branch April 10, 2026 04:25
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