harden: safe ZIP extraction, checksums, and bounded reads for extension/preset downloads#3141
Open
PascalThuet wants to merge 2 commits into
Open
harden: safe ZIP extraction, checksums, and bounded reads for extension/preset downloads#3141PascalThuet wants to merge 2 commits into
PascalThuet wants to merge 2 commits into
Conversation
Add a shared _download_security module (read_response_limited, is_https_or_localhost_http, size constants) and route the GitHub release and Azure DevOps token network reads through bounded reads so an oversized response can't exhaust memory. Add a strict_redirects mode to authentication.open_url: the redirect handler now rejects any redirect whose target isn't HTTPS (or HTTP to localhost), composing with the existing per-hop redirect_validator and auth-stripping. The Azure DevOps token POST is routed through that handler so a 307/308 cannot forward the client_secret body to a non-HTTPS host.
…on/preset downloads Extend _download_security with safe_extract_zip (path + symlink + per-member and total size + entry-count bounds), read_zip_member_limited, and verify_sha256, and adopt them across the extension and preset install paths: - install_from_zip extracts via safe_extract_zip (zip-slip + symlink + zip-bomb defense) instead of the previous path-only containment check. - catalog JSON fetches and package downloads use bounded reads; the inline manifest pre-read in extension update uses read_zip_member_limited. - downloaded packages are verified against their catalog sha256 when present. - CatalogStackBase and the preset catalog validator reject hostless URLs before the scheme check. The command-registration path-traversal guard the original change carried is already on main (github#3088) and is intentionally omitted.
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
1 participant
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.
Part of splitting #2442 into smaller, dedicated PRs. Re-derived against current
mainand adapted to theextensions/package layout (the original PR predated the module→package refactor #3014).What
src/specify_cli/_download_security.pywithsafe_extract_zip(path-traversal + symlink + per-member/total-size + entry-count bounds),read_zip_member_limited, andverify_sha256.extensions/__init__.py+presets/__init__.py:install_from_zipnow extracts viasafe_extract_zip(zip-slip + symlink + zip-bomb defense) instead of the path-only containment check; catalog fetches and package downloads use bounded reads; downloads are checked against their catalogsha256when present.extensions/_commands.py: the inlineextension.ymlmanifest pre-read (before extraction) usesread_zip_member_limitedso a crafted manifest can't zip-bomb memory.catalogs.py(CatalogStackBase) + the preset catalog validator: reject hostless URLs before the scheme check.Deliberately omitted
The command-registration path-traversal guard the original #2442 change carried is already on
main(landed via #3088, and more thoroughly) — soagents.py/test_registrar_path_traversal.pyare not touched here.Validation
test_download_security.py,test_extensions.py,test_presets.py,integrations/test_integration_catalog.py— 757 passed.ruff checkclean on all changed files.integrations/test_integration_catalog.pycarries a one-line param fix because it asserts the sharedCatalogStackBasebehavior changed here (the rest of that file's bounded-read work is a later PR).Catalog bounded-reads for the workflow/integration/bundler readers, and the tree-wide unbounded-read gate, come in the next PR.