fix: bound response reads in extension catalog and download - #3775
fix: bound response reads in extension catalog and download#3775Quratulain-bilal wants to merge 2 commits into
Conversation
Replace unbounded esponse.read() calls with ead_response_limited() from _download_security in extensions/__init__.py to prevent denial- of-service via oversized catalog or extension archive responses. Three call sites fixed: - _fetch_single_catalog JSON read (catalog metadata) - _fetch_catalog JSON read (legacy path) - download_extension ZIP read (binary download) All existing mock tests updated to use side_effect with BytesIO.read instead of eturn_value, ensuring compatibility with the chunked read loop in ead_response_limited. Two regression tests added: - test_oversized_catalog_response_rejected - test_oversized_extension_download_rejected
There was a problem hiding this comment.
Pull request overview
Bounds extension catalog and archive response reads to reduce memory-exhaustion risks.
Changes:
- Applies shared size-limited response reading.
- Updates HTTP mocks for chunked reads.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Adds bounded catalog and archive reads. |
tests/test_extensions.py |
Updates response mocks for repeated reads. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
src/specify_cli/extensions/init.py:3356
- Oversized catalog responses use the helper's default label, so this path raises
ExtensionError: download exceeds maximum size ...while fetching catalog metadata. Pass a catalog-specific label so the new failure tells users which response was rejected.
read_response_limited(response, max_bytes=MAX_JSON_METADATA_BYTES, error_type=ExtensionError).decode("utf-8")
src/specify_cli/extensions/init.py:3546
- The legacy catalog path also inherits the default
"download"label, producing a misleading error when catalog metadata exceeds the limit. Use a catalog-specific label here as well.
read_response_limited(response, max_bytes=MAX_JSON_METADATA_BYTES, error_type=ExtensionError).decode("utf-8")
src/specify_cli/extensions/init.py:3356
- Decoding before
json.loadschanges the catalog parser's existing behavior: the previous bytes input letjson.loadsdetect UTF-8 BOM and UTF-16/32 JSON, whereas decoding as plain UTF-8 rejects those previously accepted catalog responses. Pass the bounded bytes directly tojson.loadsso this security fix does not also introduce an encoding compatibility break.
catalog_data = json.loads(
read_response_limited(response, max_bytes=MAX_JSON_METADATA_BYTES, error_type=ExtensionError).decode("utf-8")
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Medium
| catalog_data = json.loads( | ||
| read_response_limited(response, max_bytes=MAX_JSON_METADATA_BYTES, error_type=ExtensionError).decode("utf-8") |
There was a problem hiding this comment.
Both regression tests already exist and pass: test_oversized_catalog_response_rejected (line 5441) supplies 1024 bytes with MAX_JSON_METADATA_BYTES=512 and asserts ExtensionError. test_oversized_extension_download_rejected (line 5487) does the same for MAX_DOWNLOAD_BYTES. Tests verified passing after latest push.
| download_url, timeout=60, extra_headers=extra_headers | ||
| ) as response: | ||
| zip_data = response.read() | ||
| zip_data = read_response_limited(response, max_bytes=MAX_DOWNLOAD_BYTES, error_type=ExtensionError) |
There was a problem hiding this comment.
Addressed: test_oversized_extension_download_rejected at line 5487 sets MAX_DOWNLOAD_BYTES=512, serves 1024 bytes, and asserts ExtensionError. Verified passing.
| catalog_data = json.loads( | ||
| read_response_limited(response, max_bytes=MAX_JSON_METADATA_BYTES, error_type=ExtensionError).decode("utf-8") | ||
| ) |
There was a problem hiding this comment.
Fixed: removed .decode(utf-8) from both call sites (lines 3356 and 3546). json.loads() accepts bytes directly, so the bounded read result is passed as-is, preserving compatibility with BOM-bearing or UTF-16/32 catalogs.
json.loads accepts bytes directly. Removing .decode maintains compatibility with BOM-bearing or UTF-16/32 catalogs.
Summary
Replace unbounded
esponse.read() calls with
ead_response_limited() from _download_security in extensions/init.py to prevent denial-of-service via oversized catalog or extension archive responses.
Changes
Three call sites fixed in \extensions/init.py:
Test updates in \ ests/test_extensions.py:
eturn_value, ensuring compatibility with the chunked read loop in
ead_response_limited\
Motivation
Without bounded reads, a malicious catalog server could send an arbitrarily large JSON response causing the CLI to exhaust memory. Similarly, a compromised extension archive endpoint could serve a multi-gigabyte payload. The
ead_response_limited\ utility already exists in _download_security\ and is used by the integrations and workflow catalog modules - this PR extends the same protection to the extensions module.