diff --git a/src/strands/vended_plugins/skills/agent_skills.py b/src/strands/vended_plugins/skills/agent_skills.py index 5e42b9230..e82070223 100644 --- a/src/strands/vended_plugins/skills/agent_skills.py +++ b/src/strands/vended_plugins/skills/agent_skills.py @@ -10,7 +10,7 @@ import logging from pathlib import Path from typing import TYPE_CHECKING, Any, TypeAlias -from xml.sax.saxutils import escape +from xml.sax.saxutils import escape as _xml_escape from ...hooks.events import BeforeInvocationEvent from ...plugins import Plugin, hook @@ -27,6 +27,24 @@ _RESOURCE_DIRS = ("scripts", "references", "assets") _DEFAULT_MAX_RESOURCE_FILES = 20 +_XML_EXTRA_ENTITIES = {'"': """, "'": "'"} + + +def _escape_xml(text: str) -> str: + """Escape all 5 XML special characters in text. + + Extends the standard library's ``xml.sax.saxutils.escape`` to also + escape ``"`` and ``'``, which are not covered by default. + + Args: + text: The text to escape. + + Returns: + The escaped text. + """ + return _xml_escape(text, _XML_EXTRA_ENTITIES) + + SkillSource: TypeAlias = str | Path | Skill """A single skill source: path string, Path object, or Skill instance.""" @@ -271,10 +289,10 @@ def _generate_skills_xml(self) -> str: for skill in self._skills.values(): lines.append("") - lines.append(f"{escape(skill.name)}") - lines.append(f"{escape(skill.description)}") + lines.append(f"{_escape_xml(skill.name)}") + lines.append(f"{_escape_xml(skill.description)}") if skill.path is not None: - lines.append(f"{escape(str(skill.path / 'SKILL.md'))}") + lines.append(f"{_escape_xml(str(skill.path / 'SKILL.md'))}") lines.append("") lines.append("") diff --git a/src/strands/vended_plugins/skills/skill.py b/src/strands/vended_plugins/skills/skill.py index 3e1b6bba5..65272245b 100644 --- a/src/strands/vended_plugins/skills/skill.py +++ b/src/strands/vended_plugins/skills/skill.py @@ -104,7 +104,8 @@ def _fix_yaml_colons(yaml_str: str) -> str: key, value = match.group(1), match.group(2) # If value contains a colon and isn't already quoted if ":" in value and not (value.startswith('"') or value.startswith("'")): - line = f'{key}: "{value}"' + escaped = value.replace("\\", "\\\\").replace('"', '\\"') + line = f'{key}: "{escaped}"' lines.append(line) return "\n".join(lines) diff --git a/tests/strands/vended_plugins/skills/test_agent_skills.py b/tests/strands/vended_plugins/skills/test_agent_skills.py index 52802a6c1..ac4adc44c 100644 --- a/tests/strands/vended_plugins/skills/test_agent_skills.py +++ b/tests/strands/vended_plugins/skills/test_agent_skills.py @@ -689,3 +689,43 @@ def test_skills_plugin_isinstance_check(self): plugin = AgentSkills(skills=[]) assert isinstance(plugin, Plugin) + + +class TestSkillsXmlEscaping: + """Tests for XML escaping of all 5 special characters.""" + + def test_escapes_double_quotes(self): + """Test that double quotes in descriptions are escaped.""" + skill = _make_skill(description='Use "quotes" in text') + plugin = AgentSkills(skills=[skill]) + xml = plugin._generate_skills_xml() + + assert """ in xml + assert '"quotes"' not in xml.split("")[1].split("")[0] + + def test_escapes_single_quotes(self): + """Test that single quotes (apostrophes) in descriptions are escaped.""" + skill = _make_skill(description="It's a skill") + plugin = AgentSkills(skills=[skill]) + xml = plugin._generate_skills_xml() + + # Should escape ' to ' or ' or similar + desc_content = xml.split("")[1].split("")[0] + assert "'" not in desc_content + + def test_escapes_all_five_xml_special_chars(self): + """Test that all 5 XML special characters are escaped.""" + skill = _make_skill( + name="test-skill", + description="Has & < > \" ' chars", + ) + plugin = AgentSkills(skills=[skill]) + xml = plugin._generate_skills_xml() + + desc_content = xml.split("")[1].split("")[0] + assert "&" in desc_content + assert "<" in desc_content + assert ">" in desc_content + assert """ in desc_content + # ' should be escaped as ' or similar entity + assert "'" not in desc_content diff --git a/tests/strands/vended_plugins/skills/test_skill.py b/tests/strands/vended_plugins/skills/test_skill.py index 53d6f3507..ad220f954 100644 --- a/tests/strands/vended_plugins/skills/test_skill.py +++ b/tests/strands/vended_plugins/skills/test_skill.py @@ -559,3 +559,47 @@ def test_skill_classmethods_exist(self): assert callable(getattr(Skill, "from_file", None)) assert callable(getattr(Skill, "from_content", None)) assert callable(getattr(Skill, "from_directory", None)) + + +class TestFixYamlColonsEscaping: + """Tests for _fix_yaml_colons escaping edge cases (Bug fix).""" + + def test_escapes_double_quotes_in_value(self): + """Test that double quotes in values are escaped before wrapping.""" + raw = 'description: Use when: user says "hello" and "goodbye"' + fixed = _fix_yaml_colons(raw) + # Must escape quotes: description: "Use when: user says \"hello\" and \"goodbye\"" + assert r"\"hello\"" in fixed + assert r"\"goodbye\"" in fixed + # Verify it produces valid YAML + import yaml + + result = yaml.safe_load(fixed) + assert result["description"] == 'Use when: user says "hello" and "goodbye"' + + def test_escapes_backslashes_in_value(self): + r"""Test that backslashes in values are escaped before wrapping.""" + raw = r"description: Path C:\Users\test: with colons" + fixed = _fix_yaml_colons(raw) + # Must escape backslashes before wrapping + import yaml + + result = yaml.safe_load(fixed) + assert r"C:\Users\test" in result["description"] + + def test_escapes_mixed_quotes_and_backslashes(self): + r"""Test that both quotes and backslashes are properly escaped.""" + raw = r'description: Run "cmd": C:\path\to: file' + fixed = _fix_yaml_colons(raw) + import yaml + + result = yaml.safe_load(fixed) + assert '"cmd"' in result["description"] + assert "C:\\path\\to" in result["description"] + + def test_frontmatter_with_quotes_in_colon_value(self): + """Test end-to-end: frontmatter with quotes and colons parses correctly.""" + content = '---\nname: my-skill\ndescription: Use when: user says "hello"\n---\nBody.' + frontmatter, body = _parse_frontmatter(content) + assert frontmatter["name"] == "my-skill" + assert '"hello"' in frontmatter["description"]