From 0582635300fd2d8579b671a8c5e2dfe2728539b4 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 25 Aug 2026 19:48:56 +0500 Subject: [PATCH] fix(workflows): reject falsy non-mapping catalog config in add/remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `WorkflowCatalog.remove_catalog`, `StepCatalog.add_catalog`, and `StepCatalog.remove_catalog` all read their config file with `yaml.safe_load(...) or {}`, which coerces a FALSY non-mapping top-level document (`[]`, `false`, `0`, `''`) to `{}` before the `isinstance(data, dict)` check ever runs — silently swallowing a corrupted config instead of raising, while a truthy non-mapping (`5`, a bare list with items) correctly raises. `WorkflowCatalog._load_catalog_config` (used by `get_active_catalogs`) and `WorkflowCatalog.add_catalog` already guard against this correctly, with a comment explaining why `or {}` is wrong here; the other three call sites in the same file reimplement the read inline and missed the fix. Same shape as the falsy-or-coerce bug class fixed in #4094 (preset catalog add/remove) and #4187 (integration descriptor loading). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt --- src/specify_cli/workflows/catalog.py | 25 ++++++++++++---- tests/test_workflows.py | 43 ++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 61f490631c..f944f95bfd 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -784,12 +784,17 @@ def remove_catalog(self, index: int) -> str: raise WorkflowValidationError("No catalog config file found.") try: - data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(config_path.read_text(encoding="utf-8")) except (yaml.YAMLError, OSError, UnicodeDecodeError) as exc: raise WorkflowValidationError( f"Catalog config file is unreadable or malformed: {exc}" ) from exc - if not isinstance(data, dict): + # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping + # (top-level ``[]``, ``false``, ``0``, ``''``) into ``{}`` and silently + # swallows it, matching _load_catalog_config's guard above. + if data is None: + data = {} + elif not isinstance(data, dict): raise WorkflowValidationError( "Catalog config file is corrupted (expected a mapping)." ) @@ -1394,11 +1399,16 @@ def add_catalog(self, url: str, name: str | None = None) -> None: data: dict[str, Any] = {"catalogs": []} if config_path.exists(): try: - raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} + raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) except (yaml.YAMLError, OSError, UnicodeDecodeError) as exc: raise StepValidationError( f"Catalog config file is unreadable or malformed: {exc}" ) from exc + # Do NOT coerce with ``or {}`` here: that also turns a FALSY + # non-mapping (top-level ``[]``, ``false``, ``0``, ``''``) into + # ``{}`` and silently swallows it. + if raw is None: + raw = {"catalogs": []} if not isinstance(raw, dict): raise StepValidationError( "Catalog config file is corrupted (expected a mapping)." @@ -1463,12 +1473,17 @@ def remove_catalog(self, index: int) -> str: raise StepValidationError("No step catalog config file found.") try: - data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(config_path.read_text(encoding="utf-8")) except (yaml.YAMLError, OSError, UnicodeDecodeError) as exc: raise StepValidationError( f"Catalog config file is unreadable or malformed: {exc}" ) from exc - if not isinstance(data, dict): + # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping + # (top-level ``[]``, ``false``, ``0``, ``''``) into ``{}`` and silently + # swallows it. + if data is None: + data = {} + elif not isinstance(data, dict): raise StepValidationError( "Catalog config file is corrupted (expected a mapping)." ) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d599f3c6a4..9e9a896438 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8453,6 +8453,20 @@ def test_remove_catalog_malformed_yaml_raises(self, project_dir): with pytest.raises(WorkflowValidationError, match="unreadable or malformed"): catalog.remove_catalog(0) + @pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"]) + def test_remove_catalog_rejects_falsy_non_mapping_config(self, project_dir, body): + """A FALSY non-mapping top-level config ([], false, 0, '') must raise + 'corrupted (expected a mapping)', not be silently coerced to {} by + ``or {}`` and then fail as a misleading 'out of range' error.""" + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + + catalog = WorkflowCatalog(project_dir) + with pytest.raises(WorkflowValidationError, match="expected a mapping"): + catalog.remove_catalog(0) + def test_add_catalog_wraps_write_oserror(self, project_dir, monkeypatch): """An OSError on write must be wrapped as WorkflowValidationError.""" from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError @@ -9099,6 +9113,21 @@ def test_add_catalog_duplicate_rejected(self, project_dir): with pytest.raises(StepValidationError, match="already configured"): catalog.add_catalog("https://example.com/steps.json") + @pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"]) + def test_add_catalog_rejects_falsy_non_mapping_config(self, project_dir, body): + """A FALSY non-mapping top-level config ([], false, 0, '') must raise + 'corrupted (expected a mapping)', not be silently coerced to {} by + ``or {}`` — matching the empty-document case above, which correctly + treats only a real absence of a document (None) as empty.""" + from specify_cli.workflows.catalog import StepCatalog, StepValidationError + + config_path = project_dir / ".specify" / "step-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + + catalog = StepCatalog(project_dir) + with pytest.raises(StepValidationError, match="expected a mapping"): + catalog.add_catalog("https://example.com/steps.json") + def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import StepCatalog @@ -9122,6 +9151,20 @@ def test_remove_catalog_invalid_index(self, project_dir): with pytest.raises(StepValidationError, match="out of range"): catalog.remove_catalog(5) + @pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"]) + def test_remove_catalog_rejects_falsy_non_mapping_config(self, project_dir, body): + """A FALSY non-mapping top-level config ([], false, 0, '') must raise + 'corrupted (expected a mapping)', not be silently coerced to {} by + ``or {}`` and then fail as a misleading 'out of range' error.""" + from specify_cli.workflows.catalog import StepCatalog, StepValidationError + + config_path = project_dir / ".specify" / "step-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + + catalog = StepCatalog(project_dir) + with pytest.raises(StepValidationError, match="expected a mapping"): + catalog.remove_catalog(0) + def test_remove_catalog_no_config(self, project_dir): from specify_cli.workflows.catalog import StepCatalog, StepValidationError