diff --git a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py index 2bfd4703..1bddfd4d 100644 --- a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py +++ b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py @@ -21,7 +21,12 @@ from ..._errors import CLIConnectionError, CLINotFoundError, ProcessError from ..._errors import CLIJSONDecodeError as SDKJSONDecodeError from ..._version import __version__ -from ...types import ClaudeAgentOptions, SystemPromptFile, SystemPromptPreset +from ...types import ( + _SKILLS_ALL, + ClaudeAgentOptions, + SystemPromptFile, + SystemPromptPreset, +) from .._task_compat import TaskHandle, spawn_detached from . import Transport @@ -55,6 +60,98 @@ def _kill_active_children() -> None: atexit.register(_kill_active_children) +# Parentheses and commas are delimiters to the --allowedTools tokenizer; +# control characters (C0, DEL, C1) never appear in a skill directory name. +# U+FEFF is here rather than with the whitespace check below because the +# CLI trims it as whitespace and Python's str.strip() does not. +_SKILL_NAME_INVALID_CHARS = re.compile(r"[(),\x00-\x1f\x7f-\x9f\ufeff]") + +# Every surrogate in a Python str is unpaired by construction: a well-formed +# astral character is a single code point, not a pair. +_SURROGATE_RE = re.compile("[\ud800-\udfff]") + + +def _reject_non_list_skills(skills: object) -> None: + """Reject values other than a list or "all". + + A string iterates as characters, and any other iterable builds rules + here but is dropped from the initialize request, which installs no + skill filter at all. + """ + if isinstance(skills, list) or skills == _SKILLS_ALL: + return + suggestion = f" Did you mean [{skills!r}]?" if isinstance(skills, str) else "" + raise TypeError( + "ClaudeAgentOptions.skills must be a list of skill names or" + f' "all", got {skills!r}.{suggestion}' + ) + + +def _validate_skill_name(name: str) -> None: + """Reject skill names that cannot ride safely in a ``Skill(name)`` rule. + + Names from ``options.skills`` are formatted into the ``--allowedTools`` + value, which the CLI splits into rules on commas and spaces outside + parentheses. That tokenizer does not honor escape sequences -- escaping + exists only in the per-rule grammar, applied after splitting -- so a + name carrying a delimiter cannot be passed through reliably: what it + tokenizes into depends on what surrounds it. + + Names that tokenize cleanly but can never match the listed skill are + rejected too, so a dead rule fails loudly here instead of silently + granting nothing. Each check below states its own reason. + """ + if not isinstance(name, str): + raise TypeError( + f"Skill names must be strings, got {type(name).__name__}: {name!r}" + ) + if not name.strip(): + raise ValueError("Skill names must be non-empty strings") + if _SURROGATE_RE.search(name): + raise ValueError( + f"Invalid skill name {name!r}: contains a surrogate code point," + " which can never match a skill the CLI discovered." + ) + if name != name.strip(): + raise ValueError( + f"Invalid skill name {name!r}: leading or trailing whitespace" + " can never match — the Skill tool trims the invoked name." + ) + if _SKILL_NAME_INVALID_CHARS.search(name): + raise ValueError( + f"Invalid skill name {name!r}: parentheses, commas, control" + " characters, and byte-order marks are not allowed. Names match" + " the skill's directory name, or 'plugin:skill' for" + " plugin-qualified skills." + ) + if name == "*": + raise ValueError( + "Invalid skill name '*': use skills=\"all\" to enable every skill." + ) + if name.endswith(":*") or name.endswith(" *"): + raise ValueError( + f"Invalid skill name {name!r}: wildcard-suffix names are not" + " allowed; list each skill by its exact name." + ) + if name.startswith("/"): + raise ValueError( + f"Invalid skill name {name!r}: skill names may not start with" + " '/'. The skills option takes the canonical name, not the" + " slash-command form." + ) + if "\\\\" in name: + raise ValueError( + f"Invalid skill name {name!r}: consecutive backslashes are not" + " allowed — the per-rule parser collapses them, so the rule" + " would name a different skill." + ) + if name.endswith("\\"): + raise ValueError( + f"Invalid skill name {name!r}: names may not end with an" + " unpaired backslash." + ) + + class _LineFramer: """Reassembles complete lines from a stream that yields arbitrary chunks. @@ -429,6 +526,9 @@ def _apply_skills_defaults( unset so the CLI discovers installed skills without the caller having to wire up both options manually. ``None`` is a no-op. + Each listed skill name is validated before being formatted into a + rule; see :func:`_validate_skill_name`. + Does not mutate the original options object. """ allowed_tools: list[str] = list(self._options.allowed_tools) @@ -441,12 +541,14 @@ def _apply_skills_defaults( skills = self._options.skills if skills is None: return allowed_tools, setting_sources + _reject_non_list_skills(skills) - if skills == "all": + if skills == _SKILLS_ALL: if "Skill" not in allowed_tools: allowed_tools.append("Skill") else: for name in skills: + _validate_skill_name(name) pattern = f"Skill({name})" if pattern not in allowed_tools: allowed_tools.append(pattern) diff --git a/src/claude_agent_sdk/types.py b/src/claude_agent_sdk/types.py index c5fb8689..32bfbb71 100644 --- a/src/claude_agent_sdk/types.py +++ b/src/claude_agent_sdk/types.py @@ -5,7 +5,7 @@ from collections.abc import Awaitable, Callable from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING, Any, Literal, Protocol, TypeAlias +from typing import TYPE_CHECKING, Any, Final, Literal, Protocol, TypeAlias if sys.version_info >= (3, 11): from typing import NotRequired, Required, TypedDict @@ -31,6 +31,9 @@ # Agent definitions SettingSource = Literal["user", "project", "local"] + +# The ClaudeAgentOptions.skills value meaning "every discovered skill". +_SKILLS_ALL: Final = "all" EffortLevel: TypeAlias = Literal["low", "medium", "high", "xhigh", "max"] @@ -1745,7 +1748,7 @@ def _warn_if_can_use_tool_shadowed(options: "ClaudeAgentOptions") -> None: # allowed_tools, so it shadows the callback just like a hand-written entry. # skills=[names] appends Skill(name) specifiers, which do not. allowed_tools = options.allowed_tools - if options.skills == "all" and "Skill" not in allowed_tools: + if options.skills == _SKILLS_ALL and "Skill" not in allowed_tools: allowed_tools = [*allowed_tools, "Skill"] message = _get_can_use_tool_shadowed_warning(options.permission_mode, allowed_tools) if message is not None: @@ -2009,6 +2012,8 @@ class ClaudeAgentOptions: - ``"all"``: enable every discovered skill. - ``list[str]``: enable only the listed skills. Names match the SKILL.md ``name`` / directory name, or ``plugin:skill`` for plugin-qualified skills. + Names must be exact: wildcards, delimiters, and surrounding whitespace + raise at ``connect()``. This is a **context filter**, not a sandbox: unlisted skills are hidden from the model's listing and rejected by the Skill tool, but their files diff --git a/tests/test_transport.py b/tests/test_transport.py index 4c1a5abc..ce700876 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -666,6 +666,209 @@ def test_build_command_skills_does_not_duplicate_entries(self): cmd = transport._build_command() assert cmd[cmd.index("--allowedTools") + 1] == "Skill(pdf)" + @pytest.mark.parametrize( + "hostile_name", + [ + # Names containing rule-syntax delimiters cannot be represented + # as a single Skill(name) entry and must be rejected, never + # formatted into the --allowedTools value. + "x),Bash(*", + "safe),Bash,Skill(dummy", + "name,with,commas", + "unbalanced(", + "unbalanced)", + "()", + ], + ) + def test_build_command_skills_rejects_rule_syntax_delimiters( + self, hostile_name: str + ): + """Skill names containing rule-syntax delimiters raise ValueError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="Invalid skill name"): + transport._build_command() + + @pytest.mark.parametrize( + "hostile_name", + ["with\nnewline", "with\ttab", "nul\x00byte", "del\x7fchar"], + ) + def test_build_command_skills_rejects_control_characters(self, hostile_name: str): + """Skill names containing control characters raise ValueError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="Invalid skill name"): + transport._build_command() + + @pytest.mark.parametrize("empty_name", ["", " ", " \t "]) + def test_build_command_skills_rejects_empty_names(self, empty_name: str): + """Empty or whitespace-only skill names raise ValueError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[empty_name]), + ) + with pytest.raises(ValueError, match="non-empty"): + transport._build_command() + + def test_build_command_skills_rejects_non_string_names(self): + """Non-string entries in the skills list raise TypeError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[42]), # type: ignore[list-item] + ) + with pytest.raises(TypeError, match="must be strings"): + transport._build_command() + + @pytest.mark.parametrize( + "wildcard_name", + ["pdf:*", "my skill *", ":*"], + ) + def test_build_command_skills_rejects_wildcard_suffix_names( + self, wildcard_name: str + ): + """Wildcard-suffix skill names raise ValueError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[wildcard_name]), + ) + with pytest.raises(ValueError, match="wildcard-suffix"): + transport._build_command() + + @pytest.mark.parametrize("skills", ["pdf", "pdf-tools", "ALL"]) + def test_build_command_skills_rejects_a_bare_string(self, skills: str): + """A string is iterable, so skills="pdf" would build Skill(p),Skill(d),...""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=skills), # type: ignore[arg-type] + ) + with pytest.raises(TypeError, match="must be a list of skill names"): + transport._build_command() + + @pytest.mark.parametrize( + "skills", + [("pdf",), {"pdf"}, (n for n in ["pdf"])], + ids=["tuple", "set", "generator"], + ) + def test_build_command_skills_rejects_non_list_iterables(self, skills): + """These build Skill(name) rules but are dropped from initialize, so the + session skill filter is never installed.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=skills), # type: ignore[arg-type] + ) + with pytest.raises(TypeError, match="must be a list of skill names"): + transport._build_command() + + def test_build_command_skills_rejects_bare_wildcard(self): + """A literal '*' name raises, pointing to the skills="all" option.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=["*"]), + ) + with pytest.raises(ValueError, match='use skills="all"'): + transport._build_command() + + def test_build_command_skills_rejects_unpaired_trailing_backslash(self): + """A name ending in a single trailing backslash raises ValueError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=["name\\"]), + ) + with pytest.raises(ValueError, match="unpaired backslash"): + transport._build_command() + + @pytest.mark.parametrize("hostile_name", ["name\\\\", "name\\\\\\", "mid\\\\dle"]) + def test_build_command_skills_rejects_consecutive_backslashes( + self, hostile_name: str + ): + """Consecutive backslashes collapse at parse time, renaming the skill.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="consecutive backslashes"): + transport._build_command() + + @pytest.mark.parametrize("hostile_name", ["\ufeffpdf", "pdf\ufeff"]) + def test_build_command_skills_rejects_byte_order_marks(self, hostile_name: str): + """The CLI trims U+FEFF as whitespace; Python's str.strip() does not.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="Invalid skill name"): + transport._build_command() + + @pytest.mark.parametrize("hostile_name", [" pdf", "pdf ", "\tpdf", " pdf "]) + def test_build_command_skills_rejects_surrounding_whitespace( + self, hostile_name: str + ): + """A padded rule can never match: the Skill tool trims before matching.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="whitespace"): + transport._build_command() + + @pytest.mark.parametrize("hostile_name", ["/pdf", "/myplugin:pdf"]) + def test_build_command_skills_rejects_leading_slash(self, hostile_name: str): + """The session allowlist matches verbatim, so '/pdf' hides every skill.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="may not start with"): + transport._build_command() + + @pytest.mark.parametrize("hostile_name", ["lone\ud800surrogate", "\udc00leading"]) + def test_build_command_skills_rejects_surrogate_code_points( + self, hostile_name: str + ): + """No CLI-discovered skill name contains a surrogate code point.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="surrogate"): + transport._build_command() + + @pytest.mark.parametrize("hostile_name", ["nel\u0085end", "csi\u009bend"]) + def test_build_command_skills_rejects_c1_control_characters( + self, hostile_name: str + ): + """Names containing C1 control characters raise ValueError.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[hostile_name]), + ) + with pytest.raises(ValueError, match="Invalid skill name"): + transport._build_command() + + @pytest.mark.parametrize( + "benign_name", + [ + "pdf-tools", + "my_skill.v2", + "myplugin:pdf", + "skill with spaces", + "dir\\sub", + "日本語スキル", + ], + ) + def test_build_command_skills_accepts_ordinary_names(self, benign_name: str): + """Ordinary names -- plugin-qualified, spaced, non-ASCII -- still work.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(skills=[benign_name]), + ) + cmd = transport._build_command() + assert cmd[cmd.index("--allowedTools") + 1] == f"Skill({benign_name})" + @pytest.mark.parametrize( ("skills", "extra", "want_tools", "want_sources", "want_init_skills"), [