Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/specify_cli/extensions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down Expand Up @@ -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:
Expand Down
31 changes: 31 additions & 0 deletions src/specify_cli/presets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down Expand Up @@ -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:
Expand Down
71 changes: 71 additions & 0 deletions tests/test_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 51 additions & 0 deletions tests/test_presets.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"] = []
Expand Down Expand Up @@ -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)
Expand Down