From 92d59719e42c1cbfd600b2fd3f6692f50afed98b Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 11 Sep 2026 21:06:59 +0500 Subject: [PATCH] fix(bundler): treat an explicitly null catalog field as empty, not "None" `CatalogSource.from_dict` and `CatalogEntry.from_dict` coerced fields with `str(data.get(key, ""))`. That default only covers a *missing* key. A key that is present but null -- how YAML spells an empty field (`author:` with nothing after it), and what a generator emits for an absent value in JSON -- yields `None`, and `str(None)` is the literal `"None"`. Reproduced on main: name = 'None' version = 'None' role = 'None' description = 'None' author = 'None' license = 'None' download_url = 'None' sha256 = None (already guarded) repository = None (already guarded) The same constructor already guards `sha256` and `repository` against exactly this, so the treatment was internally inconsistent. For `CatalogSource` it is more than cosmetic: `"None"` is truthy, so it defeats the required-field guards and a malformed source was ACCEPTED -- CatalogSource.from_dict({"id": None, "url": None, ...}) -> id = 'None', url = 'None' registering a catalog source named "None" pointing at a URL "None". Routed through the existing `manifest._text` helper, whose docstring already describes this failure mode for bundle manifests. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/bundler/models/catalog.py | 29 ++++++---- tests/contract/test_catalog_schema.py | 66 +++++++++++++++++++++++ 2 files changed, 85 insertions(+), 10 deletions(-) diff --git a/src/specify_cli/bundler/models/catalog.py b/src/specify_cli/bundler/models/catalog.py index 2ef882d576..fe360a6744 100644 --- a/src/specify_cli/bundler/models/catalog.py +++ b/src/specify_cli/bundler/models/catalog.py @@ -13,6 +13,7 @@ from .. import BundlerError from ..lib.yamlio import ensure_within, load_yaml +from .manifest import _text CONFIG_FILENAME = "bundle-catalogs.yml" # Supported bundle-catalogs.yml schema (major version). Both readers of the @@ -70,8 +71,14 @@ def install_allowed(self) -> bool: def from_dict(cls, data: Any, scope: Scope) -> "CatalogSource": if not isinstance(data, dict): raise BundlerError("Each catalog source must be a mapping.") - source_id = str(data.get("id", "")).strip() - url = str(data.get("url", "")).strip() + # ``_text`` rather than ``str(...get(k, ""))``: the default only covers a + # *missing* key. A key present but null -- how YAML spells an empty field + # (``id:`` with nothing after it) -- yields ``None``, and ``str(None)`` + # is the literal ``"None"``, which is truthy and so sailed straight past + # the required-field guards below: a source with ``id: null`` was + # accepted and registered under the name ``"None"``. + source_id = _text(data.get("id")) + url = _text(data.get("url")) if not source_id: raise BundlerError("A catalog source is missing its 'id'.") if not url: @@ -185,14 +192,16 @@ def from_dict(cls, data: Any) -> "CatalogEntry": ) return cls( id=entry_id, - name=str(data.get("name", "")).strip(), - version=str(data.get("version", "")).strip(), - role=str(data.get("role", "")).strip(), - description=str(data.get("description", "")).strip(), - author=str(data.get("author", "")).strip(), - license=str(data.get("license", "")).strip(), - download_url=str(data.get("download_url", "")).strip(), - requires_speckit_version=str(requires.get("speckit_version", "")).strip(), + # See the note in ``CatalogSource.from_dict``: an explicitly null + # field must read as empty, not as the literal string "None". + name=_text(data.get("name")), + version=_text(data.get("version")), + role=_text(data.get("role")), + description=_text(data.get("description")), + author=_text(data.get("author")), + license=_text(data.get("license")), + download_url=_text(data.get("download_url")), + requires_speckit_version=_text(requires.get("speckit_version")), sha256=( None if data.get("sha256") is None diff --git a/tests/contract/test_catalog_schema.py b/tests/contract/test_catalog_schema.py index 3fd3a5c53d..4f6e4ca13e 100644 --- a/tests/contract/test_catalog_schema.py +++ b/tests/contract/test_catalog_schema.py @@ -256,6 +256,72 @@ def test_catalog_entry_rejects_non_boolean_verified(): CatalogEntry.from_dict(data) +@pytest.mark.parametrize( + "field", + [ + "name", + "version", + "role", + "description", + "author", + "license", + "download_url", + ], +) +def test_catalog_entry_explicit_null_field_reads_as_empty(field: str): + """An explicitly null field must read as "", not the literal "None". + + `str(data.get(key, ""))` only defaults for a *missing* key. A key present + but null — how YAML spells an empty field (`author:` with nothing after + it) — yields `None`, and `str(None)` is the truthy string `"None"`. The + same constructor already guards `sha256` and `repository` against exactly + this. + """ + from specify_cli.bundler.models.catalog import CatalogEntry + + data = catalog_entry_dict("demo") + data[field] = None + + entry = CatalogEntry.from_dict(data) + + assert getattr(entry, field) == "" + + +def test_catalog_source_rejects_an_explicitly_null_id(): + """`id: null` must be refused, not registered as a source named "None". + + The `if not source_id` guard was defeated by the truthy literal, so the + source was accepted and carried the name `"None"` into the stack. + """ + from specify_cli.bundler.models.catalog import CatalogSource, Scope + + with pytest.raises(BundlerError, match="missing its 'id'"): + CatalogSource.from_dict( + { + "id": None, + "url": "https://example.test/catalog.json", + "priority": 5, + "install_policy": "install-allowed", + }, + Scope.PROJECT, + ) + + +def test_catalog_source_rejects_an_explicitly_null_url(): + from specify_cli.bundler.models.catalog import CatalogSource, Scope + + with pytest.raises(BundlerError, match="missing its 'url'"): + CatalogSource.from_dict( + { + "id": "demo", + "url": None, + "priority": 5, + "install_policy": "install-allowed", + }, + Scope.PROJECT, + ) + + def test_catalog_entry_preserves_sha256_through_provenance(): digest = "a" * 64 payload = catalog_payload(