mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
feat(extensions): verify catalog archive sha256 before install (#3080)
* feat(extensions): verify catalog archive sha256 before install Extension and preset archives were downloaded over HTTPS and unpacked (with Zip-Slip protection) but their bytes were never checked against a known digest. Trust rested entirely on TLS and the integrity of the release host, so a tampered or swapped archive from a compromised third-party release would be installed silently. Maintainers do not audit extension code, so consumer-side integrity is the only available defence. Catalog entries may now pin an optional `sha256` digest. When present, the downloaded archive is verified before it is written to disk and installed; a mismatch aborts with a clear error. Entries without `sha256` keep working unchanged (a DEBUG line records that the download was unverified), so the change is backwards compatible. The check runs on both download paths (extensions and presets) via a single shared helper so the two stay in parity. - Add `verify_archive_sha256` helper in shared_infra (digest match, `sha256:` prefix, case-insensitive; DEBUG log when no digest declared) - Enforce it in ExtensionCatalog.download_extension and PresetCatalog.download_pack, before the archive is written to disk - Document the optional `sha256` field in the publishing guides - Tests: helper unit tests + matching/mismatch/no-digest on both paths Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Assisted-by: AI * fix(extensions): harden sha256 parsing and tidy download test mocks Follow-up to the review on #3080: - shared_infra.verify_archive_sha256: strip only a literal `sha256:` algorithm prefix (case-insensitive) instead of `split(':', 1)[-1]`, which silently dropped any prefix — so `md5:<64-hex>` was accepted as if it were a valid SHA-256. Validate that the declared value is exactly 64 hex characters and raise a clear error otherwise, and compare with `hmac.compare_digest` for a constant-time check. Add tests covering a malformed digest and a non-`sha256:` prefix (both previously accepted). - Download test helpers: configure the context-manager mock via `__enter__.return_value`/`__exit__.return_value` rather than assigning a `lambda s: s`, which is clearer and independent of the invocation arity. Assisted-by: AI Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com> * fix(extensions): reject a declared-but-empty sha256 instead of skipping verification verify_archive_sha256 skipped on any falsy expected value, so a present-but-empty digest (e.g. sha256: "" reached via ...get("sha256")) silently disabled the integrity check instead of surfacing the authoring error. Guard on expected is None so only an absent digest skips; blank/whitespace/bare-prefix values fall through to the 64-hex validation and are rejected. Adds a regression test. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> * docs(shared_infra): clarify _SHA256_HEX_RE accepts and normalizes uppercase The comment described the regex as matching '64 lowercase' hex characters, but verify_archive_sha256 lowercases the declared value (raw.lower()) before matching, so an uppercase digest is accepted and normalized rather than rejected. Clarify the comment to avoid misleading future readers. Addresses Copilot review feedback on shared_infra.py. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> * test(presets): cover the no-sha256 backwards-compatible path Address Copilot review: download_pack's optional sha256 verification was tested for match/mismatch but not the backwards-compatible path where a catalog entry has no sha256 (pack_info.get("sha256") is None). Add a no-sha256 test mirroring the extensions coverage so the helper never silently becomes mandatory for presets. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> --------- Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com>
This commit is contained in:
@@ -31,6 +31,7 @@ from .._invocation_style import is_dollar_skills_agent, is_slash_skills_agent
|
||||
from .._utils import dump_frontmatter, relative_extension_path_violation
|
||||
from ..catalogs import CatalogEntry as BaseCatalogEntry
|
||||
from ..catalogs import CatalogStackBase
|
||||
from ..shared_infra import verify_archive_sha256
|
||||
|
||||
_FALLBACK_CORE_COMMAND_NAMES = frozenset(
|
||||
{
|
||||
@@ -2621,6 +2622,10 @@ class ExtensionCatalog(CatalogStackBase):
|
||||
) as response:
|
||||
zip_data = response.read()
|
||||
|
||||
verify_archive_sha256(
|
||||
zip_data, ext_info.get("sha256"), extension_id, ExtensionError
|
||||
)
|
||||
|
||||
zip_path.write_bytes(zip_data)
|
||||
return zip_path
|
||||
|
||||
|
||||
@@ -31,6 +31,7 @@ from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priorit
|
||||
from .._init_options import is_ai_skills_enabled
|
||||
from ..integrations.base import IntegrationBase
|
||||
from .._utils import dump_frontmatter
|
||||
from ..shared_infra import verify_archive_sha256
|
||||
|
||||
|
||||
def _substitute_core_template(
|
||||
@@ -2505,6 +2506,10 @@ class PresetCatalog:
|
||||
with self._open_url(download_url, timeout=60, extra_headers=extra_headers) as response:
|
||||
zip_data = response.read()
|
||||
|
||||
verify_archive_sha256(
|
||||
zip_data, pack_info.get("sha256"), pack_id, PresetError
|
||||
)
|
||||
|
||||
zip_path.write_bytes(zip_data)
|
||||
return zip_path
|
||||
|
||||
|
||||
@@ -2,6 +2,9 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
import hmac
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import tempfile
|
||||
@@ -11,6 +14,74 @@ from typing import Any
|
||||
from .integrations.base import IntegrationBase
|
||||
from .integrations.manifest import IntegrationManifest
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Matches a SHA-256 digest in its normalized form: exactly 64 hexadecimal
|
||||
# characters. Callers lowercase the declared value before matching (see
|
||||
# ``expected_hex = raw.lower()`` below), so an uppercase digest is accepted and
|
||||
# normalized rather than rejected.
|
||||
_SHA256_HEX_RE = re.compile(r"^[0-9a-f]{64}$")
|
||||
|
||||
|
||||
def verify_archive_sha256(
|
||||
data: bytes,
|
||||
expected: str | None,
|
||||
name: str,
|
||||
error_cls: type[Exception],
|
||||
) -> None:
|
||||
"""Verify downloaded archive bytes against a catalog-declared SHA-256.
|
||||
|
||||
Catalog entries may pin the expected digest of their release archive in a
|
||||
``sha256`` field (optionally prefixed with ``"sha256:"``). When present, the
|
||||
downloaded bytes must match before they are written to disk and installed,
|
||||
so a corrupted or tampered archive is rejected even though the transport was
|
||||
HTTPS. Entries without a declared digest are accepted unchanged, keeping the
|
||||
check backwards compatible.
|
||||
|
||||
Args:
|
||||
data: The raw downloaded archive bytes.
|
||||
expected: The catalog-declared SHA-256 hex digest, or ``None``.
|
||||
name: The extension/preset id, used in the error message.
|
||||
error_cls: Exception type to raise on mismatch (e.g. ``ExtensionError``).
|
||||
|
||||
Raises:
|
||||
error_cls: If ``expected`` is provided and is not a well-formed
|
||||
SHA-256 hex digest, or does not match ``data``.
|
||||
"""
|
||||
# Skip only when no digest is declared at all (``None``). A declared but
|
||||
# empty/blank value (e.g. ``sha256: ""``) is an authoring error, not an
|
||||
# opt-out: let it fall through to the format check below so it is rejected
|
||||
# rather than silently disabling verification.
|
||||
if expected is None:
|
||||
logger.debug(
|
||||
"No sha256 declared for %r; archive integrity was not verified.",
|
||||
name,
|
||||
)
|
||||
return
|
||||
# Strip *only* a literal ``sha256:`` algorithm prefix (case-insensitive).
|
||||
# Any other prefix is part of the value and must not be silently dropped,
|
||||
# otherwise a malformed or wrong-algorithm digest (e.g. ``md5:...``) would
|
||||
# be quietly accepted as if it were a valid SHA-256.
|
||||
raw = str(expected).strip()
|
||||
if raw[:7].lower() == "sha256:":
|
||||
raw = raw[7:].strip()
|
||||
expected_hex = raw.lower()
|
||||
if not _SHA256_HEX_RE.match(expected_hex):
|
||||
raise error_cls(
|
||||
f"Invalid sha256 declared for {name!r}: expected 64 hexadecimal "
|
||||
f"characters (optionally prefixed with 'sha256:'), got "
|
||||
f"{expected!r}."
|
||||
)
|
||||
actual_hex = hashlib.sha256(data).hexdigest()
|
||||
# Constant-time comparison: both sides are fixed-length hex digests, so use
|
||||
# ``hmac.compare_digest`` to avoid leaking information through timing.
|
||||
if not hmac.compare_digest(actual_hex, expected_hex):
|
||||
raise error_cls(
|
||||
f"Integrity check failed for {name!r}: the catalog declares "
|
||||
f"sha256 {expected_hex}, but the downloaded archive is "
|
||||
f"{actual_hex}. The archive may be corrupted or tampered with."
|
||||
)
|
||||
|
||||
|
||||
class SymlinkedSharedPathError(ValueError):
|
||||
"""Raised when a shared infrastructure path or ancestor is a symlink.
|
||||
|
||||
Reference in New Issue
Block a user