Skip to content

fix(bundle): wrap local .zip errors in BundlerError instead of raw crash#3713

Open
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/bundle-local-zip-clean-error
Open

fix(bundle): wrap local .zip errors in BundlerError instead of raw crash#3713
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/bundle-local-zip-clean-error

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

The local .zip branch of _local_manifest_source opens the archive and parses its embedded bundle.yml with no error handling:

with zipfile.ZipFile(candidate) as archive:      # raw BadZipFile
    ...
data = _yaml.safe_load(io.BytesIO(raw))           # raw YAMLError
  • A .zip-suffixed file that isn't a valid archive → zipfile.BadZipFile.
  • A valid zip whose embedded bundle.yml is malformed → yaml.YAMLError.

Neither is a BundlerError, and bundle_install only does except BundlerError, so both escape to an unhandled traceback instead of the intended clean Error: … + exit 1. Reproduced both on main (4d3a428).

This is a parity gap: the directory and bundle.yml branches go through BundleManifest.from_fileload_yaml, which wraps failures in BundlerError; and the remote sibling _download_remote_manifest explicitly wraps its _local_manifest_source call in try/except → BundlerError. Only the direct local .zip branch is unguarded.

Fix

Wrap both failure modes in BundlerError (hardening the source function, so every caller benefits):

try:
    archive = zipfile.ZipFile(candidate)
except (zipfile.BadZipFile, OSError) as exc:
    raise BundlerError(f"Artifact '{candidate}' is not a valid .zip bundle: {exc}") from exc
with archive:
    ... # existing KeyError guard for missing bundle.yml
try:
    data = _yaml.safe_load(io.BytesIO(raw))
except _yaml.YAMLError as exc:
    raise BundlerError(f"Artifact '{candidate}' contains an invalid bundle.yml: {exc}") from exc

Valid archives are unaffected — no behaviour change for correct input.

Verification

  • New test_local_source_corrupt_zip_raises_clean_error and test_local_source_zip_with_malformed_bundle_yml_raises_clean_error: fail before (raw BadZipFile / YAMLError), pass after (clean BundlerError).
  • Full test_bundler_local_install.py: 12 passed. ruff clean.

AI-assisted: authored with Claude Code. I reproduced both raw crashes on main via a direct _local_manifest_source call and mirrored the remote-download path's existing error wrapping.

_local_manifest_source's .zip branch called zipfile.ZipFile() and
yaml.safe_load() with no error handling. A .zip-suffixed file that is not a
valid archive raised a raw zipfile.BadZipFile, and a valid zip with a
malformed embedded bundle.yml raised a raw yaml.YAMLError. bundle_install
only does `except BundlerError`, so both escaped to an unhandled traceback
instead of the intended clean `Error: ...` + exit 1.

Wrap both failure modes in BundlerError, mirroring the remote-download
sibling (_download_remote_manifest already wraps its _local_manifest_source
call for exactly this). The directory and bundle.yml branches already report
cleanly via BundleManifest.from_file -> load_yaml; this aligns the .zip
branch. Valid archives are unaffected.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens local bundle ZIP parsing by translating archive and YAML failures into BundlerError.

Changes:

  • Wraps invalid ZIP and malformed YAML errors.
  • Adds regression tests for both cases.
Show a summary per file
File Description
src/specify_cli/commands/bundle/__init__.py Adds error translation for local ZIP manifests.
tests/integration/test_bundler_local_install.py Tests corrupt ZIP and malformed YAML handling.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread src/specify_cli/commands/bundle/__init__.py Outdated
Address review feedback: ZipFile() validates only the central directory, so a
corrupt MEMBER still failed after the open and escaped bundle_install's
`except BundlerError` as a traceback.

Verified both failure modes on a directory-intact archive: a flipped byte in a
stored member raises zipfile.BadZipFile("Bad CRC-32 for file 'bundle.yml'"), and
a mangled deflate stream raises zlib.error. Neither is an OSError subclass, so
the read boundary lists BadZipFile/OSError/zlib.error/EOFError explicitly (the
review named BadZipFile; zlib.error is the same class of corruption and would
have escaped an OSError-only clause).

Valid archives are unaffected.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali

Copy link
Copy Markdown
Contributor Author

Confirmed and fixed in 6d5f8d2 — you're right that ZipFile() only validates the central directory.

I reproduced both corruption modes on a directory-intact archive:

  • flipped byte in a stored member → zipfile.BadZipFile: Bad CRC-32 for file 'bundle.yml' (exactly your example)
  • mangled deflate stream → zlib.error: Error -3 while decompressing data

Since neither is an OSError subclass, an OSError-only clause would still have leaked the second one, so the read boundary lists (BadZipFile, OSError, zlib.error, EOFError) explicitly. The KeyError → "does not contain a bundle.yml" case is unchanged, and the new test asserts both modes now surface as BundlerError (the CRC case fails before this commit with a raw traceback).

…ied text

Self-review follow-up: the exception sets were still incomplete, so a raw
exception could escape bundle_install's `except BundlerError` after all.
Verified against CPython's zipfile:

  read()  RuntimeError       -- password-protected artifact (`zip -e`)
          NotImplementedError -- unsupported compression method
  open()  NotImplementedError -- unsupported zip version
          ValueError

None are OSError subclasses, so each is now listed explicitly.

Also escape the interpolated exception text: zipfile embeds bytes taken FROM
THE ARCHIVE in some messages -- BadZipFile("File name in directory 'bundle.yml'
and header b'[/red]abcd' differ.") -- and that text reaches the user through
_fail(), which renders Rich markup. Unescaped, a crafted member name raised
MarkupError instead of reporting the corruption. The user's own typed path is
left as-is, matching the other messages in this module.

Three new tests (encrypted, unsupported compression, crafted member name) all
fail before this commit.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants