fix: enforce size limit on individual files in build_bundle() - #3911
Quratulain-bilal wants to merge 4 commits into
Conversation
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please resolve conflicts
build_bundle() called read_bytes() without any size guard. A single large asset file could exhaust memory. Enforce MAX_ZIP_MEMBER_BYTES (10 MiB) limit before reading.
Enforce MAX_ZIP_MEMBER_BYTES (10 MiB) limit before reading each file via os.fstat() on the same file descriptor used for reading, avoiding a TOCTOU gap between stat() and read_bytes(). Add regression tests: oversized file (>limit) is rejected, file at exact limit is accepted. Co-authored-by: GitHub Copilot (model: mimo-v2.5-free, supervised)
0724b00 to
9f59bfb
Compare
There was a problem hiding this comment.
Pull request overview
Adds per-file size enforcement when building bundle archives.
Changes:
- Rejects files exceeding the 10 MiB member limit.
- Adds oversized and boundary-size tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/bundler/services/packager.py |
Adds file-size validation before archive writes. |
tests/unit/test_bundler_packager.py |
Tests rejection and exact-limit acceptance. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
fstat() is only a snapshot; another process can append to the file after the check. Read MAX_ZIP_MEMBER_BYTES + 1 bytes and validate the length so the limit cannot be bypassed by a concurrent writer.
mnriem
added
the
triage-can-wait
There was a problem hiding this comment.
🟡 Changes recommended
Rejection can leave a partial archive at the final output path and overwrite a previous valid artifact.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Please address Copilot feedback |
…ction Addresses Copilot feedback: when a file exceeds MAX_ZIP_MEMBER_BYTES, the rejection now occurs after writing to a temporary sibling file. The final artifact path is only replaced atomically after all members pass validation. This prevents a partial/corrupt archive from being left at the output path. Also cleans up the temp file on any failure (exception, interrupt). Changes: - Build into tempfile.mkstemp() sibling in output_dir - os.replace() only after all members written successfully - BaseException handler removes temp file on any failure - Added test_oversized_file_does_not_corrupt_output - Added test_temp_file_cleaned_up_on_failure Assisted-by: GitHub Copilot (autonomous)
|
Hi @mnriem — I've addressed Copilot's feedback: builds now write to a temporary sibling file first, and the final artifact path is only replaced atomically after every member passes validation. This prevents a partial/corrupt archive from being left at the output path if a file exceeds the size limit. The temp file is cleaned up on any failure. Added est_oversized_file_does_not_corrupt_output and est_temp_file_cleaned_up_on_failure to verify the atomic behavior. All 19 tests pass. Ready for re-review when you get a chance. |
|
Please resolve conflicts |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Archive permissions and staging-file handling introduce unresolved blocking issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
Resolved since last review (1)
| tmp_fd, tmp_path_str = tempfile.mkstemp( | ||
| suffix=".tmp", prefix=f"{manifest.bundle.id}-{manifest.bundle.version}-", dir=str(out_dir) | ||
| ) |
| tmp_fd, tmp_path_str = tempfile.mkstemp( | ||
| suffix=".tmp", prefix=f"{manifest.bundle.id}-{manifest.bundle.version}-", dir=str(out_dir) | ||
| ) |
Problem
build_bundle()calledread_bytes()without any size guard. A single large asset file could exhaust memory.Fix
Enforce
MAX_ZIP_MEMBER_BYTES(10 MiB) limit before reading each file.Testing