Repository navigation
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
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
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 |
|
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)
|
Please address Copilot feedback and resolve conflicts |
Reject files exceeding MAX_ZIP_MEMBER_BYTES (10 MiB) before archiving, bounding the actual read to prevent a TOCTOU bypass of the size check. Build into a temporary sibling file and atomically replace the final artifact only after every member passes validation, so a rejected member never leaves a partial/corrupt archive at the output path; the staging file is cleaned up on any failure.
…lection mkstemp() creates the staging file 0600 and os.replace() preserves that mode, so every rebuilt artifact became owner-only; chmod the staging file before replacing (preserving an existing artifact's mode on rebuild, otherwise 0644). Also exclude leftover mkstemp staging files (<id>-<version>-<random>.tmp) from _collect_files so a killed build's partial archive is never re-packaged when out_dir is the bundle source tree.
2888599 to
2619a89
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Staging-file filtering is incorrect in several cases, and manifest loading remains unbounded.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (2)
|
Please address Copilot feedback |
The member loop's size check runs after BundleManifest.from_file() has
already read bundle.yml in full, so an oversized manifest still reached
the YAML parser. load_yaml() now takes an optional max_bytes and reads
through a bounded byte read, BundleManifest.from_file() forwards it, and
build_bundle() passes MAX_ZIP_MEMBER_BYTES for the manifest.
Regression coverage: an oversized bundle.yml must be rejected at the
manifest read, identified by the manifest's full path in the message.
Before this change that test failed with the member-loop message
("Bundle file bundle.yml exceeds ...") only after the unbounded parse had
already run.
Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
| if artifact_path.exists(): | ||
| os.chmod(tmp_path, artifact_path.stat().st_mode & 0o777) | ||
| else: | ||
| os.chmod(tmp_path, 0o644) |
| if staging_re is not None and staging_re.match(name): | ||
| # A leftover packager staging file — never re-package it. | ||
| continue |
| content = fh.read(MAX_ZIP_MEMBER_BYTES + 1) | ||
| if len(content) > MAX_ZIP_MEMBER_BYTES: |
|
Please address Copilot feedback |
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