diff --git a/CHANGELOG.md b/CHANGELOG.md index 34c673f..9f93d6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- `--exit-code` no longer fails a passing scan because of an informational note. + Scanning a secure `https://`/`wss://` endpoint with `--exit-code` printed + `PASSED` but exited `1`, because the run counted the INFO `URL_SCAN_LIMITED` + note as a "finding" — breaking the documented CI gate on the advertised + URL-scan path and contradicting the just-landed #27 fix. `--exit-code` now + fails only on *actionable* findings (severity above INFO); INFO notes never + fail the gate. The exit-code decision was extracted into a pure, unit-tested + `shouldFailExit()` helper (mirroring the `args.ts`/`cli.ts` split). - URL/endpoint mode no longer produces false findings (#27). Scanning a URL previously ran every config rule against an effectively empty config, so any endpoint always emitted a CRITICAL `NO_AUTH` and a MEDIUM `MISSING_RATE_LIMIT` diff --git a/src/__tests__/exit-policy.test.ts b/src/__tests__/exit-policy.test.ts new file mode 100644 index 0000000..70cbf56 --- /dev/null +++ b/src/__tests__/exit-policy.test.ts @@ -0,0 +1,69 @@ +import { shouldFailExit } from '../exit-policy'; +import { Finding, RuleId, Severity, SecurityReport } from '../types'; + +function makeFinding(ruleId: RuleId, severity: Severity): Finding { + return { + ruleId, + severity, + title: 'Test', + description: 'Test finding', + remediation: 'Fix it', + }; +} + +function makeReport(partial: Partial): SecurityReport { + return { + findings: [], + score: 0, + passed: true, + summary: { critical: 0, high: 0, medium: 0, low: 0, info: 0 }, + scannedAt: '2026-01-01T00:00:00.000Z', + durationMs: 0, + ...partial, + }; +} + +describe('shouldFailExit()', () => { + it('fails whenever the report did not pass, regardless of --exit-code', () => { + const report = makeReport({ passed: false }); + expect(shouldFailExit(report, false)).toBe(true); + expect(shouldFailExit(report, true)).toBe(true); + }); + + it('passes a clean report when --exit-code is not set', () => { + const report = makeReport({ passed: true, findings: [] }); + expect(shouldFailExit(report, false)).toBe(false); + }); + + it('fails a passing report with --exit-code when an actionable finding exists', () => { + const report = makeReport({ + passed: true, + findings: [makeFinding(RuleId.MISSING_RATE_LIMIT, Severity.MEDIUM)], + }); + expect(shouldFailExit(report, true)).toBe(true); + }); + + // Regression (secure URL scan + --exit-code): scanning a secure https:// / + // wss:// endpoint produces only the INFO URL_SCAN_LIMITED note. The report + // PASSES, so --exit-code must NOT fail solely because of that informational + // note — otherwise the documented CI gate reports PASSED yet exits 1. + it('does not fail a passing URL scan whose only finding is the INFO note', () => { + const report = makeReport({ + passed: true, + findings: [makeFinding(RuleId.URL_SCAN_LIMITED, Severity.INFO)], + }); + expect(shouldFailExit(report, true)).toBe(false); + expect(shouldFailExit(report, false)).toBe(false); + }); + + it('still fails when an INFO note accompanies a real finding under --exit-code', () => { + const report = makeReport({ + passed: true, + findings: [ + makeFinding(RuleId.URL_SCAN_LIMITED, Severity.INFO), + makeFinding(RuleId.DEBUG_MODE_ENABLED, Severity.LOW), + ], + }); + expect(shouldFailExit(report, true)).toBe(true); + }); +}); diff --git a/src/cli.ts b/src/cli.ts index 86283dd..b26e9fe 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -18,6 +18,7 @@ import { scan } from './scanner.js'; import { toSarif } from './sarif.js'; import { Severity, Finding, RuleId } from './types.js'; import { parseArgs } from './args.js'; +import { shouldFailExit } from './exit-policy.js'; /** Selectable rule IDs (excludes the URL_SCAN_LIMITED informational note). */ const VALID_RULE_IDS = Object.values(RuleId).filter( @@ -136,11 +137,7 @@ async function main(): Promise { printTable(report.findings, report.passed); } - const shouldFail = - !report.passed || - (exitCode && report.findings.length > 0); - - if (shouldFail) { + if (shouldFailExit(report, exitCode)) { process.exit(1); } } catch (err) { diff --git a/src/exit-policy.ts b/src/exit-policy.ts new file mode 100644 index 0000000..d6532f0 --- /dev/null +++ b/src/exit-policy.ts @@ -0,0 +1,34 @@ +/** + * Pure exit-code policy for the mcp-security-scanner CLI. + * + * Kept free of side effects (no process.exit / console calls) so it can be + * unit-tested directly, separate from the executable entry point in cli.ts — + * mirroring the split between args.ts and cli.ts. + */ + +import { SecurityReport, Severity } from './types.js'; + +/** + * Decide whether the CLI should exit non-zero for a completed scan. + * + * The process fails when either: + * - the report did not pass the gate (`report.passed === false`), or + * - `--exit-code` was given AND the scan produced at least one *actionable* + * finding. + * + * INFO-severity entries are informational, not vulnerabilities, and must never + * fail a gate. The `URL_SCAN_LIMITED` note is the concrete case: scanning a + * secure `https://` / `wss://` endpoint produces only that INFO note, so the + * report PASSES — but a naive "any finding" check would count the note and exit + * 1, contradicting the PASSED result and breaking the documented CI gate on the + * advertised URL-scan path. + * + * @param report - The completed scan report. + * @param exitCode - Whether the `--exit-code` flag was supplied. + * @returns true if the process should exit with a non-zero status. + */ +export function shouldFailExit(report: SecurityReport, exitCode: boolean): boolean { + if (!report.passed) return true; + if (!exitCode) return false; + return report.findings.some((f) => f.severity !== Severity.INFO); +}