From 1dadf3807c92c881e4cb51ab2ac5c44bce6599fa Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Mon, 3 Aug 2026 17:04:20 +0500 Subject: [PATCH 1/2] fix(manifests): reject non-string metadata instead of crashing on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- src/specify_cli/extensions/__init__.py | 26 +++++++ src/specify_cli/presets/__init__.py | 29 ++++++++ tests/test_extensions.py | 96 ++++++++++++++++++++++++++ tests/test_presets.py | 96 ++++++++++++++++++++++++++ 4 files changed, 247 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 0936e5a445..61cbaac89f 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -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"]): @@ -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 diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 2f32d162b4..035b4ffcb1 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -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"]): @@ -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 ("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']}': " diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 63df3133fe..22da438281 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -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. 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 ): diff --git a/tests/test_presets.py b/tests/test_presets.py index d4c964c838..798d7aaf60 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -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. 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", [ From aa1a85aec6a9e710b66de18946358f240b214951 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Mon, 3 Aug 2026 18:35:56 +0500 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/presets/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 035b4ffcb1..db3ab8b367 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -389,7 +389,7 @@ def _validate(self): # malformed manifest. The sibling extension manifest already # rejects a non-string command 'file' via # relative_extension_path_violation(). - for field in ("name", "file"): + for field in ("type", "name", "file"): if not isinstance(tmpl[field], str): raise PresetValidationError( f"Invalid template {field}: expected a string, "