fix: use bounded read for bundle download HTTP responses - #3764
fix: use bounded read for bundle download HTTP responses#3764Quratulain-bilal wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Bounds bundle HTTP response reads at 50 MiB to prevent memory exhaustion.
Changes:
- Replaces unbounded response reading with the shared limited-read helper.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/commands/bundle/__init__.py |
Enforces the bundle download size limit. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| ) as resp: | ||
| _require_https(f"bundle '{entry_id}'", resp.geturl()) | ||
| raw = resp.read() | ||
| raw = read_response_limited(resp, max_bytes=MAX_DOWNLOAD_BYTES) |
There was a problem hiding this comment.
Added test_bundle_download_rejects_oversized_response which monkeypatches MAX_DOWNLOAD_BYTES to 100, serves 200 bytes via a fake response, and asserts the command fails with exceeds maximum size of 100 bytes. This proves the size-limit bound is retained in the bundle CLI download path.
e5c651b to
b8c293d
Compare
|
Thanks for the review! Regression test added in the latest push: All 32 bundle CLI tests pass. |
|
|
||
| # Must fail with a size-limit error, not an unhandled traceback. | ||
| assert result.exit_code == 1 | ||
| assert "Error:" in result.output or "exceeds" in result.output.lower() |
There was a problem hiding this comment.
Fixed: the assertion now checks for exceeds maximum size of 100 bytes specifically, which only appears when read_response_limited enforces the bound. A plain YAML/schema parse failure would not produce this message.
| from ..._download_security import MAX_DOWNLOAD_BYTES, read_response_limited | ||
|
|
||
| import typer |
There was a problem hiding this comment.
Fixed: moved the from ..._download_security import ... after import typer to match the third-party-before-first-party convention established in commands/init.py:11-15.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
The bundle download used unbounded resp.read() to read HTTP responses into memory. A malicious or misconfigured catalog server could return an arbitrarily large payload causing OOM. Replace with read_response_limited() capped at MAX_DOWNLOAD_BYTES (50 MiB), consistent with how other download paths in the codebase enforce bounded reads. Add regression test that monkeypatches MAX_DOWNLOAD_BYTES to 100 bytes and verifies oversized responses are rejected.
b8c293d to
d222d92
Compare
Summary
The bundle download in
commands/bundle/__init__.pyused unboundedresp.read()to read HTTP responses into memory. A malicious or misconfigured catalog server could return an arbitrarily large payload causing OOM.Changes
src/specify_cli/commands/bundle/__init__.py: Replacedresp.read()withread_response_limited(resp, max_bytes=MAX_DOWNLOAD_BYTES)capped at 50 MiB, consistent with how other download paths in the codebase enforce bounded reads.Testing
All 31 tests in
test_bundle_cli.pypass after the fix.Security Impact
This is a Medium severity fix - it closes a potential memory exhaustion vector against the bundle download endpoint. The 50 MiB limit is appropriate for bundle artifacts (ZIPs, archives) which are larger than JSON metadata but still bounded.