Validate model zip members before extraction

[massy-o]

Summary

• validate model zip member paths before extraction
• reject zip entries that would resolve outside the intended model directory

Why

Model initialization extracts a downloaded zip archive into the local model folder. Validating each member path before extraction prevents archive entries with parent-directory or absolute-path components from writing outside the destination directory.

Testing

• python3 -m py_compile Prj-Python/hyperlpr3/config/configuration.py
• git diff --check

[massy-o]

Self-review: Resolving both the destination root and each candidate member path before extraction blocks parent-directory and absolute-path entries while still allowing normal nested model files.

[devloai]

PR Summary:

• Adds a safe_extract_zip helper that validates each zip member's resolved path before extraction.
• Replaces the raw f.extract(file, save_path) loop in down_model_zip with the new helper.
• Guards against zip-slip attacks (path traversal via ../ or absolute paths in member names).

Review Summary:

The security intent is sound, but there are two implementation issues. First, the validation and extraction happen in the same loop, so if a malicious entry is found at position N, entries 0..N-1 are already written to disk before the RuntimeError is raised — the attack is partially effective. This needs a two-pass approach: validate all members, then extract. Second, the ZipFile object is opened without a with statement and is never explicitly closed; a context manager is needed to ensure the handle is released on both happy and error paths.

Suggestions

• Add a check for symlink members (ZipInfo.external_attr) since a zip entry that is a symlink pointing outside the root bypasses the current realpath-based guard. Apply

[devloai]

Validation and extraction are interleaved in the same loop. If a malicious member path is encountered at position N, members 0..N-1 have already been extracted before the RuntimeError fires. The security intent is broken.

Fix by splitting into two passes — validate all members first, then extract:

def safe_extract_zip(zip_file, save_path):
    root = os.path.realpath(save_path)
    for member in zip_file.namelist():
        target_path = os.path.realpath(os.path.join(save_path, member))
        if target_path != root and not target_path.startswith(root + os.sep):
            raise RuntimeError(f"Blocked unsafe zip member path: {member}")
    for member in zip_file.namelist():
        zip_file.extract(member, save_path)

actions

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[devloai]

The ZipFile object f is opened but never closed — there's no with block or f.close(). If safe_extract_zip raises, the file handle leaks and the zip file may remain locked (especially on Windows).

Replace the open + extract block with a context manager:

    if is_unzip:
        with zipfile.ZipFile(name, "r") as f:
            safe_extract_zip(f, save_path)
        os.remove(name)

actions

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[massy-o]

Self-review: This now validates every ZipInfo entry before calling extractall, so a later malicious member cannot leave earlier files partially extracted; symlink entries are also rejected before extraction.

[massy-o]

Addressed the review feedback: validation now runs over all ZipInfo entries before extraction, the ZipFile is opened with a context manager, and zip symlink entries are rejected before extractall.