From 9efb9dd4f47a27018950be047b8f003e38e41f54 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 9 Jun 2026 22:26:07 +0000 Subject: [PATCH] fix(cli): validate --rule and reject unknown rule IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unknown or mistyped --rule value (e.g. NOAUTH instead of NO_AUTH) was cast straight to RuleId and passed to scan(). Because the scanner filters the rule set to the requested IDs, a value matching no real rule filtered it down to nothing: the scan produced zero findings, a perfect score, passed:true, and exit code 0 — silently turning a CI security gate green. Validate each --rule value against the RuleId enum and exit 2 on an unknown value, mirroring how --format and --fail-on already behave. Also list the valid rule IDs in --help so they are discoverable. Argument parsing is extracted into a side-effect-free src/args.ts module (no console output, no process.exit, no top-level execution) so it can be unit-tested directly; cli.ts now consumes a discriminated ParseResult and owns all I/O and exit codes. Adds src/__tests__/args.test.ts covering the unknown-rule regression plus format/fail-on/target/help parsing. Closes #11 --- src/__tests__/args.test.ts | 142 +++++++++++++++++++++++++++++++++++++ src/args.ts | 126 ++++++++++++++++++++++++++++++++ src/cli.ts | 74 ++++--------------- 3 files changed, 282 insertions(+), 60 deletions(-) create mode 100644 src/__tests__/args.test.ts create mode 100644 src/args.ts diff --git a/src/__tests__/args.test.ts b/src/__tests__/args.test.ts new file mode 100644 index 0000000..d17856b --- /dev/null +++ b/src/__tests__/args.test.ts @@ -0,0 +1,142 @@ +import { parseArgs } from '../args'; +import { RuleId, Severity } from '../types'; + +// ─── parseArgs() unit tests ────────────────────────────────────────────────── + +describe('parseArgs', () => { + describe('--rule validation (regression for issue #11)', () => { + it('rejects an unknown/typo’d rule ID instead of silently passing', () => { + // NO_AUTH mistyped as NOAUTH previously filtered the rule set to nothing, + // producing a perfect score + passed:true + exit 0 (a false security gate). + const result = parseArgs(['--rule=NOAUTH', './config.json']); + expect(result.kind).toBe('error'); + if (result.kind === 'error') { + expect(result.message).toContain('Unknown rule "NOAUTH"'); + expect(result.showHelp).toBe(false); + } + }); + + it('accepts a valid rule ID', () => { + const result = parseArgs(['--rule=NO_AUTH', './config.json']); + expect(result.kind).toBe('run'); + if (result.kind === 'run') { + expect(result.scanConfig.rules).toEqual([RuleId.NO_AUTH]); + } + }); + + it('accepts multiple valid rule IDs', () => { + const result = parseArgs([ + '--rule=NO_AUTH', + '--rule=MISSING_TLS', + './config.json', + ]); + expect(result.kind).toBe('run'); + if (result.kind === 'run') { + expect(result.scanConfig.rules).toEqual([ + RuleId.NO_AUTH, + RuleId.MISSING_TLS, + ]); + } + }); + + it('rejects the batch if any rule ID is invalid', () => { + const result = parseArgs([ + '--rule=NO_AUTH', + '--rule=BOGUS', + './config.json', + ]); + expect(result.kind).toBe('error'); + if (result.kind === 'error') { + expect(result.message).toContain('Unknown rule "BOGUS"'); + } + }); + }); + + describe('target resolution', () => { + it('treats a file path as configPath', () => { + const result = parseArgs(['./mcp-config.json']); + expect(result.kind).toBe('run'); + if (result.kind === 'run') { + expect(result.isUrl).toBe(false); + expect(result.scanConfig.configPath).toBe('./mcp-config.json'); + expect(result.scanConfig.serverUrl).toBeUndefined(); + } + }); + + it('treats an http(s)/ws(s) URL as serverUrl', () => { + for (const url of [ + 'https://mcp.example.com', + 'http://localhost:3000', + 'wss://mcp.example.com', + ]) { + const result = parseArgs([url]); + expect(result.kind).toBe('run'); + if (result.kind === 'run') { + expect(result.isUrl).toBe(true); + expect(result.scanConfig.serverUrl).toBe(url); + expect(result.scanConfig.configPath).toBeUndefined(); + } + } + }); + + it('errors with showHelp when no target is provided', () => { + const result = parseArgs(['--format=table']); + expect(result.kind).toBe('error'); + if (result.kind === 'error') { + expect(result.message).toContain('No config path or URL provided'); + expect(result.showHelp).toBe(true); + } + }); + }); + + describe('--format / --fail-on / --exit-code', () => { + it('defaults format to json', () => { + const result = parseArgs(['./config.json']); + if (result.kind === 'run') expect(result.format).toBe('json'); + }); + + it('accepts sarif and table formats', () => { + expect((parseArgs(['--format=sarif', './c.json']) as { format: string }).format).toBe('sarif'); + expect((parseArgs(['--format=table', './c.json']) as { format: string }).format).toBe('table'); + }); + + it('rejects an unknown format', () => { + const result = parseArgs(['--format=xml', './config.json']); + expect(result.kind).toBe('error'); + if (result.kind === 'error') expect(result.message).toContain('Unknown format "xml"'); + }); + + it('maps --fail-on to a Severity (case-insensitive)', () => { + const result = parseArgs(['--fail-on=HIGH', './config.json']); + if (result.kind === 'run') expect(result.scanConfig.failOn).toBe(Severity.HIGH); + }); + + it('rejects an unknown --fail-on severity', () => { + const result = parseArgs(['--fail-on=urgent', './config.json']); + expect(result.kind).toBe('error'); + if (result.kind === 'error') expect(result.message).toContain('Unknown severity "urgent"'); + }); + + it('sets exitCode when --exit-code is present', () => { + const result = parseArgs(['--exit-code', './config.json']); + if (result.kind === 'run') expect(result.exitCode).toBe(true); + }); + }); + + describe('help and unknown flags', () => { + it('returns help with exit code 2 when no args are given', () => { + expect(parseArgs([])).toEqual({ kind: 'help', exitCode: 2 }); + }); + + it('returns help with exit code 0 for --help / -h', () => { + expect(parseArgs(['--help'])).toEqual({ kind: 'help', exitCode: 0 }); + expect(parseArgs(['-h'])).toEqual({ kind: 'help', exitCode: 0 }); + }); + + it('rejects an unknown flag', () => { + const result = parseArgs(['--nope', './config.json']); + expect(result.kind).toBe('error'); + if (result.kind === 'error') expect(result.message).toContain('Unknown flag "--nope"'); + }); + }); +}); diff --git a/src/args.ts b/src/args.ts new file mode 100644 index 0000000..fa85c32 --- /dev/null +++ b/src/args.ts @@ -0,0 +1,126 @@ +/** + * CLI argument parsing for mcp-security-scanner. + * + * Kept free of side effects (no console output, no process.exit, no top-level + * execution) so it can be unit-tested directly. `cli.ts` consumes the result + * and is responsible for all I/O and exit codes. + */ + +import { ScanConfig, Severity, RuleId } from './types.js'; + +export type OutputFormat = 'json' | 'sarif' | 'table'; + +/** + * Result of parsing argv. A discriminated union so the caller can decide how + * to render output and which exit code to use. + */ +export type ParseResult = + | { kind: 'help'; exitCode: 0 | 2 } + | { kind: 'error'; message: string; showHelp: boolean } + | { + kind: 'run'; + scanConfig: ScanConfig; + format: OutputFormat; + exitCode: boolean; + target: string; + isUrl: boolean; + }; + +/** All valid rule identifiers, for validating the `--rule` flag. */ +export const VALID_RULE_IDS: ReadonlySet = new Set( + Object.values(RuleId) +); + +const SEVERITY_MAP: Record = { + critical: Severity.CRITICAL, + high: Severity.HIGH, + medium: Severity.MEDIUM, + low: Severity.LOW, + info: Severity.INFO, +}; + +function isUrlTarget(target: string): boolean { + return ( + target.startsWith('http://') || + target.startsWith('https://') || + target.startsWith('ws://') || + target.startsWith('wss://') + ); +} + +/** + * Parse CLI arguments (everything after `node cli.js`). + * + * Validates `--format`, `--fail-on`, and `--rule` and surfaces unknown values + * as `error` results rather than silently ignoring them — an invalid value + * must never produce a passing scan. + */ +export function parseArgs(args: string[]): ParseResult { + if (args.length === 0) return { kind: 'help', exitCode: 2 }; + if (args.includes('--help') || args.includes('-h')) { + return { kind: 'help', exitCode: 0 }; + } + + let format: OutputFormat = 'json'; + let exitCode = false; + let failOn: Severity | undefined; + const rules: RuleId[] = []; + let target: string | undefined; + + for (const arg of args) { + if (arg.startsWith('--format=')) { + const val = arg.slice('--format='.length); + if (val === 'json' || val === 'sarif' || val === 'table') { + format = val; + } else { + return { + kind: 'error', + message: `Unknown format "${val}". Expected json, sarif, or table.`, + showHelp: false, + }; + } + } else if (arg === '--exit-code') { + exitCode = true; + } else if (arg.startsWith('--fail-on=')) { + const val = arg.slice('--fail-on='.length).toLowerCase(); + if (val in SEVERITY_MAP) { + failOn = SEVERITY_MAP[val]; + } else { + return { + kind: 'error', + message: `Unknown severity "${val}". Expected critical, high, medium, low, or info.`, + showHelp: false, + }; + } + } else if (arg.startsWith('--rule=')) { + const ruleId = arg.slice('--rule='.length); + if (!VALID_RULE_IDS.has(ruleId)) { + return { + kind: 'error', + message: `Unknown rule "${ruleId}". Use --help to see valid rule IDs.`, + showHelp: false, + }; + } + rules.push(ruleId as RuleId); + } else if (!arg.startsWith('--')) { + target = arg; + } else { + return { + kind: 'error', + message: `Unknown flag "${arg}". Use --help to see available options.`, + showHelp: false, + }; + } + } + + if (!target) { + return { kind: 'error', message: 'No config path or URL provided.', showHelp: true }; + } + + const isUrl = isUrlTarget(target); + const scanConfig: ScanConfig = isUrl ? { serverUrl: target } : { configPath: target }; + if (failOn !== undefined) scanConfig.failOn = failOn; + if (rules.length > 0) scanConfig.rules = rules; + + return { kind: 'run', scanConfig, format, exitCode, target, isUrl }; +} diff --git a/src/cli.ts b/src/cli.ts index 576118a..ffee75e 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -15,7 +15,8 @@ import { scan } from './scanner.js'; import { toSarif } from './sarif.js'; -import { ScanConfig, Severity, RuleId, Finding } from './types.js'; +import { Severity, RuleId, Finding } from './types.js'; +import { parseArgs } from './args.js'; const SEVERITY_ORDER: Record = { critical: 4, @@ -36,6 +37,9 @@ function printHelp(): void { console.log(' --rule=RULE_ID Run only the specified rule (repeatable)'); console.log(' --help, -h Show this help message'); console.log(''); + console.log('Valid rule IDs:'); + console.log(' ' + Object.values(RuleId).join(', ')); + console.log(''); console.log('Examples:'); console.log(' mcp-security-scanner ./mcp-server.json'); console.log(' mcp-security-scanner --format=table ./mcp-server.json'); @@ -93,73 +97,23 @@ function printTable(findings: Finding[], passed: boolean): void { } async function main(): Promise { - const args = process.argv.slice(2); + const parsed = parseArgs(process.argv.slice(2)); - if (args.length === 0 || args.includes('--help') || args.includes('-h')) { + if (parsed.kind === 'help') { printHelp(); - if (args.length === 0) process.exit(2); - return; + process.exit(parsed.exitCode); } - let format: 'json' | 'sarif' | 'table' = 'json'; - let exitCode = false; - let failOn: Severity | undefined; - const rules: RuleId[] = []; - let target: string | undefined; - - for (const arg of args) { - if (arg.startsWith('--format=')) { - const val = arg.slice('--format='.length); - if (val === 'json' || val === 'sarif' || val === 'table') { - format = val; - } else { - console.error(`Error: Unknown format "${val}". Expected json, sarif, or table.`); - process.exit(2); - } - } else if (arg === '--exit-code') { - exitCode = true; - } else if (arg.startsWith('--fail-on=')) { - const val = arg.slice('--fail-on='.length).toLowerCase(); - const severityMap: Record = { - critical: Severity.CRITICAL, - high: Severity.HIGH, - medium: Severity.MEDIUM, - low: Severity.LOW, - info: Severity.INFO, - }; - if (val in severityMap) { - failOn = severityMap[val]; - } else { - console.error(`Error: Unknown severity "${val}". Expected critical, high, medium, low, or info.`); - process.exit(2); - } - } else if (arg.startsWith('--rule=')) { - const ruleId = arg.slice('--rule='.length) as RuleId; - rules.push(ruleId); - } else if (!arg.startsWith('--')) { - target = arg; - } else { - console.error(`Error: Unknown flag "${arg}". Use --help to see available options.`); - process.exit(2); + if (parsed.kind === 'error') { + console.error(`Error: ${parsed.message}`); + if (parsed.showHelp) { + console.error(''); + printHelp(); } - } - - if (!target) { - console.error('Error: No config path or URL provided.'); - console.error(''); - printHelp(); process.exit(2); } - const isUrl = target.startsWith('http://') || target.startsWith('https://') || - target.startsWith('ws://') || target.startsWith('wss://'); - - const scanConfig: ScanConfig = isUrl - ? { serverUrl: target } - : { configPath: target }; - - if (failOn !== undefined) scanConfig.failOn = failOn; - if (rules.length > 0) scanConfig.rules = rules; + const { scanConfig, format, exitCode, target, isUrl } = parsed; try { const report = await scan(scanConfig);