Skip to content

test(security): add unit tests for network_guard and zip_safe modules#1395

Merged
qin-ctx merged 2 commits intovolcengine:mainfrom
sjhddh:test/security-module-coverage
Apr 13, 2026
Merged

test(security): add unit tests for network_guard and zip_safe modules#1395
qin-ctx merged 2 commits intovolcengine:mainfrom
sjhddh:test/security-module-coverage

Conversation

@sjhddh
Copy link
Copy Markdown
Contributor

@sjhddh sjhddh commented Apr 12, 2026

Summary

Add comprehensive unit tests for two security-critical modules with zero prior test coverage:

  • network_guard: SSRF protection — tests internal IP blocking, DNS rebinding edge cases, IPv6 mapped addresses, protocol validation, malformed URLs
  • zip_safe: Path traversal prevention — tests Zip Slip attacks, symlink handling, path normalization, special characters

Motivation

These modules protect against OWASP Top 10 vulnerabilities (SSRF, path traversal). Untested security code is a liability — regression tests ensure boundaries aren't accidentally weakened.

Test plan

  • All existing tests still pass
  • New tests cover happy paths and adversarial inputs
  • Edge cases: IPv6, unicode, empty input, malformed data

Add comprehensive test coverage for two security-critical modules
that previously had zero tests:

- network_guard: SSRF protection, internal IP blocking, DNS rebinding
  edge cases, protocol validation, malformed URL handling
- zip_safe: Zip Slip traversal prevention, path normalization,
  special character handling

These modules protect against OWASP Top 10 vulnerabilities (SSRF,
path traversal) and should have regression tests to prevent
accidental weakening of security boundaries.
@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🏅 Score: 95
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add unit tests for network_guard SSRF protection

Relevant files:

  • tests/misc/test_network_guard.py

Sub-PR theme: Add unit tests for zip_safe path traversal protection

Relevant files:

  • tests/misc/test_zip_safe.py

⚡ Recommended focus areas for review

Incomplete Test

The test for CJK filename repair does not include an assertion to verify the filename was actually normalized, leaving the core behavior unvalidated.

def test_repairs_cjk_filename_from_cp437_mojibake(self) -> None:
    """Simulate a CJK filename stored without UTF-8 flag.

    The zip module reads it via cp437 producing mojibake.
    normalize_zip_filenames should re-decode from cp437 -> UTF-8.
    """
    # Create a zip with raw bytes: write CJK UTF-8 bytes as the filename
    # but do NOT set the UTF-8 flag, so Python's zipfile reads it as cp437.
    cjk_name = "测试文件.txt"
    utf8_bytes = cjk_name.encode("utf-8")

    buf = io.BytesIO()
    with zipfile.ZipFile(buf, "w") as zf:
        info = zipfile.ZipInfo(cjk_name)
        info.flag_bits = 0  # no UTF-8 flag
        zf.writestr(info, "content")
    # Manually patch the raw zip to remove UTF-8 flag
    # (Python's ZipFile may set it automatically for non-ASCII)
    raw = buf.getvalue()
    # The flag_bits field is at offset 6 in the local file header
    # This is complex; instead test the logic paths individually
    # by directly calling with a pre-mangled ZipInfo
    buf2 = io.BytesIO()
    with zipfile.ZipFile(buf2, "w") as zf:
        # Encode filename as cp437 representation of the UTF-8 bytes
        try:
            cp437_name = utf8_bytes.decode("cp437")
        except UnicodeDecodeError:
            pytest.skip("Cannot represent CJK UTF-8 bytes in cp437")
        info = zipfile.ZipInfo(cp437_name)
        info.flag_bits = 0
        zf.writestr(info, "content")
    buf2.seek(0)
    with zipfile.ZipFile(buf2, "r") as zf:
        member = zf.infolist()[0]
        # Before normalization, filename should be the mojibake
        assert member.filename == cp437_name
        normalize_zip_filenames(zf)
        # After normalization, the name should be repaired to CJK
        # (only if all heuristic conditions are met)

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

The test_repairs_cjk_filename_from_cp437_mojibake test was missing a
final assertion to verify that normalize_zip_filenames() actually
repaired the mojibake filename back to the original CJK name.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@qin-ctx qin-ctx merged commit b153ad9 into volcengine:main Apr 13, 2026
5 of 6 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants