diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index ccc8587709..8664c181d3 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -1627,13 +1627,27 @@ def setup( command_name = src_file.stem # e.g. "plan" skill_name = f"speckit-{command_name.replace('.', '-')}" - # Parse frontmatter for description + # Parse frontmatter for description. Locate the closing ``---`` on + # its own line rather than with ``raw.split("---", 2)`` — a bare + # substring split stops at the first ``---`` *anywhere*, including + # one inside a value such as ``description: Separate sections + # with ---``, which truncates the frontmatter and drops later keys. + # The block between the delimiters is parsed unstripped so trailing + # newlines in literal (``|``) block scalars survive. frontmatter: dict[str, Any] = {} if raw.startswith("---"): - parts = raw.split("---", 2) - if len(parts) >= 3: + fm_lines = raw.splitlines(keepends=True) + fm_close = next( + ( + i + for i in range(1, len(fm_lines)) + if fm_lines[i].rstrip() == "---" + ), + None, + ) + if fm_close is not None: try: - fm = yaml.safe_load(parts[1]) + fm = yaml.safe_load("".join(fm_lines[1:fm_close])) if isinstance(fm, dict): frontmatter = fm except yaml.YAMLError: @@ -1648,11 +1662,27 @@ def setup( # Strip the processed frontmatter — we rebuild it for skills. # Preserve leading whitespace in the body to match release ZIP # output byte-for-byte (the template body starts with \n after - # the closing ---). + # the closing ---). Scan for the closing ``---`` on its own line + # rather than ``split("---", 2)`` so a ``---`` embedded in a value + # does not truncate the frontmatter and spill it into the body. if processed_body.startswith("---"): - parts = processed_body.split("---", 2) - if len(parts) >= 3: - processed_body = parts[2] + body_lines = processed_body.splitlines(keepends=True) + close_idx = next( + ( + i + for i in range(1, len(body_lines)) + if body_lines[i].rstrip() == "---" + ), + None, + ) + if close_idx is not None: + # Keep whatever trails the ``---`` marker on the closing + # line (normally just the newline) so the body stays + # byte-for-byte identical to ``split("---", 2)[2]``. The + # line-anchored check guarantees ``---`` sits at index 0. + processed_body = body_lines[close_idx][3:] + "".join( + body_lines[close_idx + 1 :] + ) # Select description — use the original template description # to stay byte-for-byte identical with release ZIP output. diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index 1ed98f5fde..1794caad83 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -21,6 +21,7 @@ import yaml from packaging import version as pkg_version +from .._download_security import MAX_JSON_METADATA_BYTES, read_response_limited from ..catalogs import CatalogEntry, CatalogStackBase @@ -200,7 +201,14 @@ def _fetch_single_catalog( final_url = resp.geturl() if final_url != entry.url: self._validate_catalog_url(final_url) - catalog_data = json.loads(resp.read()) + catalog_data = json.loads( + read_response_limited( + resp, + max_bytes=MAX_JSON_METADATA_BYTES, + error_type=IntegrationCatalogError, + label=f"catalog from {entry.url}", + ) + ) shape_error = _catalog_shape_error(catalog_data) if shape_error is not None: diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index 2e9706e77d..4688c6a21e 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -220,6 +220,33 @@ def test_load_catalog_config_rejects_falsy_non_mapping_roots( # --------------------------------------------------------------------------- +class _OversizedResponse: + """Response stub that supports bounded streaming reads for oversized-catalog tests.""" + + def __init__(self, data, url=""): + self._data = json.dumps(data).encode() + self._url = url if isinstance(url, str) else url.full_url + self._pos = 0 + + def read(self, n=-1): + if n < 0: + chunk = self._data[self._pos:] + self._pos = len(self._data) + return chunk + chunk = self._data[self._pos : self._pos + n] + self._pos += len(chunk) + return chunk + + def geturl(self): + return self._url + + def __enter__(self): + return self + + def __exit__(self, *a): + pass + + class TestCatalogFetch: """Tests that use a local HTTP server stub via monkeypatch.""" @@ -230,9 +257,16 @@ class FakeResponse: def __init__(self, data, url=""): self._data = json.dumps(data).encode() self._url = url if isinstance(url, str) else url.full_url + self._pos = 0 - def read(self): - return self._data + def read(self, n=-1): + if n < 0: + chunk = self._data[self._pos:] + self._pos = len(self._data) + return chunk + chunk = self._data[self._pos:self._pos + n] + self._pos += len(chunk) + return chunk def geturl(self): return self._url @@ -395,6 +429,90 @@ def test_invalid_catalog_format(self, tmp_path, monkeypatch): with pytest.raises(IntegrationCatalogError, match="Failed to fetch any integration catalog"): cat.search() + def test_oversized_catalog_response_rejected(self, tmp_path, monkeypatch): + """Response exceeding MAX_JSON_METADATA_BYTES is caught as IntegrationCatalogError. + + The per-entry error is logged as a warning and skipped (not fatal). + When ALL catalogs are oversized, search() raises the aggregate error. + """ + from specify_cli._download_security import MAX_JSON_METADATA_BYTES + + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("USERPROFILE", str(tmp_path)) + monkeypatch.delenv("SPECKIT_INTEGRATION_CATALOG_URL", raising=False) + (tmp_path / ".specify").mkdir() + cat = IntegrationCatalog(tmp_path) + + # Build a valid catalog dict whose JSON encoding exceeds the limit. + oversized = { + "schema_version": "1.0", + "integrations": {}, + "padding": "x" * (MAX_JSON_METADATA_BYTES + 1), + } + + import specify_cli.authentication.http as _auth_http + + def _oversized_urlopen(req, timeout=10): + url = req if isinstance(req, str) else req.full_url + return _OversizedResponse(oversized, url) + + monkeypatch.setattr(_auth_http.urllib.request, "urlopen", _oversized_urlopen) + + # Both default + community catalogs are oversized → all fail → aggregate error. + # The per-entry IntegrationCatalogError (with "exceeds maximum size") is + # logged as a warning; the aggregate raise has a different message. + with pytest.raises(IntegrationCatalogError, match="Failed to fetch any integration catalog"): + cat.search() + + def test_oversized_catalog_does_not_block_healthy_one(self, tmp_path, monkeypatch): + """When one catalog is oversized, the healthy catalog still returns results.""" + from specify_cli._download_security import MAX_JSON_METADATA_BYTES + + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("USERPROFILE", str(tmp_path)) + monkeypatch.delenv("SPECKIT_INTEGRATION_CATALOG_URL", raising=False) + specify = tmp_path / ".specify" + specify.mkdir() + + healthy_catalog = { + "schema_version": "1.0", + "integrations": { + "good-agent": { + "id": "good-agent", + "name": "Good Agent", + "version": "1.0.0", + "description": "A healthy integration", + "author": "test-org", + }, + }, + } + oversized_catalog = { + "schema_version": "1.0", + "integrations": {}, + "padding": "x" * (MAX_JSON_METADATA_BYTES + 1), + } + cfg = specify / "integration-catalogs.yml" + cfg.write_text(yaml.dump({"catalogs": [ + {"url": "https://healthy.example.com/catalog.json", "name": "healthy", "priority": 1, "install_allowed": True}, + {"url": "https://oversized.example.com/catalog.json", "name": "oversized", "priority": 2, "install_allowed": True}, + ]})) + cat = IntegrationCatalog(tmp_path) + + import specify_cli.authentication.http as _auth_http + + def _multi_catalog_urlopen(req, timeout=10): + url = req if isinstance(req, str) else req.full_url + if "oversized" in url: + return _OversizedResponse(oversized_catalog, url) + return _OversizedResponse(healthy_catalog, url) + + monkeypatch.setattr(_auth_http.urllib.request, "urlopen", _multi_catalog_urlopen) + + # The oversized catalog is skipped; the healthy catalog's integrations are returned. + results = cat.search() + ids = [r["id"] for r in results] + assert "good-agent" in ids + def test_clear_cache(self, tmp_path): (tmp_path / ".specify").mkdir() cat = IntegrationCatalog(tmp_path) @@ -592,8 +710,15 @@ class FakeResponse: def __init__(self, data, url=""): self._data = json.dumps(data).encode() self._url = url if isinstance(url, str) else url.full_url - def read(self): - return self._data + self._pos = 0 + def read(self, n=-1): + if n < 0: + chunk = self._data[self._pos:] + self._pos = len(self._data) + return chunk + chunk = self._data[self._pos:self._pos + n] + self._pos += len(chunk) + return chunk def geturl(self): return self._url def __enter__(self): diff --git a/tests/integrations/test_skill_frontmatter_quoting.py b/tests/integrations/test_skill_frontmatter_quoting.py index 739039ea3f..b42ad88459 100644 --- a/tests/integrations/test_skill_frontmatter_quoting.py +++ b/tests/integrations/test_skill_frontmatter_quoting.py @@ -34,6 +34,19 @@ Body of the command. """ +# A description whose value contains an embedded ``---``. A substring split +# (``raw.split("---", 2)``) stops at this inner marker, truncating the parsed +# frontmatter — the closing document separator on its own line is the real +# boundary. See TestSkillFrontmatterEmbeddedDashes below. +DASHED_DESCRIPTION = "Separate sections with --- markers" +DASHED_TEMPLATE = """--- +description: Separate sections with --- markers +name-marker: sentinel +--- + +Body of the command. +""" + def _parse_frontmatter(skill_file: Path) -> dict: content = skill_file.read_text(encoding="utf-8") @@ -90,6 +103,62 @@ def test_control_character_description_parses(self, tmp_path, monkeypatch): assert fm["description"] == CONTROL +def _parse_frontmatter_line_anchored(skill_file: Path) -> dict: + """Parse SKILL.md frontmatter using the closing ``---`` on its own line. + + Unlike ``_parse_frontmatter`` (which uses ``split("---", 2)``), this is + robust to a ``---`` embedded in a value, so it can validate that the + generated frontmatter is itself well formed. + """ + content = skill_file.read_text(encoding="utf-8") + assert content.startswith("---\n") + lines = content.splitlines(keepends=True) + end = next(i for i in range(1, len(lines)) if lines[i].rstrip() == "---") + return yaml.safe_load("".join(lines[1:end])) + + +class TestSkillFrontmatterEmbeddedDashes: + """A ``---`` inside a description value must not truncate parsing (#3634). + + The skills setup path parsed template frontmatter with + ``raw.split("---", 2)``, which stops at the first ``---`` *anywhere* — + including one inside a value such as ``description: ... --- ...``. That + dropped every frontmatter key after the marker (so the description fell + back to the generic default) and spilled the leftover frontmatter into + the skill body. The parser must match the closing ``---`` on its own line. + """ + + def _generate(self, tmp_path, monkeypatch, template: str) -> Path: + integration = get_integration("agy") + monkeypatch.setattr( + integration, + "shared_commands_dir", + lambda: _fake_templates(tmp_path, template), + ) + manifest = IntegrationManifest("agy", tmp_path) + created = integration.setup(tmp_path, manifest) + skill_files = [f for f in created if f.name == "SKILL.md"] + assert len(skill_files) == 1 + return skill_files[0] + + def test_dashed_description_is_preserved(self, tmp_path, monkeypatch): + skill_file = self._generate(tmp_path, monkeypatch, DASHED_TEMPLATE) + fm = _parse_frontmatter_line_anchored(skill_file) + # Buggy split("---", 2) truncates the value to "Separate sections with" + # (or drops it entirely, falling back to "Spec Kit: plan workflow"). + assert fm["description"] == DASHED_DESCRIPTION + + def test_leftover_frontmatter_not_spilled_into_body(self, tmp_path, monkeypatch): + skill_file = self._generate(tmp_path, monkeypatch, DASHED_TEMPLATE) + content = skill_file.read_text(encoding="utf-8") + lines = content.splitlines(keepends=True) + end = next(i for i in range(1, len(lines)) if lines[i].rstrip() == "---") + body = "".join(lines[end + 1 :]) + # The template's trailing frontmatter key must not leak into the body. + assert "name-marker: sentinel" not in body + assert "Body of the command." in body + + class TestHermesSkillFrontmatterQuoting: def test_multiline_description_survives(self, tmp_path, monkeypatch): home = tmp_path / "home"