harden: secure extension and preset archive downloads - #3141
Open
PascalThuet wants to merge 7 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens extension/preset/catalog download and extraction paths by introducing bounded reads, optional SHA-256 verification, and safer ZIP handling, then wiring those protections into the extension/preset install flows and associated tests.
Changes:
- Add
_download_securityhelpers for strict URL validation, bounded HTTP reads, SHA-256 verification, bounded ZIP-member reads, and safe ZIP extraction (zip-slip/zip-bomb/symlink defenses). - Update extension and preset download/install flows to use bounded reads, checksum verification (when
sha256is present), andsafe_extract_zip. - Update auth redirect handling and tests to reflect strict redirect enforcement and stream-like
read(size)semantics.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/_download_security.py |
New security primitives: bounded reads, ZIP extraction/member limits, checksum verification, scheme/host predicate. |
src/specify_cli/authentication/http.py |
Strict redirect validation integrated into redirect handler and exposed via open_url(..., strict_redirects=...). |
src/specify_cli/authentication/azure_devops.py |
Bounded token-response reads and strict redirect protection for client-credentials POST. |
src/specify_cli/_version.py |
Latest-release metadata fetch now uses bounded reads and strict redirects. |
src/specify_cli/_github_http.py |
GitHub release metadata resolution now uses bounded reads to avoid unbounded JSON loads. |
src/specify_cli/catalogs.py |
Catalog URL validation rejects hostless URLs before scheme checks. |
src/specify_cli/extensions/__init__.py |
Extension ZIP installs use safe_extract_zip; catalog and ZIP downloads are bounded and optionally checksum-verified. |
src/specify_cli/extensions/_commands.py |
Extension manifest pre-read from ZIP is now bounded (read_zip_member_limited). |
src/specify_cli/presets/__init__.py |
Preset ZIP installs use safe_extract_zip; catalog and ZIP downloads are bounded and optionally checksum-verified. |
src/specify_cli/presets/_commands.py |
Shared URL predicate for download/redirect validation; preset ZIP downloads use bounded reads and strict redirects for GitHub metadata resolution. |
tests/test_download_security.py |
New tests covering bounded reads, checksum verification, safe ZIP extraction, and bounded ZIP member reads. |
tests/test_authentication.py |
Tests updated for strict redirect rejection and bounded read semantics; Azure AD token tests patch opener behavior. |
tests/test_github_http.py |
Tests updated for stream-like reads; add multi-hop GitHub redirect auth-preservation test. |
tests/http_helpers.py |
Stream-backed read() mocks and an autouse fixture to route build_opener().open() through patched urlopen. |
tests/test_upgrade.py |
Add regression test asserting _fetch_latest_release_tag reads via read_response_limited(max_bytes=...). |
tests/self_upgrade_helpers.py |
Re-export the opener-routing fixture for self-upgrade test modules. |
tests/test_self_upgrade_detection.py |
Enable opener-routing fixture to keep existing urlopen patches effective. |
tests/test_self_upgrade_execution.py |
Enable opener-routing fixture to keep existing urlopen patches effective. |
tests/test_self_upgrade_verification.py |
Enable opener-routing fixture to keep existing urlopen patches effective. |
tests/test_extensions.py |
Stream-like read(size) mocks and one assertion update for host-before-scheme URL validation. |
tests/test_presets.py |
Stream-like read(size) mocks and open_url fake signature updated for strict_redirects. |
tests/integrations/test_integration_catalog.py |
One assertion updated for host-before-scheme URL validation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/22 changed files
- Comments generated: 2
mnriem
marked this pull request as draft
June 24, 2026 18:29
Collaborator
|
Converting to draft as #3140 has to land first |
Adopt the shared download-security primitives from github#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)
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)
PascalThuet
force-pushed
the
split/extension-preset-downloads
branch
from
July 23, 2026 21:49
99773ba to
b52570e
Compare
PascalThuet
changed the title
PascalThuet
marked this pull request as ready for review
July 23, 2026 21:50
Contributor
Author
|
Rebased this PR onto current The refreshed diff removes the duplicated #3140 history, retains the shared redirect/checksum behavior from Validation on the published tree: full Python 3.12 suite Posted on behalf of @PascalThuet by OpenAI Codex (model: GPT-5). |
Comment on lines
+1254
to
+1257
| for name in namelist: | ||
| normalized_name = name.replace("\\", "/") | ||
| parts = normalized_name.split("/") | ||
| path_key = portable_zip_path_key(normalized_name) |
| reserved_stem = part.partition(".")[0].partition(":")[0].rstrip(" ") | ||
| if ( | ||
| len(part.encode("utf-8")) > MAX_ZIP_COMPONENT_BYTES | ||
| or any(ord(character) < 32 for character in part) |
mnriem
requested changes
Jul 24, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback. Also what about workflow, workflow steps and bundle downloads? Do they need this too?
Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
Comment on lines
+537
to
+544
| filename_size, extra_size, comment_size = struct.unpack_from( | ||
| "<HHH", header, 28 | ||
| ) | ||
| variable_size = filename_size + extra_size + comment_size | ||
| record_size = _ZIP_CENTRAL_HEADER_SIZE + variable_size | ||
| if record_size > remaining: | ||
| _raise(error_type, f"Invalid ZIP archive: {zip_path}") | ||
| archive_file.seek(variable_size, 1) |
Collaborator
|
Please address Copilot feedback and fix test & lint errors |
Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
Comment on lines
+1299
to
+1302
| if manifest_path is not None: | ||
| parsed_manifest = yaml.safe_load( | ||
| read_zip_member_limited(zf, manifest_path) | ||
| ) |
Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
mnriem
requested changes
Jul 27, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/specify_cli/_download_security.py:472
- Checking only equality with version 4.5 does not reliably reject ZIP64. A ZIP64 data-descriptor archive can change both “version needed” fields from 45 to 46 while retaining the 64-bit descriptor;
ZipFileaccepts it, and the preflight no longer sees any ZIP64 indicator after the sentinels/extra field are masked. Since supported STORE/DEFLATE ZIP32 entries require versions below 4.5, reject version-needed values at or above 45.
if extract_version == _ZIP64_EXTRACT_VERSION:
_raise_zip64(error_type)
src/specify_cli/extensions/_commands.py:1316
- Root selection uses an exact case-sensitive string even though candidate identity is case-folded. An archive containing
EXTENSION.YMLat the root plus a validrepo/extension.ymltherefore preflights the nested manifest, but after extractioninstall_from_zip()finds the root file asextension.ymlon a case-insensitive filesystem and installs that different, unvalidated manifest. Select/reject root aliases using the portable key and ensure installation uses the same selected member on every filesystem.
root_manifest = next(
(
name
for name, normalized_name, _path_key
in manifest_candidates
if normalized_name == "extension.yml"
),
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Medium
Comment on lines
1518
to
1522
| new_registry_entry = manager.registry.get(extension_id) | ||
| if new_registry_entry is None or not isinstance(new_registry_entry, dict): | ||
| new_registered_commands = {} | ||
| else: | ||
| new_registered_commands = new_registry_entry.get("registered_commands", {}) |
Comment on lines
+1375
to
+1377
| candidate_root = skills_candidate.resolve() | ||
| skill_subdir = (skills_candidate / skill_name).resolve() | ||
| skill_subdir.relative_to(candidate_root) |
mnriem
requested changes
Jul 27, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback and fix test & lint errors
Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Originally rebased onto
mainat4d3a428. The published head now includes a validated merge of currentmain(0117a7b). #3140 and #3671 are merged, so this PR reuses their shared URL, redirect, bounded-read, and checksum primitives instead of carrying the #3140 commit.What
ZipFilematerializes entries, including declared/actual entry counts, a central-directory size cap, and archive-level or per-entry ZIP64 indicators.Security and compatibility boundaries
ZipFilebecause the standard-library read path cannot enforce the requested decompressed-output bound for those methods.Validation
ab2a825,mainat0117a7b):5641 passed, 172 skipped.5427 passed, 166 skipped.793 passed.uvx ruff@0.15.0 check src tests: clean on the published merged head.git diff --check: clean on the published merged head.Deliberately out of scope
Authorship disclosure: updated on behalf of @PascalThuet by OpenAI Codex (model: GPT-5).