mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix(manifests): reject non-string metadata instead of crashing on it (#3943)
* fix(manifests): reject non-string metadata instead of crashing on it `ExtensionManifest` and `PresetManifest` checked only key PRESENCE for `id`/`name`/`version`/`description`, then fed the values straight to `re.match()` and `packaging.Version()`. Both raise a bare `TypeError` on a non-string, which is neither `ValidationError` nor `PresetValidationError`, so it escaped every caller that already handles a malformed manifest. YAML makes this an easy authoring slip rather than a contrived one: an unquoted `version: 1.0` parses as a float and `id: 2` as an int. The user-visible symptom is the one the in-tree comment above the section guards was written to prevent (#3898 for presets, and its extension twin): `list_installed()` degrades a bad manifest to "⚠️ Corrupted extension" but catches only the domain error, so a single bad manifest made `specify extension list` / `specify preset list` exit 1 with a raw traceback and *no output at all* — hiding every healthy extension/preset too, not just the broken one. Also unguarded on the same path: - extension `provides.commands[].name` → `TypeError` from the command-name pattern match. The sibling `file` field was already safe, since `relative_extension_path_violation()` rejects a non-string. - preset `provides.templates[].name`/`.file` → `TypeError` from `re.match` and `os.path.normpath` respectively. The third manifest twin, `IntegrationDescriptor`, is already hardened: it type-checks the same four fields and catches `TypeError` alongside `InvalidVersion`. This brings the other two in line with it. Tests: 68 added across both suites, covering each field against float, int, None, list, dict, and bool, plus an end-to-end guard per manifest type asserting a healthy entry still lists while the bad one degrades. All 68 fail with the source change reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
co-authored by
Claude Opus 5
Copilot Autofix powered by AI
parent
7ddb8194d9
commit
2d8904a21e
@@ -285,9 +285,25 @@ class ExtensionManifest:
|
||||
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"]):
|
||||
@@ -391,6 +407,16 @@ class ExtensionManifest:
|
||||
)
|
||||
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
|
||||
|
||||
@@ -307,9 +307,25 @@ class PresetManifest:
|
||||
|
||||
# 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"]):
|
||||
@@ -367,6 +383,19 @@ class PresetManifest:
|
||||
"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']}': "
|
||||
|
||||
Reference in New Issue
Block a user