github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

harden: secure extension and preset archive downloads (#3141)

Browse Source 
* harden: secure extension and preset archive downloads

Adopt the shared download-security primitives from #3140 across extension and
preset catalog, package, direct-URL, and ZIP-install flows:

- bound catalog, package, and inline manifest reads;
- verify catalog SHA-256 values when present;
- replace path-only extraction with bounded traversal/symlink-safe extraction;
- validate malformed hosts and ports before opening download URLs;
- handle normalized trailing-backslash directory entries consistently.

Redirect enforcement and checksum verification remain owned by the shared
helpers already on main; this commit wires them into extension and preset
behavior.

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

* harden: close archive and catalog download edge cases

Preflight ZIP central directories before ZipFile allocates them, bound both
declared and actual payload sizes, and reject ambiguous or non-portable archive
paths before extraction.

Keep extension update manifest selection consistent with extraction, reject
unsafe catalog-derived output filenames and malformed URL types, and escape
untrusted values in download errors.

Add regression coverage for parser differentials, collisions, platform-specific
filenames, bounded call sites, and failure ordering.

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

* harden: address download security review feedback

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

* harden: close ZIP preflight review gaps

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

* fix: harden extension update preflight and rollback

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

* fix: harden extension update rollback

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
This commit is contained in:
Pascal THUET
2026-07-28 14:52:07 +02:00
committed by GitHub
parent 0117a7b977
commit 118062eac4
24 changed files with 6637 additions and 434 deletions
Download Patch File Download Diff File Expand all files Collapse all files

96
src/specify_cli/workflows/_commands.py
View File

@@ -12,7 +12,7 @@ import json
import os
import re
import sys
from pathlib import Path
from pathlib import Path, PurePosixPath
from typing import Any
import typer
@@ -401,6 +401,12 @@ def _reject_insecure_download_redirect(old_url: str, new_url: str) -> None:
# a ceiling any legitimate workflow definition should ever approach.
_MAX_WORKFLOW_YAML_BYTES = 5 * 1024 * 1024 # 5 MiB
_DOWNLOAD_CHUNK_SIZE = 65536
# Custom step packages contain executable Python, metadata, and optional helper
# files downloaded one-by-one rather than as an archive. Mirror the archive
# ceilings so a catalog cannot turn individually valid files into an unbounded
# aggregate download.
_MAX_STEP_PACKAGE_FILES = 512
_MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB
def _read_response_within_limit(response, max_bytes: int | None = None) -> bytes:
@@ -2658,14 +2664,39 @@ def workflow_step_add(
)
raise typer.Exit(1)
step_yml_url = info.get("step_yml_url") or info.get("url")
if not step_yml_url:
declared_step_yml_url = info.get("step_yml_url")
if declared_step_yml_url is not None and not isinstance(
declared_step_yml_url, str
):
console.print(
f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed "
"step.yml URL; expected a non-empty string"
)
raise typer.Exit(1)
step_yml_url = declared_step_yml_url or info.get("url")
if step_yml_url is None or (
isinstance(step_yml_url, str) and not step_yml_url.strip()
):
console.print(f"[red]Error:[/red] Catalog entry for '{step_id}' has no URL")
raise typer.Exit(1)
if not isinstance(step_yml_url, str):
console.print(
f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed "
"step.yml URL; expected a non-empty string"
)
raise typer.Exit(1)
# Derive __init__.py URL: replace trailing step.yml with __init__.py
# or use explicit init_url if provided.
init_url = info.get("init_url")
if init_url is not None and (
not isinstance(init_url, str) or not init_url.strip()
):
console.print(
f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed "
"__init__.py URL; expected a non-empty string"
)
raise typer.Exit(1)
if not init_url:
if step_yml_url.endswith("step.yml"):
init_url = step_yml_url[: -len("step.yml")] + "__init__.py"
@@ -2676,6 +2707,41 @@ def workflow_step_add(
)
raise typer.Exit(1)
# Preflight the declared file count before creating a staging directory or
# issuing any request. The two required files are always part of the package;
# duplicate declarations for them in extra_files are ignored below and do
# not count twice.
extra_files = info.get("extra_files")
if extra_files is not None and not isinstance(extra_files, dict):
console.print(
"[yellow]Warning:[/yellow] Catalog entry 'extra_files' is not a mapping; "
"additional package files will not be downloaded."
)
extra_files = {}
def _is_required_package_file(rel_path: object) -> bool:
"""Match portable path/case aliases of the two required package files."""
if not isinstance(rel_path, str):
return False
parts = PurePosixPath(rel_path.replace("\\", "/")).parts
return len(parts) == 1 and parts[0].casefold() in {
"step.yml",
"__init__.py",
}
declared_extra_count = sum(
1
for rel_path in (extra_files or {})
if not _is_required_package_file(rel_path)
)
package_file_count = 2 + declared_extra_count
if package_file_count > _MAX_STEP_PACKAGE_FILES:
console.print(
f"[red]Error:[/red] Step package declares {package_file_count} files, "
f"exceeding the {_MAX_STEP_PACKAGE_FILES}-file limit"
)
raise typer.Exit(1)
from specify_cli.authentication.http import open_url as _open_url
def _safe_fetch(url: str) -> bytes:
@@ -2732,6 +2798,14 @@ def workflow_step_add(
console.print(f"[red]Error:[/red] Failed to download step files: {exc}")
raise typer.Exit(1)
package_bytes = len(step_yml_content) + len(init_py_content)
if package_bytes > _MAX_STEP_PACKAGE_BYTES:
console.print(
f"[red]Error:[/red] Step package exceeds the "
f"{_MAX_STEP_PACKAGE_BYTES}-byte total size limit"
)
raise typer.Exit(1)
# Validate step.yml
try:
import yaml as _yaml
@@ -2776,13 +2850,6 @@ def workflow_step_add(
# relative-path → URL. step.yml and __init__.py are ignored here (already
# written). Paths are validated to stay within the step package directory to
# prevent path-traversal attacks.
extra_files = info.get("extra_files")
if extra_files is not None and not isinstance(extra_files, dict):
console.print(
"[yellow]Warning:[/yellow] Catalog entry 'extra_files' is not a mapping; "
"additional package files will not be downloaded."
)
extra_files = {}
for rel_path, file_url in (extra_files or {}).items():
if not isinstance(rel_path, str) or not rel_path.strip():
console.print(
@@ -2790,7 +2857,7 @@ def workflow_step_add(
"empty or non-string path key"
)
raise typer.Exit(1)
if rel_path in ("step.yml", "__init__.py"):
if _is_required_package_file(rel_path):
continue # already written above
# Reject dot-path segments ('', '.', '..') that would refer to the
# package directory itself (IsADirectoryError) or escape it.
@@ -2826,6 +2893,13 @@ def workflow_step_add(
f"[red]Error:[/red] Failed to download extra file '{rel_path}': {exc}"
)
raise typer.Exit(1)
package_bytes += len(file_content)
if package_bytes > _MAX_STEP_PACKAGE_BYTES:
console.print(
f"[red]Error:[/red] Step package exceeds the "
f"{_MAX_STEP_PACKAGE_BYTES}-byte total size limit"
)
raise typer.Exit(1)
try:
dest.parent.mkdir(parents=True, exist_ok=True)
dest.write_bytes(file_content)

32
src/specify_cli/workflows/catalog.py
View File

@@ -22,6 +22,8 @@ from typing import Any
import yaml
from .._download_security import MAX_JSON_CATALOG_BYTES, read_response_limited
# ---------------------------------------------------------------------------
# Errors
@@ -308,7 +310,8 @@ class WorkflowCatalog:
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
_ = parsed.port
except (TypeError, ValueError):
raise WorkflowValidationError(
f"Catalog URL is malformed: {url}"
) from None
@@ -505,7 +508,8 @@ class WorkflowCatalog:
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
_ = parsed.port
except (TypeError, ValueError):
raise WorkflowCatalogError(
f"Refusing to fetch catalog from malformed URL: {url}"
) from None
@@ -538,7 +542,14 @@ class WorkflowCatalog:
entry.url, timeout=30, redirect_validator=_validate_redirect
) as resp:
_validate_catalog_url(resp.geturl())
data = json.loads(resp.read().decode("utf-8"))
data = json.loads(
read_response_limited(
resp,
max_bytes=MAX_JSON_CATALOG_BYTES,
error_type=WorkflowCatalogError,
label="workflow catalog",
).decode("utf-8")
)
except Exception as exc:
# Fall back to cache if available
if cache_file.exists():
@@ -982,7 +993,8 @@ class StepCatalog:
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
_ = parsed.port
except (TypeError, ValueError):
raise StepValidationError(
f"Catalog URL is malformed: {url}"
) from None
@@ -1178,7 +1190,8 @@ class StepCatalog:
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
_ = parsed.port
except (TypeError, ValueError):
raise StepCatalogError(
f"Refusing to fetch catalog from malformed URL: {url}"
) from None
@@ -1211,7 +1224,14 @@ class StepCatalog:
entry.url, timeout=30, redirect_validator=_validate_redirect
) as resp:
_validate_url(resp.geturl())
data = json.loads(resp.read().decode("utf-8"))
data = json.loads(
read_response_limited(
resp,
max_bytes=MAX_JSON_CATALOG_BYTES,
error_type=StepCatalogError,
label="step catalog",
).decode("utf-8")
)
except Exception as exc:
if cache_safe and cache_file.exists():
try: