diff --git a/src/skillspector/nodes/analyzers/mcp_least_privilege.py b/src/skillspector/nodes/analyzers/mcp_least_privilege.py index a79ee0d..73d5745 100644 --- a/src/skillspector/nodes/analyzers/mcp_least_privilege.py +++ b/src/skillspector/nodes/analyzers/mcp_least_privilege.py @@ -125,6 +125,19 @@ def _is_test_file(path: str) -> bool: return name.startswith("test_") or stem.endswith("_test") +def _normalize_allowed_tools(value: object) -> list[str]: + """Coerce a manifest ``allowed-tools`` value into a list of tool names. + + Accepts the list form (``[Bash, Read]``) and the comma-separated string + form (``"Bash, Read"``). Anything else yields an empty list. + """ + if isinstance(value, list): + return [str(t).strip() for t in value if str(t).strip()] + if isinstance(value, str): + return [t.strip() for t in value.split(",") if t.strip()] + return [] + + def _detect_capabilities(content: str) -> set[str]: """Return set of capability categories found in *content*.""" found: set[str] = set() @@ -189,6 +202,9 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: else: permissions = None # treat missing or non-list as None + # `allowed-tools` (Agent Skills standard) is also a permission declaration. + allowed_tools = _normalize_allowed_tools(manifest.get("allowed-tools")) + # --- LP2: Wildcard permission --- if isinstance(permissions, list) and _has_wildcard(permissions): logger.debug("%s: LP2 wildcard permission detected", ANALYZER_ID) @@ -232,8 +248,8 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: for caps in file_capabilities.values(): all_caps.update(caps) - # LP3: emit when permissions is None or empty list AND capabilities detected - permissions_absent = permissions is None or permissions == [] + # LP3: no declaration via `permissions` or `allowed-tools`, yet caps detected. + permissions_absent = (permissions is None or permissions == []) and not allowed_tools if permissions_absent and all_caps: logger.debug("%s: LP3 no permissions declared but capabilities detected", ANALYZER_ID) cap_names = ", ".join(sorted(all_caps)) diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 694a048..4b9705a 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -164,8 +164,8 @@ def _read_file_cache(skill_dir: Path, components: list[str]) -> dict[str, str]: def _parse_manifest(skill_dir: Path) -> dict[str, object]: """Parse SKILL.md or skill.md YAML frontmatter into a manifest dict. - Returns dict with name, description, triggers (list), permissions (list). - Returns {} if no file or parse fails. + Returns dict with name, description, triggers (list), permissions (list), + allowed-tools (list), parameters (list). Returns {} if no file or parse fails. """ for name in ("SKILL.md", "skill.md"): path = skill_dir / name @@ -200,6 +200,14 @@ def _parse_manifest(skill_dir: Path) -> dict[str, object]: manifest["permissions"] = ( [str(p) for p in permissions] if isinstance(permissions, list) else [] ) + # `allowed-tools` (Agent Skills standard) — accept list or comma string. + allowed_tools = data.get("allowed-tools", []) + if isinstance(allowed_tools, list): + manifest["allowed-tools"] = [str(t).strip() for t in allowed_tools if str(t).strip()] + elif isinstance(allowed_tools, str): + manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split(",") if t.strip()] + else: + manifest["allowed-tools"] = [] # Preserve parameter definitions as dicts so the MCP tool-poisoning # analyzer (TP1/TP2/TP3 parameter checks) can inspect them. Without # this, those checks never fire on real scans because the manifest diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index a0fc16b..7ff92e3 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -70,6 +70,7 @@ def test_build_context_real_directory_with_skill_md(tmp_path: Path) -> None: "description": "For tests", "triggers": ["a", "b"], "permissions": ["read"], + "allowed-tools": [], "parameters": [], } assert result["ast_cache"] == {} @@ -211,3 +212,36 @@ def test_build_context_parses_parameters_from_frontmatter(tmp_path: Path) -> Non assert result["manifest"]["parameters"] == [ {"name": "path", "description": "file path to read"} ] + + +def test_build_context_parses_allowed_tools_list(tmp_path: Path) -> None: + """`allowed-tools` list form is preserved so LP3 treats it as a declaration.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: [Bash, Read]\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash", "Read"] + + +def test_build_context_allowed_tools_malformed_value(tmp_path: Path) -> None: + """A non-list, non-string `allowed-tools` value normalizes to an empty list.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: 42\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == [] + + +def test_build_context_parses_allowed_tools_comma_string(tmp_path: Path) -> None: + """`allowed-tools` comma-separated string form is normalized to a list.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash, Read\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash", "Read"] diff --git a/tests/test_mcp_least_privilege.py b/tests/test_mcp_least_privilege.py index 9e7852e..e134fca 100644 --- a/tests/test_mcp_least_privilege.py +++ b/tests/test_mcp_least_privilege.py @@ -258,6 +258,32 @@ def test_no_permissions_field(self): assert lp3.file == "SKILL.md" +class TestLP3AllowedTools: + def test_allowed_tools_list_no_lp3(self): + """allowed-tools list form ([Bash, Read]) is a declaration → no LP3.""" + state = _make_state("mcp_underdeclared_skill") + state["manifest"]["permissions"] = None + state["manifest"]["allowed-tools"] = ["Bash", "Read"] + result = mcp_least_privilege.node(state) + findings = result["findings"] + lp3_findings = [f for f in findings if f.rule_id == "LP3"] + assert lp3_findings == [], ( + f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}" + ) + + def test_allowed_tools_comma_string_no_lp3(self): + """allowed-tools comma-string form ('Bash, Read') is also a declaration → no LP3.""" + state = _make_state("mcp_underdeclared_skill") + state["manifest"]["permissions"] = None + state["manifest"]["allowed-tools"] = "Bash, Read" + result = mcp_least_privilege.node(state) + findings = result["findings"] + lp3_findings = [f for f in findings if f.rule_id == "LP3"] + assert lp3_findings == [], ( + f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}" + ) + + class TestLP4OverDeclared: def test_over_declared_detected(self): """mcp_overprivileged_skill declares bash/network/write but code only reads → LP4 for network and write."""