diff --git a/CHANGELOG.md b/CHANGELOG.md index 34c673f..d1dd6ff 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 +- `OVERPRIVILEGED_TOOL`: now flags a `shell:*` namespace wildcard, not just the + exact `shell:exec` permission. A tool granted `shell:*` — a strict superset of + `shell:exec` and full arbitrary command execution on the host — previously + produced zero findings and passed the gate, while the same tool with + `shell:exec` was correctly flagged HIGH. The rule now treats a dangerous + namespace's wildcard as at least as dangerous as its specific permission. + Wildcards owned by a dedicated rule (`filesystem:*` → `UNRESTRICTED_FILE_ACCESS`) + are not double-counted. - 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__/scanner.test.ts b/src/__tests__/scanner.test.ts index dd8e44a..169e730 100644 --- a/src/__tests__/scanner.test.ts +++ b/src/__tests__/scanner.test.ts @@ -288,6 +288,24 @@ describe('OVERPRIVILEGED_TOOL rule', () => { expect(findings).toHaveLength(1); }); + it('fires for a tool with a shell:* namespace wildcard (superset of shell:exec)', () => { + const config: ParsedMcpConfig = { + tools: [{ name: 'runner', permissions: ['shell:*'] }], + }; + const findings = runConfigRule(RuleId.OVERPRIVILEGED_TOOL, config); + expect(findings).toHaveLength(1); + expect(findings[0].ruleId).toBe(RuleId.OVERPRIVILEGED_TOOL); + expect(findings[0].evidence).toContain('shell:*'); + }); + + it('does not double-count filesystem:* (owned by UNRESTRICTED_FILE_ACCESS)', () => { + const config: ParsedMcpConfig = { + tools: [{ name: 'fs', permissions: ['filesystem:*'] }], + }; + const findings = runConfigRule(RuleId.OVERPRIVILEGED_TOOL, config); + expect(findings).toHaveLength(0); + }); + it('does not fire for safe permissions', () => { const config: ParsedMcpConfig = { tools: [{ name: 'safe-tool', permissions: ['filesystem:read'] }], @@ -295,6 +313,14 @@ describe('OVERPRIVILEGED_TOOL rule', () => { const findings = runConfigRule(RuleId.OVERPRIVILEGED_TOOL, config); expect(findings).toHaveLength(0); }); + + it('does not fire for a non-dangerous namespace wildcard (e.g. logging:*)', () => { + const config: ParsedMcpConfig = { + tools: [{ name: 'logger', permissions: ['logging:*'] }], + }; + const findings = runConfigRule(RuleId.OVERPRIVILEGED_TOOL, config); + expect(findings).toHaveLength(0); + }); }); // ─── TOOL_DESC_INJECTION ────────────────────────────────────────────────────── diff --git a/src/rules/config-rules.ts b/src/rules/config-rules.ts index 8d9c11f..f277fdb 100644 --- a/src/rules/config-rules.ts +++ b/src/rules/config-rules.ts @@ -4,6 +4,34 @@ import { Rule } from './index.js'; const DANGEROUS_PERMISSIONS = ['filesystem:write', 'network:*', 'shell:exec']; +/** + * Namespaces whose wildcard (`ns:*`) is already reported by a dedicated rule, + * so OVERPRIVILEGED_TOOL should not also flag it (avoids double-counting the + * same issue). `filesystem:*` is owned by UNRESTRICTED_FILE_ACCESS. + */ +const NAMESPACES_WITH_DEDICATED_RULE = new Set(['filesystem']); + +/** + * True when a permission is dangerous. A permission matches when it is listed + * explicitly in DANGEROUS_PERMISSIONS, or when it is a namespace wildcard + * (`shell:*`) that subsumes a dangerous permission in that namespace — a + * wildcard grants everything the specific permission does, so it is at least as + * dangerous. This closes the gap where `shell:*` (a superset of `shell:exec`) + * slipped past the exact-match check and left a tool with full shell access + * completely unflagged. Wildcards for namespaces with their own dedicated rule + * (e.g. `filesystem:*` → UNRESTRICTED_FILE_ACCESS) are skipped here. + */ +function isDangerousPermission(permission: string): boolean { + if (DANGEROUS_PERMISSIONS.includes(permission)) return true; + const wildcardMatch = permission.match(/^([^:]+):\*$/); + if (wildcardMatch) { + const namespace = wildcardMatch[1]; + if (NAMESPACES_WITH_DEDICATED_RULE.has(namespace)) return false; + return DANGEROUS_PERMISSIONS.some((p) => p.startsWith(namespace + ':')); + } + return false; +} + export const configRules: Rule[] = [ { id: RuleId.WILDCARD_CORS, @@ -68,9 +96,7 @@ export const configRules: Rule[] = [ const findings: Finding[] = []; for (const tool of config.tools ?? []) { if (!tool.permissions || tool.permissions.length === 0) continue; - const matched = tool.permissions.filter((p) => - DANGEROUS_PERMISSIONS.includes(p) - ); + const matched = tool.permissions.filter(isDangerousPermission); if (matched.length > 0) { findings.push({ ruleId: RuleId.OVERPRIVILEGED_TOOL,