Security fixes, thread safety, and complete data provider actions#1
Security fixes, thread safety, and complete data provider actions#1
Conversation
Security & Safety: - Add QoreRWLock for thread-safe access to ZipFile operations - Add reference counting between ZipFile and streams to prevent use-after-free - Add path traversal validation to prevent Zip Slip attacks - Add configurable max allocation size (ZIP_DEFAULT_MAX_ALLOC_SIZE) Bug Fixes: - Fix toData() to properly close and clean up memory stream - Fix deleteEntry() exception message formatting - Add OpenSSL 3.2+ compatibility patch (ENGINE API removed) ZipDataProvider Improvements: - Complete implementation of all 8 action providers: - ZipCreateArchiveDataProvider: Create archives to file or memory - ZipExtractArchiveDataProvider: Extract archives to directory - ZipListArchiveDataProvider: List archive contents - ZipArchiveInfoDataProvider: Get archive metadata - ZipAddFilesDataProvider: Add files to existing archives - ZipExtractFileDataProvider: Extract single files - ZipCompressDataDataProvider: Compress data in memory - ZipDecompressDataDataProvider: Decompress data in memory - Fix indexOf() -> inlist() for Qore compatibility - Fix getComment() -> comment() method name Build & CI: - Add patches/ directory with OpenSSL 3.2+ compatibility patch - Update CMakeLists.txt to apply patches to minizip-ng at configure time - Add GitLab CI configuration for ubuntu and alpine testing Tests: - Add path traversal security tests - Add active stream lifecycle tests - Add toData() state management tests - Add deleteEntry() tests - Add encryption with password tests - Add comprehensive tests for all 8 data provider actions - Total: 27 test cases, 180 assertions, all passing - Valgrind clean: 0 bytes leaked 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The minizip-ng submodule needs to be fetched for the build to work. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive security improvements, thread safety, and completes the data provider action implementations for the ZIP module. The changes add critical security protections against Zip Slip attacks, introduce thread-safe archive operations with proper reference counting between ZipFile and streams, and complete all 8 ZipDataProvider action providers.
Key Changes
- Added thread-safe access to ZipFile operations using read-write locks with reference counting to prevent use-after-free when streams are active
- Implemented path traversal validation to prevent Zip Slip security vulnerabilities in extraction operations
- Completed implementation of all 8 ZipDataProvider action providers (create, extract, list, info, add, extract-file, compress, decompress) with full request/response type definitions
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/zip.qtest | Added 5 new test cases covering security (path traversal), active stream reference counting, toData state management, delete behavior, and encryption with passwords |
| test/ZipDataProvider.qtest | Added 8 new test cases for all data provider actions with comprehensive validation of request/response handling |
| test/docker_test/test-ubuntu.sh | Updated file existence check to use new source file name QoreZipFile.cpp |
| test/docker_test/test-alpine.sh | Updated file existence check to use new source file name QoreZipFile.cpp |
| src/ZipOutputStream.h | Added parent reference for stream lifecycle management and updated constructor signature |
| src/ZipOutputStream.cpp | Implemented reference counting with parent ZipFile to track active streams |
| src/ZipInputStream.h | Added parent reference for stream lifecycle management and updated constructor signature |
| src/ZipInputStream.cpp | Implemented reference counting with parent ZipFile to track active streams |
| src/QoreZipFile.h | Added thread safety with QoreRWLock, atomic stream counter, max allocation size, and path validation |
| src/QoreZipFile.cpp | Implemented thread-safe operations with locking, path traversal security checks, active stream validation, and proper toData cleanup |
| qlib/ZipDataProvider/ZipDataProvider.qm | Completed all 8 action provider implementations, removed placeholder providers, added comprehensive request/response type definitions |
| patches/openssl-3.2-fix.patch | Added OpenSSL 3.2+ compatibility patch for minizip-ng ENGINE API deprecation |
| CMakeLists.txt | Added patch application logic for OpenSSL 3.2 compatibility |
| .gitlab-ci.yml | Added GIT_SUBMODULE_STRATEGY to ensure submodules are cloned |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/QoreZipFile.cpp
Outdated
|
|
||
| QoreObject* QoreZipFile::openInputStream(const char* name, ExceptionSink* xsink) { | ||
| if (!checkOpen(xsink, false)) { | ||
| QoreAutoRWWriteLocker lock(rwlock); |
There was a problem hiding this comment.
Using write lock for a read operation is incorrect. The openInputStream method only needs a read lock since it's accessing the archive for reading purposes. This will unnecessarily block other threads from performing concurrent read operations. Change QoreAutoRWWriteLocker to QoreAutoRWReadLocker.
| QoreAutoRWWriteLocker lock(rwlock); | |
| QoreAutoRWReadLocker lock(rwlock); |
CMakeLists.txt
Outdated
| # Result 0 = success, 1 = already applied (which is fine) | ||
| if(PATCH_RESULT GREATER 1) | ||
| message(WARNING "Failed to apply OpenSSL 3.2 compatibility patch") |
There was a problem hiding this comment.
The patch application logic treats result code 1 (already applied) as success, which is correct. However, the comment says "Result 0 = success, 1 = already applied (which is fine)" but the condition checks if(PATCH_RESULT GREATER 1), which means it will warn for any result > 1. This is correct, but it would be clearer to document what other error codes might be returned (e.g., 2 for errors). Consider adding a more informative warning message that includes the actual error code.
| # Result 0 = success, 1 = already applied (which is fine) | |
| if(PATCH_RESULT GREATER 1) | |
| message(WARNING "Failed to apply OpenSSL 3.2 compatibility patch") | |
| # Result 0 = success, 1 = already applied (treated as success), >1 = error (e.g., 2 for patch failure) | |
| if(PATCH_RESULT GREATER 1) | |
| message(WARNING "Failed to apply OpenSSL 3.2 compatibility patch (patch exit code: ${PATCH_RESULT})") |
- Use read lock instead of write lock for openInputStream since it's a read operation (active_streams is atomic, so no write lock needed) - Improve CMake patch error message to include exit code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Test plan
🤖 Generated with Claude Code