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
26 changes: 26 additions & 0 deletions src/specify_cli/extensions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,25 @@ def _validate(self):
raise ValidationError(
f"Invalid extension: expected a mapping, got {type(ext).__name__}"
)
# Check presence AND type: the format/version checks below feed these
# values straight to ``re.match`` and ``packaging.Version``, both of
# which raise a bare TypeError on a non-string. YAML makes that an easy
# authoring slip -- unquoted ``version: 1.0`` parses as a float and
# ``id: 2`` as an int -- and TypeError is not a ValidationError, so it
# escapes every caller that already handles a malformed manifest (see
# list_installed()'s "Corrupted extension" fallback, which catches
# ValidationError only, making one bad extension exit ``specify
# extension list`` with a raw traceback and hide the healthy ones).
# Mirrors the sibling IntegrationDescriptor, which already type-checks
# the same four fields.
for field in ["id", "name", "version", "description"]:
if field not in ext:
raise ValidationError(f"Missing extension.{field}")
if not isinstance(ext[field], str):
raise ValidationError(
f"Invalid extension.{field}: expected a string, "
f"got {type(ext[field]).__name__}"
)

# Validate extension ID format
if not re.match(r"^[a-z0-9-]+$", ext["id"]):
Expand Down Expand Up @@ -391,6 +407,16 @@ def _validate(self):
)
if "name" not in cmd or "file" not in cmd:
raise ValidationError("Command missing 'name' or 'file'")
# The pattern match below would raise a bare TypeError on a
# non-string name (``name: 2``), escaping the ValidationError
# contract. The 'file' field needs no check here:
# relative_extension_path_violation() below already rejects a
# non-string value.
if not isinstance(cmd["name"], str):
raise ValidationError(
f"Invalid command name: expected a string, "
f"got {type(cmd['name']).__name__}"
)

# Validate the 'file' field at manifest-load time using the single
# shared policy in relative_extension_path_violation(), so manifest
Expand Down
29 changes: 29 additions & 0 deletions src/specify_cli/presets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,25 @@ def _validate(self):

# Validate preset metadata
pack = self.data["preset"]
# Check presence AND type: the format/version checks below feed these
# values straight to ``re.match`` and ``packaging.Version``, both of
# which raise a bare TypeError on a non-string. YAML makes that an easy
# authoring slip -- unquoted ``version: 1.0`` parses as a float and
# ``id: 2`` as an int -- and TypeError is not a PresetValidationError,
# so it escapes every caller that already handles a malformed manifest
# (see list_installed()'s "Corrupted preset" fallback, which catches
# PresetValidationError only, making one bad preset exit ``specify
# preset list`` with a raw traceback and hide the healthy ones).
# Mirrors the sibling IntegrationDescriptor, which already type-checks
# the same four fields.
for field in ["id", "name", "version", "description"]:
if field not in pack:
raise PresetValidationError(f"Missing preset.{field}")
if not isinstance(pack[field], str):
raise PresetValidationError(
f"Invalid preset.{field}: expected a string, "
f"got {type(pack[field]).__name__}"
)

# Validate pack ID format
if not re.match(r'^[a-z0-9-]+$', pack["id"]):
Expand Down Expand Up @@ -367,6 +383,19 @@ def _validate(self):
"Template missing 'type', 'name', or 'file'"
)

# 'name' feeds re.match and 'file' feeds os.path.normpath below;
# both raise a bare TypeError on a non-string, which is not a
# PresetValidationError and so escapes the callers that handle a
# malformed manifest. The sibling extension manifest already
# rejects a non-string command 'file' via
# relative_extension_path_violation().
for field in ("type", "name", "file"):
if not isinstance(tmpl[field], str):
raise PresetValidationError(
f"Invalid template {field}: expected a string, "
f"got {type(tmpl[field]).__name__}"
)

if tmpl["type"] not in VALID_PRESET_TEMPLATE_TYPES:
raise PresetValidationError(
f"Invalid template type '{tmpl['type']}': "
Expand Down
96 changes: 96 additions & 0 deletions tests/test_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,102 @@ def test_required_section_not_mapping_rejected(
with pytest.raises(ValidationError, match=f"Invalid {section}"):
ExtensionManifest(manifest_path)

@pytest.mark.parametrize("field", ["id", "name", "version", "description"])
@pytest.mark.parametrize("bad", [1.0, 5, None, ["a"], {"a": 1}, True])
def test_extension_metadata_field_not_string_rejected(
self, temp_dir, valid_manifest_data, field, bad
):
"""A non-string extension.<field> must raise ValidationError, not a raw
TypeError.

The loop over these four fields only checked key PRESENCE, then fed the
values to ``re.match`` (id) and ``packaging.Version`` (version), both of
which raise a bare TypeError on a non-string. YAML makes that an easy
authoring slip: unquoted ``version: 1.0`` parses as a float and ``id: 2``
as an int. TypeError is not a ValidationError, so it escaped
list_installed()'s "Corrupted extension" fallback and made
`specify extension list` exit 1 with a raw traceback, hiding every
healthy extension too. The sibling IntegrationDescriptor already
type-checks the same four fields.
"""
import yaml

valid_manifest_data["extension"][field] = 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=f"Invalid extension.{field}"):
ExtensionManifest(manifest_path)

@pytest.mark.parametrize("bad", [1.0, 5, None, ["a"], {"a": 1}, True])
def test_command_name_not_string_rejected(
self, temp_dir, valid_manifest_data, bad
):
"""A non-string command name must raise ValidationError, not a raw
TypeError from the name-pattern match.

The sibling ``file`` field was already covered, since
relative_extension_path_violation() rejects a non-string value; ``name``
went straight into EXTENSION_COMMAND_NAME_PATTERN.match().
"""
import yaml

valid_manifest_data["provides"]["commands"][0]["name"] = 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 command name"):
ExtensionManifest(manifest_path)

def test_one_bad_manifest_does_not_hide_healthy_extensions(self, temp_dir):
"""End-to-end guard for the symptom: an unquoted ``version: 1.0`` in one
installed extension must degrade to "Corrupted extension" and still let
list_installed() report the healthy ones, instead of raising TypeError
out of the whole call.
"""
ext_root = temp_dir / ".specify" / "extensions"
for ext_id, version in (("good-ext", '"1.0.0"'), ("bad-ext", "1.0")):
ext_path = ext_root / ext_id
ext_path.mkdir(parents=True, exist_ok=True)
(ext_path / "extension.yml").write_text(
f"""schema_version: "1.0"
extension:
id: {ext_id}
name: {ext_id}
version: {version}
description: desc
requires:
speckit_version: ">=0.1.0"
provides:
commands:
- name: speckit.{ext_id}.hello
file: commands/hello.md
""",
encoding="utf-8",
)
(ext_root / ".registry").write_text(
json.dumps(
{
"schema_version": "1.0",
"extensions": {
"good-ext": {"version": "1.0.0", "enabled": True},
"bad-ext": {"version": "1.0", "enabled": True},
},
}
),
encoding="utf-8",
)

listed = {row["id"]: row for row in ExtensionManager(temp_dir).list_installed()}

assert set(listed) == {"good-ext", "bad-ext"}
assert "Corrupted" not in listed["good-ext"]["description"]
assert "Corrupted" in listed["bad-ext"]["description"]

def test_empty_provides_mapping_is_still_accepted_with_hooks(
self, temp_dir, valid_manifest_data
):
Expand Down
96 changes: 96 additions & 0 deletions tests/test_presets.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,102 @@ def test_required_section_not_mapping_raises_validation_error(
):
PresetManifest(manifest_path)

@pytest.mark.parametrize("field", ["id", "name", "version", "description"])
@pytest.mark.parametrize("bad", [1.0, 5, None, ["a"], {"a": 1}, True])
def test_preset_metadata_field_not_string_raises_validation_error(
self, temp_dir, valid_pack_data, field, bad
):
"""A non-string preset.<field> raises PresetValidationError, not a raw
TypeError.

The loop over these four fields only checked key PRESENCE, then fed the
values to ``re.match`` (id) and ``packaging.Version`` (version), both of
which raise a bare TypeError on a non-string. YAML makes that an easy
authoring slip: unquoted ``version: 1.0`` parses as a float and ``id: 2``
as an int. TypeError is not a PresetValidationError, so it escaped
list_installed()'s "Corrupted preset" fallback and made
`specify preset list` exit 1 with a raw traceback, hiding every healthy
preset too. The sibling IntegrationDescriptor already type-checks the
same four fields.
"""
valid_pack_data["preset"][field] = bad
manifest_path = temp_dir / "preset.yml"
manifest_path.write_text(yaml.safe_dump(valid_pack_data), encoding="utf-8")

with pytest.raises(
PresetValidationError,
match=rf"Invalid preset\.{field}: expected a string",
):
PresetManifest(manifest_path)

@pytest.mark.parametrize("field", ["name", "file"])
@pytest.mark.parametrize("bad", [1.0, 5, None, ["a"], {"a": 1}, True])
def test_template_entry_field_not_string_raises_validation_error(
self, temp_dir, valid_pack_data, field, bad
):
"""A non-string template ``name``/``file`` raises PresetValidationError.

``name`` reaches ``re.match`` and ``file`` reaches ``os.path.normpath``;
both raise a bare TypeError on a non-string. The sibling extension
manifest already rejects a non-string command ``file`` via
relative_extension_path_violation().
"""
valid_pack_data["provides"]["templates"][0][field] = bad
manifest_path = temp_dir / "preset.yml"
manifest_path.write_text(yaml.safe_dump(valid_pack_data), encoding="utf-8")

with pytest.raises(
PresetValidationError,
match=rf"Invalid template {field}: expected a string",
):
PresetManifest(manifest_path)

def test_one_bad_manifest_does_not_hide_healthy_presets(self, temp_dir):
"""End-to-end guard for the symptom: an unquoted ``version: 1.0`` in one
installed preset must degrade to "Corrupted preset" and still let
list_installed() report the healthy ones, instead of raising TypeError
out of the whole call.
"""
preset_root = temp_dir / ".specify" / "presets"
for pack_id, version in (("good-pack", '"1.0.0"'), ("bad-pack", "1.0")):
pack_path = preset_root / pack_id
pack_path.mkdir(parents=True, exist_ok=True)
(pack_path / "preset.yml").write_text(
f"""schema_version: "1.0"
preset:
id: {pack_id}
name: {pack_id}
version: {version}
description: desc
requires:
speckit_version: ">=0.1.0"
provides:
templates:
- type: template
name: spec
file: templates/spec.md
""",
encoding="utf-8",
)
(preset_root / ".registry").write_text(
json.dumps(
{
"schema_version": "1.0",
"presets": {
"good-pack": {"version": "1.0.0", "enabled": True},
"bad-pack": {"version": "1.0", "enabled": True},
},
}
),
encoding="utf-8",
)

listed = {row["id"]: row for row in PresetManager(temp_dir).list_installed()}

assert set(listed) == {"good-pack", "bad-pack"}
assert "Corrupted" not in listed["good-pack"]["description"]
assert "Corrupted" in listed["bad-pack"]["description"]

@pytest.mark.parametrize(
"bad",
[
Expand Down