From df3823f711eec103aca816adfbb98b0dce5499c5 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Thu, 21 May 2026 07:35:40 +0500 Subject: [PATCH] fix(catalogs): skip non-mapping entries during extension and preset merge Addresses Copilot review feedback on this PR. `_fetch_single_catalog` now validates that the ``extensions`` / ``presets`` value is a mapping, but it doesn't (and shouldn't) validate every entry inside that mapping. A payload like: {"schema_version": "1.0", "extensions": {"good": {...}, "bad": []}} passes the fetch-level guard, then later crashes inside ``_get_merged_extensions`` (resp. ``_get_merged_packs``) at ``{**ext_data, ...}`` with ``TypeError: 'list' object is not a mapping``. The sibling integration-catalog reader at ``src/specify_cli/integrations/catalog.py:245`` handles this with a per-entry ``isinstance(integ_data, dict)`` skip during merge, so one malformed entry doesn't poison an otherwise valid catalog. This change mirrors that pattern in the extension and preset mergers and adds regression tests asserting that valid entries continue to merge while malformed siblings are silently dropped. --- src/specify_cli/extensions.py | 10 +++++++++ src/specify_cli/presets.py | 11 +++++++++ tests/test_extensions.py | 42 +++++++++++++++++++++++++++++++++++ tests/test_presets.py | 41 ++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 5ad630383..19782aacb 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -1914,6 +1914,16 @@ class ExtensionCatalog(CatalogStackBase): continue for ext_id, ext_data in catalog_data.get("extensions", {}).items(): + # Per-entry guard: ``_fetch_single_catalog`` already validates + # that ``catalog_data["extensions"]`` is a mapping, but it + # does not (and should not) validate every entry shape there + # — one malformed entry shouldn't poison an otherwise valid + # catalog. Skip non-mapping entries here so a payload like + # ``{"extensions": {"foo": [], "bar": {...}}}`` still merges + # the valid entries without crashing on ``**ext_data``. + # Mirrors ``integrations/catalog.py:245``. + if not isinstance(ext_data, dict): + continue if ext_id not in merged: # Higher-priority catalog wins merged[ext_id] = { **ext_data, diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 523b50726..39611cd8b 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -2103,6 +2103,17 @@ class PresetCatalog: try: data = self._fetch_single_catalog(entry, force_refresh) for pack_id, pack_data in data.get("presets", {}).items(): + # Per-entry guard: ``_fetch_single_catalog`` already + # validates that ``data["presets"]`` is a mapping, but it + # does not (and should not) validate every entry shape + # there — one malformed entry shouldn't poison an + # otherwise valid catalog. Skip non-mapping entries here + # so a payload like ``{"presets": {"foo": [], "bar": + # {...}}}`` still merges the valid entries without + # crashing on ``**pack_data``. Mirrors + # ``integrations/catalog.py:245``. + if not isinstance(pack_data, dict): + continue pack_data_with_catalog = {**pack_data, "_catalog_name": entry.name, "_install_allowed": entry.install_allowed} merged[pack_id] = pack_data_with_catalog except PresetError: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index f1ce74c33..db8c080d7 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2622,6 +2622,48 @@ class TestExtensionCatalog: with pytest.raises(ExtensionError, match="Invalid catalog format"): catalog._fetch_single_catalog(entry, force_refresh=True) + def test_get_merged_extensions_skips_non_mapping_entries(self, temp_dir): + """Per-entry guard: one malformed entry shouldn't poison the merge. + + ``_fetch_single_catalog`` validates that ``extensions`` is a mapping, + but it doesn't (and shouldn't) validate every entry inside it — a + single bad entry in an otherwise-valid catalog should be skipped, not + crash the whole resolve path. Mirrors the per-entry skip in + ``integrations/catalog.py``: a malformed entry returns no error, + valid entries continue to merge normally. + """ + from unittest.mock import patch, MagicMock + + catalog = self._make_catalog(temp_dir) + # Mix of valid entry, list-shaped entry, and string-shaped entry. + payload = { + "schema_version": "1.0", + "extensions": { + "good": {"name": "Good", "version": "1.0.0"}, + "bad-list": [], + "bad-str": "oops", + }, + } + mock_response = MagicMock() + mock_response.read.return_value = json.dumps(payload).encode() + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + + entry = CatalogEntry( + url="https://example.com/catalog.json", + name="default", + priority=1, + install_allowed=True, + ) + + with patch.object(catalog, "_open_url", return_value=mock_response), \ + patch.object(catalog, "get_active_catalogs", return_value=[entry]): + merged = catalog._get_merged_extensions(force_refresh=True) + + # Only the well-formed entry survives; the two malformed entries are + # silently dropped rather than raising or crashing. + assert [ext["id"] for ext in merged] == ["good"] + def test_download_extension_sends_auth_header(self, temp_dir, monkeypatch): """download_extension passes Authorization header when a provider is configured.""" from unittest.mock import patch, MagicMock diff --git a/tests/test_presets.py b/tests/test_presets.py index 1928840d9..514365d1a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -1559,6 +1559,47 @@ class TestPresetCatalog: with pytest.raises(PresetError, match="Invalid preset catalog format"): catalog._fetch_single_catalog(entry, force_refresh=True) + def test_get_merged_packs_skips_non_mapping_entries(self, project_dir): + """Per-entry guard: one malformed entry shouldn't poison the merge. + + ``_fetch_single_catalog`` validates that ``presets`` is a mapping, + but it doesn't (and shouldn't) validate every entry inside it — a + single bad entry in an otherwise-valid catalog should be skipped, + not crash the whole resolve path. Mirrors the per-entry skip in + ``integrations/catalog.py``: a malformed entry returns no error, + valid entries continue to merge normally. + """ + from unittest.mock import patch, MagicMock + + catalog = PresetCatalog(project_dir) + payload = { + "schema_version": "1.0", + "presets": { + "good": {"name": "Good", "version": "1.0.0"}, + "bad-list": [], + "bad-str": "oops", + }, + } + mock_response = MagicMock() + mock_response.read.return_value = json.dumps(payload).encode() + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + + entry = PresetCatalogEntry( + url="https://example.com/catalog.json", + name="default", + priority=1, + install_allowed=True, + ) + + with patch.object(catalog, "_open_url", return_value=mock_response), \ + patch.object(catalog, "get_active_catalogs", return_value=[entry]): + merged = catalog._get_merged_packs(force_refresh=True) + + # Only the well-formed entry survives; the two malformed entries are + # silently dropped rather than raising or crashing. + assert list(merged.keys()) == ["good"] + def test_download_pack_sends_auth_header(self, project_dir, monkeypatch): """download_pack passes Authorization header when configured.""" from unittest.mock import patch, MagicMock