diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 6d78354809..43727241d3 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -341,6 +341,25 @@ def _validate(self): ) if "speckit_version" not in requires: raise ValidationError("Missing requires.speckit_version") + # Presence alone is not enough: check_compatibility() feeds this value to + # ``SpecifierSet(required)``, guarded only by ``except InvalidSpecifier``, + # which a non-string escapes two different ways. A float/int/bool/None + # raises TypeError from the constructor, while a list or dict is an + # *iterable*, so SpecifierSet accepts it and the failure surfaces much + # later as ``AttributeError: 'str' object has no attribute 'filter'`` from + # inside .contains(). Neither is a CompatibilityError, so both bypass the + # CLI's "Compatibility Error" handler and exit 1 with a raw traceback + # naming no field. An unquoted ``speckit_version: 1.0`` is an easy YAML + # slip. Mirrors the sibling IntegrationDescriptor, which already requires + # a non-empty string here. + if ( + not isinstance(requires["speckit_version"], str) + or not requires["speckit_version"].strip() + ): + raise ValidationError( + "Invalid requires.speckit_version: expected a non-empty string, " + f"got {type(requires['speckit_version']).__name__}" + ) # Validate provides section provides = self.data["provides"] @@ -1851,6 +1870,17 @@ def check_compatibility( required = manifest.requires_speckit_version # Parse version specifier (e.g., ">=0.1.0,<2.0.0") + # Defense in depth: the manifest validator now rejects a non-string + # requires.speckit_version, but this method is public and also reachable + # with a hand-built manifest object. ``InvalidSpecifier`` alone does not + # cover a non-string -- scalars raise TypeError from the constructor, and + # a list/dict is iterable so it constructs here and only breaks inside + # .contains(). Reject up front so this always reports a CompatibilityError. + if not isinstance(required, str): + raise CompatibilityError( + "Invalid version specifier: expected a string, got " + f"{type(required).__name__} ({required!r})" + ) try: SpecifierSet(required) # Just to validate except InvalidSpecifier: diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index cc5308f3fc..45c3456fe8 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -344,6 +344,25 @@ def _validate(self): requires = self.data["requires"] if "speckit_version" not in requires: raise PresetValidationError("Missing requires.speckit_version") + # Presence alone is not enough: check_compatibility() feeds this value to + # ``SpecifierSet(required)``, guarded only by ``except InvalidSpecifier``, + # which a non-string escapes two different ways. A float/int/bool/None + # raises TypeError from the constructor, while a list or dict is an + # *iterable*, so SpecifierSet accepts it and the failure surfaces much + # later as ``AttributeError: 'str' object has no attribute 'filter'`` from + # inside .contains(). Neither is a PresetCompatibilityError, so both + # bypass the CLI's "Compatibility Error" handler and exit 1 with a raw + # traceback naming no field. An unquoted ``speckit_version: 1.0`` is an + # easy YAML slip. Mirrors the sibling IntegrationDescriptor, which already + # requires a non-empty string here. + if ( + not isinstance(requires["speckit_version"], str) + or not requires["speckit_version"].strip() + ): + raise PresetValidationError( + "Invalid requires.speckit_version: expected a non-empty string, " + f"got {type(requires['speckit_version']).__name__}" + ) # Validate provides section provides = self.data["provides"] @@ -756,6 +775,18 @@ def check_compatibility( PresetCompatibilityError: If pack is incompatible """ required = manifest.requires_speckit_version + # Defense in depth: the manifest validator now rejects a non-string + # requires.speckit_version, but this method is public and also reachable + # with a hand-built manifest object. ``InvalidSpecifier`` alone does not + # cover a non-string -- scalars raise TypeError from the constructor, and + # a list/dict is iterable so it constructs here and only breaks inside + # .contains(). Reject up front so this always reports a + # PresetCompatibilityError. + if not isinstance(required, str): + raise PresetCompatibilityError( + "Invalid version specifier: expected a string, got " + f"{type(required).__name__} ({required!r})" + ) try: SpecifierSet(required) # Just to validate except InvalidSpecifier: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 3d9146d52b..c9cf43abb8 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -410,6 +410,55 @@ def test_invalid_version(self, temp_dir, valid_manifest_data): with pytest.raises(ValidationError, match="Invalid version"): ExtensionManifest(manifest_path) + @pytest.mark.parametrize( + "bad", + [ + 1.0, # unquoted YAML float -- the likeliest authoring slip + 5, # unquoted int + True, # YAML `yes`/`true` + None, # `speckit_version:` written but left empty + [">=0.1.0"], # iterable: slips past SpecifierSet() entirely + {"min": "0.1"}, # iterable: same + ], + ) + def test_non_string_speckit_version(self, temp_dir, valid_manifest_data, bad): + """A non-string requires.speckit_version must be a ValidationError. + + It was presence-checked only, so it reached ``SpecifierSet(required)`` in + check_compatibility(), which is guarded by ``except InvalidSpecifier`` + alone. A non-string escapes that guard two ways: scalars raise TypeError + from the constructor, and a list/dict is iterable so SpecifierSet accepts + it and the failure surfaces later as ``AttributeError: 'str' object has no + attribute 'filter'`` from inside .contains(). + """ + import yaml + + valid_manifest_data["requires"]["speckit_version"] = bad + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises( + ValidationError, match="Invalid requires.speckit_version" + ): + ExtensionManifest(manifest_path) + + def test_empty_speckit_version(self, temp_dir, valid_manifest_data): + """A blank requires.speckit_version must be rejected, not treated as any.""" + import yaml + + valid_manifest_data["requires"]["speckit_version"] = " " + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises( + ValidationError, match="Invalid requires.speckit_version" + ): + ExtensionManifest(manifest_path) + def test_valid_category(self, temp_dir, valid_manifest_data): """Test manifest with various category values (free-form string).""" import yaml @@ -1265,6 +1314,28 @@ def test_check_compatibility_invalid(self, extension_dir, project_dir): with pytest.raises(CompatibilityError, match="Extension requires spec-kit"): manager.check_compatibility(manifest, "0.0.1") + @pytest.mark.parametrize( + "bad", + [1.0, 5, True, None, [">=0.1.0"], {"min": "0.1"}], + ) + def test_check_compatibility_non_string_specifier(self, project_dir, bad): + """check_compatibility() must report a non-string as CompatibilityError. + + Defense in depth for the validator check above: this method is public and + reachable with a hand-built manifest, and ``except InvalidSpecifier`` does + not cover a non-string. Without the guard, scalars raise a bare TypeError + and iterables construct fine only to break inside .contains() -- neither + is a CompatibilityError, so both bypass the CLI's "Compatibility Error" + handler and exit 1 with a raw traceback naming no field. + """ + from types import SimpleNamespace + + manager = ExtensionManager(project_dir) + manifest = SimpleNamespace(requires_speckit_version=bad) + + with pytest.raises(CompatibilityError, match="Invalid version specifier"): + manager.check_compatibility(manifest, "0.15.2") + def test_check_compatibility_allows_prerelease_builds(self, extension_dir, project_dir): """Prerelease spec-kit builds should satisfy compatible version ranges.""" manager = ExtensionManager(project_dir) diff --git a/tests/test_presets.py b/tests/test_presets.py index 243d13ab55..6c6a1ed8f2 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -406,6 +406,37 @@ def test_missing_speckit_version(self, temp_dir, valid_pack_data): with pytest.raises(PresetValidationError, match="Missing requires.speckit_version"): PresetManifest(manifest_path) + @pytest.mark.parametrize( + "bad", + [ + 1.0, # unquoted YAML float -- the likeliest authoring slip + 5, # unquoted int + True, # YAML `yes`/`true` + None, # `speckit_version:` written but left empty + [">=0.1.0"], # iterable: slips past SpecifierSet() entirely + {"min": "0.1"}, # iterable: same + " ", # blank string must not mean "any version" + ], + ) + def test_non_string_speckit_version(self, temp_dir, valid_pack_data, bad): + """A non-string requires.speckit_version must be a PresetValidationError. + + It was presence-checked only, so it reached ``SpecifierSet(required)`` in + check_compatibility(), which is guarded by ``except InvalidSpecifier`` + alone. A non-string escapes that guard two ways: scalars raise TypeError + from the constructor, and a list/dict is iterable so SpecifierSet accepts + it and the failure surfaces later as ``AttributeError: 'str' object has no + attribute 'filter'`` from inside .contains(). + """ + valid_pack_data["requires"]["speckit_version"] = bad + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + with pytest.raises( + PresetValidationError, match="Invalid requires.speckit_version" + ): + PresetManifest(manifest_path) + def test_no_templates_provided(self, temp_dir, valid_pack_data): """Test pack with no templates.""" valid_pack_data["provides"]["templates"] = [] @@ -964,6 +995,26 @@ def test_check_compatibility_invalid(self, pack_dir, temp_dir): with pytest.raises(PresetCompatibilityError, match="Invalid version specifier"): manager.check_compatibility(manifest, "0.1.5") + @pytest.mark.parametrize( + "bad", + [1.0, 5, True, None, [">=0.1.0"], {"min": "0.1"}], + ) + def test_check_compatibility_non_string_specifier(self, pack_dir, temp_dir, bad): + """check_compatibility() must report a non-string as a compatibility error. + + Defense in depth for the validator check: this method is public and the + specifier is read back out of mutable manifest data, and ``except + InvalidSpecifier`` does not cover a non-string. Without the guard, scalars + raise a bare TypeError and iterables construct fine only to break inside + .contains() -- neither is a PresetCompatibilityError, so both bypass the + CLI's "Compatibility Error" handler and exit 1 with a raw traceback. + """ + manager = PresetManager(temp_dir) + manifest = PresetManifest(pack_dir / "preset.yml") + manifest.data["requires"]["speckit_version"] = bad + with pytest.raises(PresetCompatibilityError, match="Invalid version specifier"): + manager.check_compatibility(manifest, "0.1.5") + def test_install_with_priority(self, project_dir, pack_dir): """Test installing a pack with custom priority.""" manager = PresetManager(project_dir)