fix: zip cross directory attack vulnerability#312
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a null-guard to ChangesUtils behavior and test suite expansion
Sequence Diagram(s)sequenceDiagram
participant ZipInputStream
participant Utils_processZipEntries
participant FileSystem
participant Logger
ZipInputStream->>Utils_processZipEntries: read ZipEntry
Utils_processZipEntries->>FileSystem: tempDir.toRealPath() -> safeDir
Utils_processZipEntries->>Utils_processZipEntries: resolve & normalize targetPath against safeDir
Utils_processZipEntries->>Logger: log computed targetPath
alt targetPath within safeDir
Utils_processZipEntries->>FileSystem: Files.createDirectories(targetPath or parent)
Utils_processZipEntries->>FileSystem: extractFile(targetPath)
else escape detected
Utils_processZipEntries-->>ZipInputStream: throw SecurityException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/src/main/java/com/tinyengine/it/common/utils/Utils.java`:
- Around line 242-252: The code validates targetPath against safeDir but then
creates and uses new File(tempDir, zipEntry.getName()), which can bypass the
normalization/symlink check; update the extraction/read logic in Utils (the
unzip/unpack method) to use the already-validated targetPath for all filesystem
operations: when creating directories, extracting entries, and reading contents,
call Files.createDirectories(targetPath) /
Files.createDirectories(targetPath.getParent()), pass targetPath (or
targetPath.toFile() if extractFile/readFileContent require File) into
extractFile and readFileContent instead of newFile, and remove the new
File(tempDir, ...) usage so all IO goes through the validated Path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c56d314-e82e-4809-b1ee-8a219b9b2807
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/common/utils/Utils.java
hexqi
reviewed
May 15, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
base/src/main/java/com/tinyengine/it/common/utils/Utils.java (1)
245-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the validated
targetPathfor extraction and reads.Path validation is done on
targetPath, but file IO still usesnew File(tempDir, zipEntry.getName()). Keep all filesystem operations on the already-validated path to avoid validation/usage drift.Suggested patch
- File newFile = new File(tempDir, zipEntry.getName()); + File newFile = targetPath.toFile(); if (zipEntry.isDirectory()) { // 创建目录（同时确保父目录存在） Files.createDirectories(targetPath); fileInfoList.add(new FileInfo(newFile.getName(), "", true)); // 添加目录 } else { // 确保父目录存在 if(targetPath.getParent() != null) { Files.createDirectories(targetPath.getParent()); } extractFile(zis, newFile); // 解压文件 fileInfoList.add(new FileInfo(newFile.getName(), readFileContent(newFile), false)); // 添加文件内容 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/main/java/com/tinyengine/it/common/utils/Utils.java` around lines 245 - 257, The code validates and uses the Path variable targetPath but then performs file I/O with a different File instance newFile (constructed from tempDir and zipEntry.getName()), which can bypass validation; change the extraction and read operations to use the validated targetPath (and its corresponding File via targetPath.toFile() if needed) so that Files.createDirectories, extractFile, and readFileContent operate on the same validated path; update places that construct FileInfo (currently using newFile.getName() and readFileContent(newFile)) to use targetPath (or targetPath.getFileName().toString()) and readFileContent(targetPath.toFile()) to ensure consistent, validated filesystem access.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java`:
- Around line 715-724: The test test_encodeObjectToBase64_WithMap (and the
similar test at lines 726-735) incorrectly asserts plaintext substrings on the
Base64-encoded result; update the test to decode the Base64 output from
Utils.encodeObjectToBase64 back into bytes/string (using
Base64.getDecoder().decode or equivalent), then assert the decoded payload
contains "Alice", "20", and the expected "image/" MIME substring (or parse the
decoded JSON and assert fields), instead of asserting on the encoded string
itself.
- Around line 525-532: The test assumptions are wrong for Utils.flat (it only
flattens nested Map values, not lists/arrays); update the test methods (e.g.,
test_flat_doesNotFlattenListsOrArrays and the similar case around the other
assertions) to assert that the output map still contains a single "list" key
whose value equals the original List/array (or that output.size() == 1 and
output.get("list") equals the input list/array), instead of expecting "list[0]"
/ "list[1]" keys.
- Around line 910-916: The tests test_readAllBytes_Normal and
test_readAllBytes_BufferSizeTest mock InputStream.read to return lengths but
never write into the provided byte[] buffer, so Utils.readAllBytes receives
empty data; update the mockInputStream behavior for Utils.readAllBytes in
UtilsTest.java to use a Mockito Answer (or doAnswer) that copies the expected
bytes into the invocation’s byte[] argument before returning the number of bytes
read (for multiple-read scenarios maintain a read-offset/counter to copy the
next chunk on subsequent calls). Target the mockInputStream read(...) stubbing
in test_readAllBytes_Normal and test_readAllBytes_BufferSizeTest and ensure the
Answer writes the correct slice of expectedBytes into the provided buffer and
returns the appropriate length (then -1).
- Around line 63-70: The tests create a MockedStatic named validationUtilMock
for Utils but still stub methods with plain when(Utils.method(...)) calls, so
those stubs aren’t applied to the MockedStatic; update all stubbings that target
Utils (e.g., Utils.createTempDirectory(), Utils.convertMultipartFileToFile(...),
Utils.processZipEntries(...), and Utils.unzip() related stubs) to use
validationUtilMock.when(() -> Utils.method(...)).thenReturn(...) or
thenAnswer(...) so the mocks are registered against validationUtilMock (note
validateFileStream stubbing is already correct); keep using
validationUtilMock.close() in teardown as before.
---
Duplicate comments:
In `@base/src/main/java/com/tinyengine/it/common/utils/Utils.java`:
- Around line 245-257: The code validates and uses the Path variable targetPath
but then performs file I/O with a different File instance newFile (constructed
from tempDir and zipEntry.getName()), which can bypass validation; change the
extraction and read operations to use the validated targetPath (and its
corresponding File via targetPath.toFile() if needed) so that
Files.createDirectories, extractFile, and readFileContent operate on the same
validated path; update places that construct FileInfo (currently using
newFile.getName() and readFileContent(newFile)) to use targetPath (or
targetPath.getFileName().toString()) and readFileContent(targetPath.toFile()) to
ensure consistent, validated filesystem access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6844684a-4df6-428f-954c-83716b873b37
📒 Files selected for processing (2)
base/src/main/java/com/tinyengine/it/common/utils/Utils.javabase/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java
chilingling
reviewed
May 28, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java (1)
418-424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest mutates
outputafterflat(), making assertions tautological.
Utils.flatflattens the nested input{a:{y:100, z:{x:"value"}}}to{a.y=100, a.z.x="value"}(size 2). The twooutput.put(...)calls at Lines 419-420 inject values into the result before the assertions, soassertEquals(3, ...),a.z.x, anda.z.ypass regardless of the real flatten logic. Notea.z.yis not even a key the implementation produces (a.yis), anda.yis never validated.🐛 Proposed fix: assert on actual flatten output
Map<String, Object> output = Utils.flat(input); - output.put("a.z.x", "value"); - output.put("a.z.y", 100); assertNotNull(output); - assertEquals(3, output.size()); + assertEquals(2, output.size()); + assertEquals(100, output.get("a.y")); assertEquals("value", output.get("a.z.x")); - assertEquals(100, output.get("a.z.y"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java` around lines 418 - 424, The test mutates the result of Utils.flat and thus makes assertions tautological; remove the two output.put(...) calls and instead assert the true flattened result from Utils.flat in UtilsTest (variable output): expect size 2, assertEquals("value", output.get("a.z.x")), and assertEquals(100, output.get("a.y")). Keep references to Utils.flat and the local variable output so the test validates the real flatten logic.
🧹 Nitpick comments (1)
base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java (1)
91-105: 💤 Low valueSpy/mock scaffolding in
setUp()is unused.
@InjectMocks Utils utils,Mockito.spy(Utils.class), and@Mock MultipartFile mockFileare not referenced by any test (all call the staticUtils.*methods directly, andmockFileis reassigned to aMockMultipartFile). This is leftover scaffolding from the earliermockStaticapproach and can be removed to reduce confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java` around lines 91 - 105, The setUp() contains unused mock/spy scaffolding: remove the `@InjectMocks` Utils utils declaration, the Mockito.spy(Utils.class) call, and the `@Mock` MultipartFile mockFile field since tests call static Utils.* methods and reassign mockFile to a MockMultipartFile; simplify setUp() to only initialize the MockMultipartFile (mockFile = new MockMultipartFile(...)) or delete mockFile entirely if not used, and remove any unused MockitoAnnotations.openMocks(this) invocation to eliminate dead test setup code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java`:
- Around line 596-599: The negative contains assertions on the Base64 string
(variable base64) are brittle; instead decode the Base64 output and assert the
decoded payload contains the expected values/fields. Locate the test that builds
the base64 variable (see assertions referencing base64) and replace the
assertFalse checks with code that Base64-decodes base64, converts bytes to a
string (or parses JSON if applicable), and then assertThat the decoded
string/parsed object contains the expected fields/values (e.g., using
assertTrue/equals for specific keys) to verify proper encoding by
round-tripping.
---
Outside diff comments:
In `@base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java`:
- Around line 418-424: The test mutates the result of Utils.flat and thus makes
assertions tautological; remove the two output.put(...) calls and instead assert
the true flattened result from Utils.flat in UtilsTest (variable output): expect
size 2, assertEquals("value", output.get("a.z.x")), and assertEquals(100,
output.get("a.y")). Keep references to Utils.flat and the local variable output
so the test validates the real flatten logic.
---
Nitpick comments:
In `@base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java`:
- Around line 91-105: The setUp() contains unused mock/spy scaffolding: remove
the `@InjectMocks` Utils utils declaration, the Mockito.spy(Utils.class) call, and
the `@Mock` MultipartFile mockFile field since tests call static Utils.* methods
and reassign mockFile to a MockMultipartFile; simplify setUp() to only
initialize the MockMultipartFile (mockFile = new MockMultipartFile(...)) or
delete mockFile entirely if not used, and remove any unused
MockitoAnnotations.openMocks(this) invocation to eliminate dead test setup code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 539355f4-c57b-4432-b5c3-7d90d1c31f44
📒 Files selected for processing (1)
base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java
chilingling
requested changes
May 29, 2026
hexqi
approved these changes
May 29, 2026
chilingling
approved these changes
May 29, 2026
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Bug Fixes
Tests