-
Notifications
You must be signed in to change notification settings - Fork 227
feat(rules): external rule-pack loader (--rule-pack) #107
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { resolve } from "node:path"; | |||||
| import { dirname, join } from "node:path"; | ||||||
| import { existsSync, writeFileSync, appendFileSync, mkdirSync } from "node:fs"; | ||||||
| import { scan } from "./scanner/index.js"; | ||||||
| import { loadRulePacks } from "./rules/external.js"; | ||||||
| import { calculateScore } from "./reporter/score.js"; | ||||||
| import { renderTerminalReport } from "./reporter/terminal.js"; | ||||||
| import { renderJsonReport, renderMarkdownReport } from "./reporter/json.js"; | ||||||
|
|
@@ -288,6 +289,12 @@ program | |||||
| .option("--gate", "Fail if new critical/high findings or score drops (use with --baseline)", false) | ||||||
| .option("--supply-chain", "Verify MCP npm packages against known-bad list and typosquatting", false) | ||||||
| .option("--supply-chain-online", "Also query npm registry for metadata (requires network)", false) | ||||||
| .option( | ||||||
| "--rule-pack <path>", | ||||||
| "Load an external JSON rule pack and run it alongside built-in rules (repeatable)", | ||||||
| (value: string, previous: string[]) => [...previous, value], | ||||||
| [] as string[] | ||||||
| ) | ||||||
| .option("--policy <path>", "Validate against an organization policy file") | ||||||
| .option("--evidence-pack <dir>", "Write a portable evidence bundle for audits and security reviews") | ||||||
| .option("--remediation-plan <path>", "Write a stable-fingerprint JSON remediation plan") | ||||||
|
|
@@ -312,9 +319,29 @@ program | |||||
| const enableTaint = options.deep || options.taint; | ||||||
| const enableOpus = options.deep || options.opus; | ||||||
|
|
||||||
| // ── External rule packs (--rule-pack) ──────────────────── | ||||||
| const rulePackPaths: string[] = options.rulePack ?? []; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use immutable array typing for collected rule-pack paths. The new local collection is typed as mutable Suggested fix- const rulePackPaths: string[] = options.rulePack ?? [];
+ const rulePackPaths: ReadonlyArray<string> = options.rulePack ?? [];As per coding guidelines, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||
| let extraRules = undefined; | ||||||
| if (rulePackPaths.length > 0) { | ||||||
| const loaded = loadRulePacks(rulePackPaths); | ||||||
| if (!loaded.success) { | ||||||
| console.error(`Error: ${loaded.error}`); | ||||||
| process.exit(1); | ||||||
| } | ||||||
| extraRules = loaded.rules; | ||||||
| for (const pack of loaded.packs) { | ||||||
| process.stderr.write(` Loaded ${pack.ruleCount} external rules from ${pack.name}\n`); | ||||||
| logger.log({ | ||||||
| level: "info", | ||||||
| phase: "init", | ||||||
| message: `Loaded ${pack.ruleCount} external rules from ${pack.name}`, | ||||||
| }); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // ── Phase 1: Static rule-based scan ────────────────────── | ||||||
| logger.log({ level: "info", phase: "static", message: "Running static analysis" }); | ||||||
| const result = scan(targetPath); | ||||||
| const result = scan(targetPath, { extraRules }); | ||||||
|
|
||||||
| // Filter by severity | ||||||
| const filteredResult = { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| import { existsSync, readFileSync } from "node:fs"; | ||
| import { z } from "zod"; | ||
| import type { ConfigFile, Finding, FindingCategory, Rule, Severity } from "../types.js"; | ||
|
|
||
| /** | ||
| * External rule-pack loader (`--rule-pack`). | ||
| * | ||
| * AgentShield's built-in detectors are declarative (id/severity/category/pattern | ||
| * rows). This loads the same shape from an external JSON pack so scans can run | ||
| * community detection rules (e.g. the MIT Agent Threat Rules pack) without | ||
| * recompiling the binary. Generic: anyone with a pack in this shape can plug in. | ||
| * | ||
| * Security note: patterns are compiled to RegExp and run against file content. | ||
| * Packs are loaded only when the operator explicitly passes `--rule-pack`, so | ||
| * they are trusted input, but we still fail closed on invalid JSON, bad schema, | ||
| * or any pattern that does not compile, and we cap findings per rule per file to | ||
| * bound pathological packs. ReDoS-hardening of arbitrary external patterns is a | ||
| * known v1 limitation (documented for the loader, same as any regex linter). | ||
| */ | ||
|
|
||
| const MAX_FINDINGS_PER_RULE_PER_FILE = 200; | ||
|
|
||
| const SeveritySchema = z.enum(["critical", "high", "medium", "low", "info"]); | ||
|
|
||
| const CategorySchema = z.enum([ | ||
| "secrets", | ||
| "permissions", | ||
| "hooks", | ||
| "mcp", | ||
| "skills", | ||
| "agents", | ||
| "injection", | ||
| "exposure", | ||
| "exfiltration", | ||
| "misconfiguration", | ||
| ]); | ||
|
|
||
| const RulePackEntrySchema = z.object({ | ||
| id: z.string().min(1), | ||
| name: z.string().min(1), | ||
| description: z.string().optional(), | ||
| severity: SeveritySchema, | ||
| category: CategorySchema, | ||
| patterns: z.array(z.string().min(1)).min(1), | ||
| flags: z.string().optional(), | ||
| fileTypes: z.array(z.string()).optional(), | ||
| }); | ||
|
|
||
| export const RulePackSchema = z.object({ | ||
| version: z.literal(1), | ||
| name: z.string().optional(), | ||
| description: z.string().optional(), | ||
| rules: z.array(RulePackEntrySchema).min(1), | ||
| }); | ||
|
|
||
| export type RulePackEntry = z.infer<typeof RulePackEntrySchema>; | ||
| export type RulePack = z.infer<typeof RulePackSchema>; | ||
|
|
||
| export interface LoadRulePackResult { | ||
| readonly success: boolean; | ||
| readonly rules?: ReadonlyArray<Rule>; | ||
| readonly meta?: { readonly name: string; readonly ruleCount: number }; | ||
| readonly error?: string; | ||
| } | ||
|
|
||
| function findLineNumber(content: string, matchIndex: number): number { | ||
| return content.substring(0, matchIndex).split("\n").length; | ||
| } | ||
|
|
||
| function findAllMatches(content: string, pattern: RegExp): Array<RegExpMatchArray> { | ||
| return [...content.matchAll(pattern)]; | ||
| } | ||
|
Comment on lines
+70
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift Refactor new collection logic to immutable This module introduces mutable array typings and mutation ( As per coding guidelines, Also applies to: 97-99, 139-140, 173-174 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| /** Ensure the global flag is present (matchAll requires it) and dedupe flags. */ | ||
| function normalizeFlags(flags: string | undefined): string { | ||
| const set = new Set((flags ?? "").split("")); | ||
| set.add("g"); | ||
| return [...set].join(""); | ||
| } | ||
|
|
||
| function compileEntryPatterns(entry: RulePackEntry): ReadonlyArray<RegExp> { | ||
| const flags = normalizeFlags(entry.flags); | ||
| return entry.patterns.map((source) => new RegExp(source, flags)); | ||
| } | ||
|
|
||
| function entryToRule(entry: RulePackEntry, compiled: ReadonlyArray<RegExp>): Rule { | ||
| const fileTypes = entry.fileTypes ? new Set(entry.fileTypes) : null; | ||
| return { | ||
| id: `external-${entry.id}`, | ||
| name: entry.name, | ||
| description: entry.description ?? entry.name, | ||
| severity: entry.severity as Severity, | ||
| category: entry.category as FindingCategory, | ||
| check(file: ConfigFile): ReadonlyArray<Finding> { | ||
| if (fileTypes && !fileTypes.has(file.type)) return []; | ||
|
|
||
| const findings: Finding[] = []; | ||
| let seq = 0; | ||
| for (const pattern of compiled) { | ||
| for (const match of findAllMatches(file.content, pattern)) { | ||
| findings.push({ | ||
| id: `external-${entry.id}-${seq}`, | ||
| severity: entry.severity as Severity, | ||
| category: entry.category as FindingCategory, | ||
| title: entry.name, | ||
| description: `${entry.description ?? entry.name} (external rule ${entry.id}).`, | ||
| file: file.path, | ||
| line: findLineNumber(file.content, match.index ?? 0), | ||
| evidence: match[0].substring(0, 100), | ||
| }); | ||
| seq += 1; | ||
| if (findings.length >= MAX_FINDINGS_PER_RULE_PER_FILE) return findings; | ||
|
Comment on lines
+97
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } | ||
| return findings; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Load and validate an external rule pack from a JSON file path, returning | ||
| * ready-to-run Rule objects. Fails closed on missing file, invalid JSON, schema | ||
| * mismatch, duplicate ids, or any pattern that does not compile. | ||
| */ | ||
| export function loadRulePack(rulePackPath: string): LoadRulePackResult { | ||
| if (!existsSync(rulePackPath)) { | ||
| return { success: false, error: `Rule pack not found: ${rulePackPath}` }; | ||
| } | ||
|
|
||
| let pack: RulePack; | ||
| try { | ||
| pack = RulePackSchema.parse(JSON.parse(readFileSync(rulePackPath, "utf-8"))); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| return { success: false, error: `Invalid rule pack ${rulePackPath}: ${message}` }; | ||
| } | ||
|
|
||
| const seenIds = new Set<string>(); | ||
| const rules: Rule[] = []; | ||
| for (const entry of pack.rules) { | ||
| if (seenIds.has(entry.id)) { | ||
| return { success: false, error: `Duplicate rule id in pack: ${entry.id}` }; | ||
| } | ||
| seenIds.add(entry.id); | ||
|
|
||
| let compiled: ReadonlyArray<RegExp>; | ||
| try { | ||
| compiled = compileEntryPatterns(entry); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| return { success: false, error: `Rule "${entry.id}" has an invalid pattern: ${message}` }; | ||
| } | ||
| rules.push(entryToRule(entry, compiled)); | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| rules, | ||
| meta: { name: pack.name ?? rulePackPath, ruleCount: rules.length }, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Load several rule packs and concatenate their rules. Returns the first error | ||
| * encountered, or all rules plus per-pack metadata on success. | ||
| */ | ||
| export function loadRulePacks(paths: ReadonlyArray<string>): { | ||
| readonly success: boolean; | ||
| readonly rules: ReadonlyArray<Rule>; | ||
| readonly packs: ReadonlyArray<{ name: string; ruleCount: number }>; | ||
| readonly error?: string; | ||
| } { | ||
| const rules: Rule[] = []; | ||
| const packs: Array<{ name: string; ruleCount: number }> = []; | ||
| for (const path of paths) { | ||
| const result = loadRulePack(path); | ||
| if (!result.success || !result.rules || !result.meta) { | ||
| return { success: false, rules: [], packs: [], error: result.error }; | ||
| } | ||
| rules.push(...result.rules); | ||
| packs.push(result.meta); | ||
| } | ||
|
Comment on lines
+167
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Enforce rule-ID uniqueness across all loaded packs.
Suggested fix export function loadRulePacks(paths: ReadonlyArray<string>): {
readonly success: boolean;
readonly rules: ReadonlyArray<Rule>;
readonly packs: ReadonlyArray<{ name: string; ruleCount: number }>;
readonly error?: string;
} {
const rules: Rule[] = [];
const packs: Array<{ name: string; ruleCount: number }> = [];
+ const seenRuleIds = new Set<string>();
for (const path of paths) {
const result = loadRulePack(path);
if (!result.success || !result.rules || !result.meta) {
return { success: false, rules: [], packs: [], error: result.error };
}
+ for (const rule of result.rules) {
+ if (seenRuleIds.has(rule.id)) {
+ return { success: false, rules: [], packs: [], error: `Duplicate rule id across packs: ${rule.id}` };
+ }
+ seenRuleIds.add(rule.id);
+ }
rules.push(...result.rules);
packs.push(result.meta);
}
return { success: true, rules, packs };
}🤖 Prompt for AI Agents |
||
| return { success: true, rules, packs }; | ||
|
Comment on lines
+167
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,12 +21,18 @@ export interface ScanResult { | |
| readonly harnessAdapters?: HarnessAdapterSummary; | ||
| } | ||
|
|
||
| export interface ScanOptions { | ||
| /** Extra rules loaded from external rule packs (`--rule-pack`). */ | ||
| readonly extraRules?: ReadonlyArray<Rule>; | ||
| } | ||
|
|
||
| /** | ||
| * Main scanner: discovers config files and runs all rules against them. | ||
| * External rule packs (if any) run alongside the built-in rules. | ||
| */ | ||
| export function scan(targetPath: string): ScanResult { | ||
| export function scan(targetPath: string, options: ScanOptions = {}): ScanResult { | ||
| const target = discoverConfigFiles(targetPath); | ||
| const rules = getBuiltinRules(); | ||
| const rules = [...getBuiltinRules(), ...(options.extraRules ?? [])]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Type the merged rules list as Line 35 currently infers a mutable array type. Keep the scanner’s effective rule set immutable at the type level. Suggested fix- const rules = [...getBuiltinRules(), ...(options.extraRules ?? [])];
+ const rules: ReadonlyArray<Rule> = [...getBuiltinRules(), ...(options.extraRules ?? [])];As per coding guidelines, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| const findings = runRules(target.files, rules, target.path); | ||
| const skillHealth = analyzeSkillHealth(target.files); | ||
| const harnessAdapters = detectHarnessAdapters(targetPath); | ||
|
|
||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document that external regexes are normalized to global matching.
The docs describe pattern execution but omit that the loader enforces global matching, which affects match cardinality and expected findings.
Suggested wording tweak
📝 Committable suggestion
🤖 Prompt for AI Agents