mirror of
https://github.com/github/spec-kit.git
synced 2026-07-05 21:49:47 +08:00
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.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user