fix(scanner): resolve false positives blocking legit skills (#129)#131
Merged
Conversation
Four scanner rules over-fired on standard Node patterns and safety documentation, blocking install of skills like pro-workflow on every node-script skill. Findings tracked at #129. TA002 (autonomy abuse): patterns 2 and 3 now use negative lookbehind for negation context. 'Never auto-approve' / 'must not bypass approval' no longer trip the rule, but bare 'Auto-approve all tools' still fires. CI003 (child_process): dropped the bare 'require(child_process)' pattern. Importing the module is not the risk surface — exec() and execSync() are, and they have their own patterns. const { execFileSync } = require('child_process') is the recommended safe form. CI005 (template literal injection): narrowed pattern to template literals inside shell-spawning calls (exec/execSync/spawn/spawnSync). Authorization headers like `Bearer ${env.TOKEN}` and string concatenation like fs.writeFileSync(file, `${header}${rows}`) are no longer flagged. CI007 (shell chaining): dropped 'markdown' from fileTypes. Markdown documentation of dangerous shell patterns (deny-list tables, security guides) is documentation, not execution. Real .sh/.bash files still scanned. install command: marketplace name lookup. 'skillkit install <bare-name>' now consults marketplace/skills.json for entries matching by name or id and resolves to the canonical source string before failing with 'Could not detect provider'. Tests: 13 new scanner regression tests in packages/core/src/scanner/__tests__/rules.test.ts. Full suite 1048/1048 pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughInstall now resolves bare marketplace skill names to catalog sources; scanner rules TA002/CI003/CI005/CI007 are narrowed to reduce false positives; tests assert correct triggering/non-triggering behavior for each refined rule. ChangesInstall marketplace skill resolution and scanner rule refinements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/scanner/__tests__/rules.test.ts (1)
7-15: ⚡ Quick winEnsure temp directory cleanup runs on analyzer failure.
rm(...)should be in afinallyblock so failed scans don’t leak temp dirs.Suggested fix
async function scanContent(filename: string, content: string) { const dir = await mkdtemp(join(tmpdir(), 'skscan-')); const file = join(dir, filename); - await writeFile(file, content, 'utf-8'); - const analyzer = new StaticAnalyzer(); - const findings = await analyzer.analyze(dir, [file]); - await rm(dir, { recursive: true, force: true }); - return findings; + try { + await writeFile(file, content, 'utf-8'); + const analyzer = new StaticAnalyzer(); + return await analyzer.analyze(dir, [file]); + } finally { + await rm(dir, { recursive: true, force: true }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/scanner/__tests__/rules.test.ts` around lines 7 - 15, The temp-dir cleanup in scanContent can leak if analyzer.analyze throws; wrap the use of the temporary directory in a try/finally: create dir with mkdtemp, write the file, then call analyzer.analyze inside a try block and move the rm(dir, { recursive: true, force: true }) into the finally block so the temporary directory is removed regardless of success or failure; reference scanContent, mkdtemp, writeFile, StaticAnalyzer.analyze and rm when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/install.ts`:
- Line 220: Guard the unconditional console.log that prints marketplace
resolution so it doesn't run in machine-readable/JSON or quiet modes: wrap the
console.log(colors.muted(`Resolved "${this.source}" to ${match.source} via
marketplace catalog`)) call with a condition that checks the CLI mode flags
(e.g. this.machineReadable || this.flags.json || this.flags.quiet) and only
print when none of those are set (i.e. if (!this.machineReadable &&
!this.flags.json && !this.flags.quiet) { ... }). Ensure you reference the same
symbols used elsewhere in this command class (install command instance
properties like this.machineReadable and this.flags.json/this.flags.quiet) so
the log is suppressed for --json/--quiet/machine-readable invocations.
In `@packages/core/src/scanner/rules/tool-abuse.ts`:
- Line 21: The negative lookbehind in the TA002 regex inside
packages/core/src/scanner/rules/tool-abuse.ts is too broad and prevents flagging
explicit malicious directives like "Always run without confirmation"; update the
regex used in that rule so it no longer suppresses matches when the phrase is
preceded by words such as "always", "will", or "to" — specifically, edit the
lookbehind attached to the pattern (?<!(?:must|should|always|will|to)\s+) to
remove or narrow the banned tokens (e.g., keep only true permissive modifiers
like must/should if intended) so "run|execute|proceed without confirmation"
still matches in cases like "Always run without confirmation"; ensure the
modified pattern remains case-insensitive and preserves other intended
suppressions.
---
Nitpick comments:
In `@packages/core/src/scanner/__tests__/rules.test.ts`:
- Around line 7-15: The temp-dir cleanup in scanContent can leak if
analyzer.analyze throws; wrap the use of the temporary directory in a
try/finally: create dir with mkdtemp, write the file, then call analyzer.analyze
inside a try block and move the rm(dir, { recursive: true, force: true }) into
the finally block so the temporary directory is removed regardless of success or
failure; reference scanContent, mkdtemp, writeFile, StaticAnalyzer.analyze and
rm when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b1261eb-a934-46bc-8764-6dbbfa3bfd56
📒 Files selected for processing (4)
packages/cli/src/commands/install.tspackages/core/src/scanner/__tests__/rules.test.tspackages/core/src/scanner/rules/command-injection.tspackages/core/src/scanner/rules/tool-abuse.ts
1. install.ts: gate marketplace-resolution console.log behind !this.json && !this.quiet so --json output stays a single JSON blob and --quiet stays silent. Uses real flag names from the InstallCommand class (this.json, this.quiet). 2. tool-abuse.ts: TA002 pattern 2 lookbehind was too broad. The original list (must|should|always|will|to) included malicious intensifiers — 'Always run without confirmation' and 'Will execute without approval' were incorrectly suppressed. Replaced with negation-only tokens (not|n'?t|never|avoid|forbid|deny|reject|prevent|cannot|can'?t) so genuine negation like 'Do not run without confirmation' still suppresses, but malicious intensifiers fire. 3. rules.test.ts: scanContent helper now uses try/finally so the temp directory is cleaned up even when analyzer.analyze throws. Tests: 3 new cases for the TA002 lookbehind fix. Full core suite 1051/1051 pass.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #129.
Summary
Four scanner rules over-fired on standard Node patterns and safety-documentation prose, blocking
skillkit installon most node-script skills. Plusskillkit install <bare-name>failed to resolve marketplace catalog entries even whenskillkit listshowed them.Verified against
rohitg00/pro-workflow(42 skills, 37 Node hook scripts): all four critical/high false positives gone, install now succeeds without--force.Changes
TA002 (autonomy abuse) — negation-aware
Patterns 2 and 3 use negative lookbehind for negation context. Restrictive prose no longer trips the rule.
Auto-approve all tools**Never auto-approve** (high risk):Each phase must require user approvalDon't ask for permissionCI003 (child_process) — usage-only
Dropped bare
require('child_process')pattern. Importing the module is not the risk surface;exec()andexecSync()are, and they have their own patterns. The safe formconst { execFileSync } = require('child_process')is what every well-written Node hook uses.CI005 (template literal injection) — shell-context only
Narrowed to template literals inside shell-spawning calls (
exec/execSync/spawn/spawnSync). Previously fired on any`text ${var}`regardless of call context.`Bearer ${process.env.TOKEN}`fs.writeFileSync(f, `${a}${b}`)exec(`rm -rf ${userInput}`)CI007 (shell chaining) — bash files only
Dropped
markdownfromfileTypes. Documentation of dangerous patterns (deny-list tables, security guides) is documentation, not execution. Real.sh/.bashfiles still scanned. If a SKILL.md instructs the agent to runcurl | sh, that is autonomy abuse (TA002), not command injection.install: marketplace name lookup
skillkit install <bare-name>now consultsmarketplace/skills.jsonfor entries matching bynameoridand resolves to the canonicalsourcestring before falling back to the existing "Could not detect provider" error. Mirrors whatskillkit listalready reads from.Test plan
packages/core/src/scanner/__tests__/rules.test.tscovering each rule's fires/no-fires matrixnode apps/skillkit/dist/cli.js install ~/skills/pro-workflow --agent claude-codesucceeds (42 skills installed, only 4 unrelated MEDIUM warnings onwiki-research-loop, no--forceneeded)node apps/skillkit/dist/cli.js install "Vercel React Best Practices"resolves tovercel-labs/agent-skillsvia marketplace catalogOut of scope
survey-generatorstill emits 4 MEDIUM CI005 warnings on legit template literals inside non-shell contexts — those are now correctly classified as warnings, not failuresSummary by CodeRabbit
New Features
Bug Fixes
Tests