Skip to content

test(zip): add regression tests for upload zip validation#42

Merged
anvie merged 1 commit into
anvie:mainfrom
DeryFerd:test/zip-validator-regression-tests
May 21, 2026
Merged

test(zip): add regression tests for upload zip validation#42
anvie merged 1 commit into
anvie:mainfrom
DeryFerd:test/zip-validator-regression-tests

Conversation

@DeryFerd
Copy link
Copy Markdown
Contributor

Bcakground

Plugin and skill uploads go through backend/zip_validator.validate_upload_zip() before extraction. The checks are non-trivial: compressed size limits, per-entry caps, path traversal, extension whitelist, and corrupt-archive handling. Until now that module had no dedicated unit tests, so a refactor or a well-meaning one-line change could weaken install-time defenses without anyone noticing in CI.

This PR adds a focused regression suite. It does not change production code.

Cases covered

Each test builds small archives under a temp directory and calls validate_upload_zip() directly.

Scenario Expected
Typical skill layout (skill.json + .py) Accept
../ in entry name Reject (traversal)
Absolute path (/etc/passwd) Reject
Disallowed extension (.exe) Reject
Empty archive Reject
File that is not a zip Reject

Together these lock in the behaviors plugin/skill routes already rely on. They are cheap to run and fail with clear assertion messages on the error string returned by the validator.

What I don't Fix

  • Cryptographic signing of plugin packages — covered elsewhere (#29).
  • Route-level upload tests (multipart form, auth, disk paths) — could be a follow-up; this PR stays at the validator layer.
  • Zip bomb limits that need hundreds of entries or huge uncompressed totals — not exercised here; existing constants in zip_validator.py are unchanged.

How to verify

  • python -m pytest unit_tests/test_zip_validator.py -q

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Owner

@anvie anvie left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! This is a solid first-pass regression suite.

The six tests lock in the three most critical security checks: path traversal (../), absolute paths (/etc/passwd), and extension whitelisting (.exe blocked). The _write_zip helper keeps things clean, and setUp/tearDown with tempfile.mkdtemp() means no temp artifacts leak.

Good coverage for the happy path, empty zip, and non-zip file checks too. The assertions on error message substrings are pragmatic — more robust than exact matching.

Gaps worth noting (all acknowledged in your PR description):

  • No corrupt-zip test (BadZipFile) — would be easy to add with a truncated file
  • No Windows-style absolute path test (\\evil\\file) — the validator explicitly handles it
  • Size-limit tests (50 MB upload, 500 entries, 200 MB uncompressed) are understandably out of scope

One minor nit: import shutil inside tearDown is functional but a bit unusual — moving it to module level would be cleaner.

Full review report saved at artifacts/pr42-review.md.

Best,
Richard

Robin Syihab's agent.

anvie added a commit that referenced this pull request May 16, 2026
- PR #41 (fix/agent-sandbox-workplace-enforcement): approved — clean fix
  for sandbox_enabled enforcement on remote/cloud workplaces
- PR #42 (test/zip-validator-regression-tests): approved — solid
  first-pass regression suite covering security-critical paths
jeffrysurya pushed a commit to jeffrysurya/evonic that referenced this pull request May 20, 2026
- _generate_pair_code(): static method, 8-char uppercase+digits
- create_pending_approval(): insert record
- get_pending_approvals(): list non-expired by channel
- get_pending_approval_by_code(): lookup non-expired by code
- approve_pending(): add user to allowed_users in config, delete record
- reject_pending(): delete record
- cleanup_expired_approvals(): bulk delete expired
- is_user_allowed(): check allowed_users in restricted mode
@anvie anvie merged commit 7ee87f5 into anvie:main May 21, 2026
2 checks passed
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.

2 participants