From c36572bb4dfcb598b89a6ad0ac4b9e923d9637c0 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 22 Jul 2026 10:25:05 +0500 Subject: [PATCH 1/3] fix(integrations): validate cached catalog shape before returning it The catalog cache-read branch returned json.loads(cache_file) directly, skipping the shape validation the fresh-fetch branch enforces (dict root + 'integrations' mapping). A poisoned or older-format cache (e.g. {"integrations": []}) was therefore returned as-is and later crashed with 'AttributeError: list object has no attribute items' when the caller iterated integrations. Validate the cached object the same way; the raised ValueError is already caught by the surrounding handler, which drops the corrupt cache and refetches from source. Test: a fresh-but-mis-shaped cache is dropped and the valid source refetched (fails before: AttributeError). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/integrations/catalog.py | 12 +++++- .../integrations/test_integration_catalog.py | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index c9797934b5..0b06e41a80 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -153,7 +153,17 @@ def _fetch_single_catalog( cached_at = cached_at.replace(tzinfo=timezone.utc) age = (datetime.now(timezone.utc) - cached_at).total_seconds() if age < self.CACHE_DURATION: - return json.loads(cache_file.read_text(encoding="utf-8")) + cached = json.loads(cache_file.read_text(encoding="utf-8")) + # A poisoned/older-format cache must clear the same shape + # checks as a fresh fetch — otherwise a payload like [] or + # {"integrations": []} is returned and later crashes on + # .items()/.get(). The ValueError is caught just below, + # which drops the corrupt cache and refetches from source. + if not isinstance(cached, dict) or not isinstance( + cached.get("integrations"), dict + ): + raise ValueError("cached catalog has invalid shape") + return cached except (json.JSONDecodeError, ValueError, KeyError, TypeError, AttributeError, OSError, UnicodeError): # Cache is invalid or stale metadata; delete and refetch from source. try: diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index 3ab9c3612e..815f6f75f9 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -251,6 +251,48 @@ def test_fetch_and_search_all(self, tmp_path, monkeypatch): ids = [r["id"] for r in results] assert "acme-coder" in ids + def test_poisoned_cache_shape_is_dropped_and_refetched(self, tmp_path, monkeypatch): + """A fresh-but-mis-shaped cache (e.g. integrations as a list) must be + dropped and refetched, not returned — otherwise it later crashes on + .items(). The cache path must clear the same shape checks as a fresh + fetch.""" + 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) + + catalog = { + "schema_version": "1.0", + "updated_at": "2026-01-01T00:00:00Z", + "integrations": { + "acme-coder": { + "id": "acme-coder", "name": "Acme Coder", "version": "2.0.0", + "description": "Community integration", "author": "acme-org", + "tags": ["cli"], + }, + }, + } + self._patch_urlopen(monkeypatch, catalog) + cat.search() # populate the cache legitimately + + # Poison the cached payload (integrations as a list), keeping the fresh + # metadata so the age check passes and the cache branch is taken. + cache_dir = tmp_path / ".specify" / "integrations" / ".cache" + data_files = [ + f for f in cache_dir.glob("catalog-*.json") + if not f.name.endswith("-metadata.json") + ] + assert data_files, "cache was not populated" + data_files[0].write_text( + json.dumps({"schema_version": "1.0", "integrations": []}), + encoding="utf-8", + ) + + # The poisoned cache is dropped and the (valid) source is refetched. + results = cat.search() + assert "acme-coder" in [r["id"] for r in results] + def test_search_by_tag(self, tmp_path, monkeypatch): monkeypatch.setenv("HOME", str(tmp_path)) monkeypatch.setenv("USERPROFILE", str(tmp_path)) From b88bce98a984b9873d5542a6f3bcd2c4a2e54223 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 22 Jul 2026 18:43:50 +0500 Subject: [PATCH 2/3] fix(integrations): share one catalog-shape validator across cache and fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review: the cache-read path checked only that the payload was a dict with a dict 'integrations', while the fresh-fetch path also required 'schema_version'. That asymmetry let an older/poisoned cache such as {"integrations": {}} (no schema_version) bypass the format contract instead of being dropped and refetched. Introduce a shared `_catalog_shape_error()` helper and use it in both paths so they enforce the same contract (dict + schema_version + dict integrations). The fresh path still raises IntegrationCatalogError with the "Invalid catalog format from " prefix; the cache path still raises ValueError (caught to drop+refetch). Add a test for the missing-schema_version cache case. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/integrations/catalog.py | 54 +++++++++++-------- .../integrations/test_integration_catalog.py | 41 ++++++++++++++ 2 files changed, 73 insertions(+), 22 deletions(-) diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index 0b06e41a80..1ed98f5fde 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -40,6 +40,25 @@ class IntegrationDescriptorError(Exception): """Raised when an integration.yml descriptor is invalid.""" +def _catalog_shape_error(payload: Any) -> Optional[str]: + """Return a human-readable reason if *payload* is not a valid integration + catalog document, else ``None``. + + Shared by the fresh-fetch and cache-read paths so both enforce the same + format contract: a JSON object carrying ``schema_version`` and a mapping + ``integrations``. Keeping a single validator prevents the two paths from + drifting (e.g. a cache that skips the ``schema_version`` check and lets an + older/poisoned payload bypass validation). + """ + if not isinstance(payload, dict): + return "expected a JSON object" + if "schema_version" not in payload or "integrations" not in payload: + return "missing required 'schema_version' or 'integrations' key" + if not isinstance(payload.get("integrations"), dict): + return "'integrations' must be a JSON object" + return None + + # --------------------------------------------------------------------------- # IntegrationCatalogEntry # --------------------------------------------------------------------------- @@ -154,15 +173,16 @@ def _fetch_single_catalog( age = (datetime.now(timezone.utc) - cached_at).total_seconds() if age < self.CACHE_DURATION: cached = json.loads(cache_file.read_text(encoding="utf-8")) - # A poisoned/older-format cache must clear the same shape - # checks as a fresh fetch — otherwise a payload like [] or - # {"integrations": []} is returned and later crashes on - # .items()/.get(). The ValueError is caught just below, - # which drops the corrupt cache and refetches from source. - if not isinstance(cached, dict) or not isinstance( - cached.get("integrations"), dict - ): - raise ValueError("cached catalog has invalid shape") + # A poisoned/older-format cache must clear the SAME shape + # contract as a fresh fetch (via the shared validator) — + # otherwise a payload like [], {"integrations": []}, or one + # missing "schema_version" is returned and later crashes on + # .items()/.get() or silently bypasses the format contract. + # The ValueError is caught just below, which drops the + # corrupt cache and refetches from source. + shape_error = _catalog_shape_error(cached) + if shape_error is not None: + raise ValueError(f"cached catalog has invalid shape: {shape_error}") return cached except (json.JSONDecodeError, ValueError, KeyError, TypeError, AttributeError, OSError, UnicodeError): # Cache is invalid or stale metadata; delete and refetch from source. @@ -182,20 +202,10 @@ def _fetch_single_catalog( self._validate_catalog_url(final_url) catalog_data = json.loads(resp.read()) - if not isinstance(catalog_data, dict): - raise IntegrationCatalogError( - f"Invalid catalog format from {entry.url}: expected a JSON object" - ) - if ( - "schema_version" not in catalog_data - or "integrations" not in catalog_data - ): - raise IntegrationCatalogError( - f"Invalid catalog format from {entry.url}" - ) - if not isinstance(catalog_data.get("integrations"), dict): + shape_error = _catalog_shape_error(catalog_data) + if shape_error is not None: raise IntegrationCatalogError( - f"Invalid catalog format from {entry.url}: 'integrations' must be a JSON object" + f"Invalid catalog format from {entry.url}: {shape_error}" ) try: diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index 815f6f75f9..ebb5d6a90a 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -293,6 +293,47 @@ def test_poisoned_cache_shape_is_dropped_and_refetched(self, tmp_path, monkeypat results = cat.search() assert "acme-coder" in [r["id"] for r in results] + def test_cache_missing_schema_version_is_dropped_and_refetched(self, tmp_path, monkeypatch): + """A cache that has a dict 'integrations' but no 'schema_version' must be + dropped and refetched — the cache path enforces the SAME contract as a + fresh fetch (which rejects a missing schema_version). Otherwise an + older/poisoned payload like {"integrations": {}} bypasses the format + contract and hides real catalog entries.""" + 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) + + catalog = { + "schema_version": "1.0", + "updated_at": "2026-01-01T00:00:00Z", + "integrations": { + "acme-coder": { + "id": "acme-coder", "name": "Acme Coder", "version": "2.0.0", + "description": "Community integration", "author": "acme-org", + "tags": ["cli"], + }, + }, + } + self._patch_urlopen(monkeypatch, catalog) + cat.search() # populate the cache legitimately + + cache_dir = tmp_path / ".specify" / "integrations" / ".cache" + data_files = [ + f for f in cache_dir.glob("catalog-*.json") + if not f.name.endswith("-metadata.json") + ] + assert data_files, "cache was not populated" + # Well-shaped except for the missing schema_version key. + data_files[0].write_text( + json.dumps({"integrations": {}}), + encoding="utf-8", + ) + + results = cat.search() + assert "acme-coder" in [r["id"] for r in results] + def test_search_by_tag(self, tmp_path, monkeypatch): monkeypatch.setenv("HOME", str(tmp_path)) monkeypatch.setenv("USERPROFILE", str(tmp_path)) From 0f362d3f2560a762f48019ea410a99509984e620 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 22 Jul 2026 18:47:59 +0500 Subject: [PATCH 3/3] test(integrations): unit-test the shared catalog-shape validator directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the integration-level missing-schema_version cache test (which was masked by multi-source merging — a sibling catalog source still supplied the entry, so it passed regardless of the fix) with a direct unit test of _catalog_shape_error. This deterministically proves both paths now reject a payload missing schema_version, a non-dict integrations, or a non-dict payload, and accept a well-formed one. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../integrations/test_integration_catalog.py | 66 +++++++------------ 1 file changed, 25 insertions(+), 41 deletions(-) diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index ebb5d6a90a..43eeb67889 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -13,9 +13,34 @@ IntegrationDescriptor, IntegrationDescriptorError, IntegrationValidationError, + _catalog_shape_error, ) +class TestCatalogShapeValidator: + """The shared shape validator used by BOTH the fresh-fetch and cache-read + paths, so a poisoned/older cache can't bypass the format contract the fresh + fetch enforces (dict + 'schema_version' + dict 'integrations').""" + + def test_valid_payload_returns_none(self): + assert _catalog_shape_error({"schema_version": "1.0", "integrations": {}}) is None + + def test_missing_schema_version_is_rejected(self): + # The exact bypass the two paths used to disagree on: a dict with a dict + # 'integrations' but no 'schema_version'. + assert _catalog_shape_error({"integrations": {}}) is not None + + def test_missing_integrations_is_rejected(self): + assert _catalog_shape_error({"schema_version": "1.0"}) is not None + + def test_non_dict_integrations_is_rejected(self): + assert _catalog_shape_error({"schema_version": "1.0", "integrations": []}) is not None + + @pytest.mark.parametrize("payload", [[], "x", 5, None]) + def test_non_dict_payload_is_rejected(self, payload): + assert _catalog_shape_error(payload) is not None + + # --------------------------------------------------------------------------- # IntegrationCatalogEntry # --------------------------------------------------------------------------- @@ -293,47 +318,6 @@ def test_poisoned_cache_shape_is_dropped_and_refetched(self, tmp_path, monkeypat results = cat.search() assert "acme-coder" in [r["id"] for r in results] - def test_cache_missing_schema_version_is_dropped_and_refetched(self, tmp_path, monkeypatch): - """A cache that has a dict 'integrations' but no 'schema_version' must be - dropped and refetched — the cache path enforces the SAME contract as a - fresh fetch (which rejects a missing schema_version). Otherwise an - older/poisoned payload like {"integrations": {}} bypasses the format - contract and hides real catalog entries.""" - 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) - - catalog = { - "schema_version": "1.0", - "updated_at": "2026-01-01T00:00:00Z", - "integrations": { - "acme-coder": { - "id": "acme-coder", "name": "Acme Coder", "version": "2.0.0", - "description": "Community integration", "author": "acme-org", - "tags": ["cli"], - }, - }, - } - self._patch_urlopen(monkeypatch, catalog) - cat.search() # populate the cache legitimately - - cache_dir = tmp_path / ".specify" / "integrations" / ".cache" - data_files = [ - f for f in cache_dir.glob("catalog-*.json") - if not f.name.endswith("-metadata.json") - ] - assert data_files, "cache was not populated" - # Well-shaped except for the missing schema_version key. - data_files[0].write_text( - json.dumps({"integrations": {}}), - encoding="utf-8", - ) - - results = cat.search() - assert "acme-coder" in [r["id"] for r in results] - def test_search_by_tag(self, tmp_path, monkeypatch): monkeypatch.setenv("HOME", str(tmp_path)) monkeypatch.setenv("USERPROFILE", str(tmp_path))