fix: enforce size limit on individual files in build_bundle() by Quratulain-bilal · Pull Request #3911 · github/spec-kit

Quratulain-bilal · 2026-07-31T12:57:35Z

Problem

build_bundle() called read_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

Verified normal files are packaged correctly
Verified oversized files are rejected

mnriem

Please resolve conflicts

Copilot

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

Copilot

🟡 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

mnriem · 2026-09-17T10:47:35Z

Please address Copilot feedback

Quratulain-bilal · 2026-09-17T14:19:28Z

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.

mnriem · 2026-09-23T12:29:58Z

Please resolve conflicts

Copilot

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 · 1

Open (2)

Set intentional permissions for generated archives · New
Keep staging archives out of source file collection · New

Resolved since last review (1)

This rejection occurs after the final artifact has already been opened and earlier members may have…

mnriem · 2026-09-23T17:34:53Z

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.

Copilot

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)

Bound manifest reads before parsing · New

Resolved since last review (2)

Set intentional permissions for generated archives
Keep staging archives out of source file collection

mnriem · 2026-09-24T15:58:54Z

Please address Copilot feedback

…read

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)

Copilot

Copilot review overview

🟡 Changes recommended

Fresh artifacts bypass restrictive umasks, and staging-file filtering can silently omit legitimate assets.

Review effort: Balanced
Findings: 2 · 1

Open (3)

Honor umask when setting permissions on fresh artifacts · New
Restrict staging-file filtering to the bundle root · New
Add coverage for bounded reads after asset growth · New

Resolved since last review (1)

Bound manifest reads before parsing

+                    content = fh.read(MAX_ZIP_MEMBER_BYTES + 1)
+                    if len(content) > MAX_ZIP_MEMBER_BYTES:


mnriem · 2026-10-06T19:34:55Z

Please address Copilot feedback

Addresses 3 Copilot findings: 1. Honor umask for fresh artifacts: use 0666 & ~umask instead of unconditional 0644 so restrictive umasks (077) produce private 0600 files while normal umasks yield 0644. 2. Restrict staging-file filtering to the bundle root: staging files are created in the output directory, not arbitrary asset dirs. A nested asset like assets/demo-bundle-1.2.0-backup.tmp is now correctly packaged. 3. Add bounded-read regression test: simulates a TOCTOU race where an asset grows past MAX_ZIP_MEMBER_BYTES between fstat and read. Verifies rejection, previous artifact preservation, and temp file cleanup. Tests added: - test_fresh_artifact_honors_restrictive_umask - test_nested_staging_like_asset_is_packaged - test_read_is_bounded_when_asset_grows_after_fstat Assisted-by: GitHub Copilot (autonomous)

Quratulain-bilal · 2026-10-11T01:15:17Z

Hi @mnriem — I've addressed all 3 Copilot findings:

1. Honor umask for fresh artifacts (Medium)
Changed os.chmod(tmp_path, 0o644) to os.chmod(tmp_path, 0o666 & ~umask) so restrictive umasks (077) produce private 0600 files while normal umasks (022) yield the expected 0644. Added \ est_fresh_artifact_honors_restrictive_umask.

2. Restrict staging-file filtering to bundle root (Medium)
Changed the staging filter to only apply when
oot_path == bundle_dir, so legitimate nested assets like \�ssets/demo-bundle-1.2.0-backup.tmp\ are correctly packaged. Added \ est_nested_staging_like_asset_is_packaged.

3. Add coverage for bounded reads after asset growth (Low)
Added \ est_read_is_bounded_when_asset_grows_after_fstat\ that simulates a TOCTOU race — an asset grows past MAX_ZIP_MEMBER_BYTES between fstat and read. Verifies rejection, previous artifact preservation, and temp file cleanup.

All 24 tests pass. Ready for re-review when you get a chance.

Copilot

🟡 Changes recommended

The staging filter can omit valid assets, and process-wide umask handling introduces a permissions race.

3 open findings

2 resolved since last review

Previously missed (1)

In code that hasn't changed since last review

Replace invalid characters with em dashes

src/specify_cli/bundles/yamlio.py:46

Replace the two invalid replacement characters with em dashes so this docstring remains readable.

🧠 Review effort: Balanced

+            umask = os.umask(0)
+            os.umask(umask)  # restore immediately


+    bundle = _make_bundle(tmp_path / "b")
+    result = build_bundle(bundle, output_dir=tmp_path / "out")
+    assert stat.S_IMODE(result.artifact_path.stat().st_mode) == 0o644


mnriem · 2026-10-12T15:44:45Z

Please address Copilot feedback and resolve conflicts

…read # Conflicts: # src/specify_cli/bundles/manifest.py

Quratulain-bilal requested a review from mnriem as a code owner July 31, 2026 12:57

mnriem requested changes Aug 10, 2026

View reviewed changes

Quratulain-bilal force-pushed the fix/bundle-unbounded-read branch from 0724b00 to 9f59bfb Compare August 10, 2026 21:38

mnriem requested a balanced review from Copilot August 17, 2026 21:08

Copilot started reviewing on behalf of mnriem August 17, 2026 21:09 View session

Copilot AI reviewed Aug 17, 2026

View reviewed changes

Comment thread src/specify_cli/bundler/services/packager.py Outdated

mnriem added the triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate label Sep 11, 2026

mnriem requested a balanced review from Copilot September 11, 2026 17:47

Copilot started reviewing on behalf of mnriem September 11, 2026 17:48 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread src/specify_cli/bundler/services/packager.py Outdated

mnriem requested a balanced review from Copilot September 23, 2026 12:29

Copilot started reviewing on behalf of mnriem September 23, 2026 12:30 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread src/specify_cli/bundles/packager.py

Comment thread src/specify_cli/bundles/packager.py

Quratulain-bilal added 2 commits September 23, 2026 22:51

Quratulain-bilal force-pushed the fix/bundle-unbounded-read branch from 2888599 to 2619a89 Compare September 23, 2026 17:53

mnriem requested a balanced review from Copilot September 24, 2026 11:03

Copilot started reviewing on behalf of mnriem September 24, 2026 11:04 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread src/specify_cli/bundles/packager.py

Quratulain-bilal added 2 commits October 6, 2026 22:35

Merge remote-tracking branch 'origin/main' into fix/bundle-unbounded-…

faada12

…read

mnriem requested a balanced review from Copilot October 6, 2026 18:56

Copilot started reviewing on behalf of mnriem October 6, 2026 18:57 View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread src/specify_cli/bundles/packager.py Outdated

Comment thread src/specify_cli/bundles/packager.py Outdated

Comment thread src/specify_cli/bundles/packager.py

Comment on lines +139 to +140

content = fh.read(MAX_ZIP_MEMBER_BYTES + 1)

if len(content) > MAX_ZIP_MEMBER_BYTES:

mnriem requested a balanced review from Copilot October 12, 2026 12:55

Copilot started reviewing on behalf of mnriem October 12, 2026 12:55 View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/bundle-unbounded-…

9ea5347

…read # Conflicts: # src/specify_cli/bundles/manifest.py

mnriem requested a balanced review from Copilot October 13, 2026 11:56

		content = fh.read(MAX_ZIP_MEMBER_BYTES + 1)
		if len(content) > MAX_ZIP_MEMBER_BYTES:

Conversation

Quratulain-bilal commented Jul 31, 2026

Problem

Fix

Testing

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

mnriem commented Sep 17, 2026

Uh oh!

Quratulain-bilal commented Sep 17, 2026

Uh oh!

mnriem commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mnriem commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

mnriem commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 6, 2026

Uh oh!

Quratulain-bilal commented Oct 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

mnriem commented Oct 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants