Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/skillspector/nodes/analyzers/mcp_least_privilege.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,16 @@ def _is_test_file(path: str) -> bool:
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.
Accepts the list form (``[Bash, Read]``), the comma-separated string
form (``"Bash, Read"``), and the space-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()]
if "," in value:
return [t.strip() for t in value.split(",") if t.strip()]
return [t.strip() for t in value.split() if t.strip()]
return []


Expand Down
7 changes: 5 additions & 2 deletions src/skillspector/nodes/build_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,12 +289,15 @@ 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` (Agent Skills standard) — accept list, comma string, or space-separated 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()]
if "," in allowed_tools:
manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split(",") if t.strip()]
else:
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
Expand Down
33 changes: 33 additions & 0 deletions tests/nodes/test_build_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,39 @@ def test_build_context_parses_allowed_tools_comma_string(tmp_path: Path) -> None
assert result["manifest"]["allowed-tools"] == ["Bash", "Read"]


def test_build_context_parses_allowed_tools_space_string(tmp_path: Path) -> None:
"""`allowed-tools` space-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"]


def test_build_context_parses_allowed_tools_mixed_whitespace(tmp_path: Path) -> None:
"""`allowed-tools` string with mixed whitespace (multiple spaces) is normalized."""
(tmp_path / "SKILL.md").write_text(
"---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash Read Write\n---\n",
encoding="utf-8",
)
state: SkillspectorState = {"skill_path": str(tmp_path)}
result = build_context(state)
assert result["manifest"]["allowed-tools"] == ["Bash", "Read", "Write"]


def test_build_context_parses_allowed_tools_single_space_string(tmp_path: Path) -> None:
"""`allowed-tools` single tool as space-separated string yields one item."""
(tmp_path / "SKILL.md").write_text(
"---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash\n---\n",
encoding="utf-8",
)
state: SkillspectorState = {"skill_path": str(tmp_path)}
result = build_context(state)
assert result["manifest"]["allowed-tools"] == ["Bash"]


def test_build_context_reports_exclusion_boundary_without_descendants(tmp_path: Path) -> None:
"""Excluded directory trees produce one boundary record, not child records."""
(tmp_path / "SKILL.md").write_text("# Skill\n", encoding="utf-8")
Expand Down
12 changes: 12 additions & 0 deletions tests/test_mcp_least_privilege.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,18 @@ def test_allowed_tools_comma_string_no_lp3(self):
f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}"
)

def test_allowed_tools_space_string_no_lp3(self):
"""allowed-tools space-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]}"
)

def test_allowed_tools_underdeclared_fires_lp1(self):
"""allowed-tools: [Read] + Bash code → LP3 suppressed but LP1 fires HIGH for shell."""
state = _make_state("mcp_underdeclared_skill")
Expand Down