-
Notifications
You must be signed in to change notification settings - Fork 61
feat: add security scanner for detecting malicious patterns in skills #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
822bd6f
feat: add security scanner for detecting malicious patterns in skills
rohitg00 bf0f3e1
fix: address Devin and CodeRabbit review suggestions for security sca…
rohitg00 56b5fa2
fix: address CodeRabbit round 2 review suggestions
rohitg00 e392643
fix: address deep review issues in security scanner
rohitg00 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import { Command, Option } from 'clipanion'; | ||
| import { resolve } from 'node:path'; | ||
| import { existsSync } from 'node:fs'; | ||
| import { SkillScanner, formatResult, Severity } from '@skillkit/core'; | ||
|
|
||
| const SEVERITY_MAP: Record<string, Severity> = { | ||
| critical: Severity.CRITICAL, | ||
| high: Severity.HIGH, | ||
| medium: Severity.MEDIUM, | ||
| low: Severity.LOW, | ||
| info: Severity.INFO, | ||
| }; | ||
|
|
||
| export class ScanCommand extends Command { | ||
| static override paths = [['scan']]; | ||
|
|
||
| static override usage = Command.Usage({ | ||
| description: 'Scan a skill directory for security vulnerabilities', | ||
| details: ` | ||
| Analyzes skill files for prompt injection, command injection, data exfiltration, | ||
| tool abuse, hardcoded secrets, and unicode steganography. | ||
|
|
||
| Outputs findings in various formats including SARIF for GitHub Code Scanning. | ||
| `, | ||
| examples: [ | ||
| ['Scan current directory', '$0 scan .'], | ||
| ['Scan a specific skill', '$0 scan ./my-skill'], | ||
| ['Output as JSON', '$0 scan ./my-skill --format json'], | ||
| ['Output as SARIF', '$0 scan ./my-skill --format sarif'], | ||
| ['Fail on high severity', '$0 scan ./my-skill --fail-on high'], | ||
| ['Skip unicode rules', '$0 scan ./my-skill --skip-rules UC001,UC002'], | ||
| ], | ||
| }); | ||
|
|
||
| skillPath = Option.String({ required: true, name: 'path' }); | ||
|
|
||
| format = Option.String('--format,-f', 'summary', { | ||
| description: 'Output format: summary, json, table, sarif', | ||
| }); | ||
|
|
||
| failOn = Option.String('--fail-on', { | ||
| description: 'Exit with code 1 if findings at this severity or above (critical, high, medium, low)', | ||
| }); | ||
|
|
||
| skipRules = Option.String('--skip-rules', { | ||
| description: 'Comma-separated rule IDs or categories to skip', | ||
| }); | ||
|
|
||
| async execute(): Promise<number> { | ||
| const targetPath = resolve(this.skillPath); | ||
|
|
||
| if (!existsSync(targetPath)) { | ||
| this.context.stderr.write(`Path not found: ${targetPath}\n`); | ||
| return 1; | ||
| } | ||
|
|
||
| const validFormats = ['summary', 'json', 'table', 'sarif']; | ||
| if (!validFormats.includes(this.format)) { | ||
| this.context.stderr.write(`Invalid format: "${this.format}". Must be one of: ${validFormats.join(', ')}\n`); | ||
| return 1; | ||
| } | ||
|
|
||
| const skipRules = this.skipRules?.split(',').map((s) => s.trim()) ?? []; | ||
|
|
||
| let failOnSeverity: Severity | undefined; | ||
| if (this.failOn) { | ||
| failOnSeverity = SEVERITY_MAP[this.failOn.toLowerCase()]; | ||
| if (!failOnSeverity) { | ||
| this.context.stderr.write(`Invalid --fail-on value: "${this.failOn}". Must be one of: ${Object.keys(SEVERITY_MAP).join(', ')}\n`); | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| const scanner = new SkillScanner({ | ||
| failOnSeverity, | ||
| skipRules, | ||
| }); | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const result = await scanner.scan(targetPath); | ||
|
|
||
| this.context.stdout.write(formatResult(result, this.format) + '\n'); | ||
|
|
||
| return result.verdict === 'fail' ? 1 : 0; | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import type { Finding } from '../types.js'; | ||
|
|
||
| export interface Analyzer { | ||
| name: string; | ||
| analyze(skillPath: string, files: string[]): Promise<Finding[]>; | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 No security warning shown when --force bypasses a failing scan during install
When a user runs
skillkit install <source> --forceand the security scan returns afailverdict (CRITICAL/HIGH findings), the install proceeds silently with zero feedback about the security issues.Detailed Explanation
In
packages/cli/src/commands/install.ts:270-285, the scan result handling has two branches:When
--forceis true and the verdict isfail:true && false→ skipped (no error shown)'fail' === 'warn'→ false → skipped (no warning shown)The user force-installs a skill with CRITICAL or HIGH severity findings and receives no indication that security issues were detected. At minimum, a warning should be displayed even when
--forceoverrides the block.Impact: Users who use
--force(perhaps to override an existing install) may unknowingly install skills with serious security vulnerabilities without any feedback.Was this helpful? React with 👍 or 👎 to provide feedback.