fix: zip cross directory attack vulnerability#312
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR hardens ZIP extraction in ChangesZIP Path Traversal Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
| File newFile = new File(tempDir, zipEntry.getName()); | ||
|
|
||
| if (zipEntry.isDirectory()) { | ||
| // 创建目录（同时确保父目录存在） | ||
| Files.createDirectories(targetPath); | ||
| fileInfoList.add(new FileInfo(newFile.getName(), "", true)); // 添加目录 | ||
| } else { | ||
| // 确保父目录存在 | ||
| Files.createDirectories(targetPath.getParent()); | ||
| extractFile(zis, newFile); // 解压文件 | ||
| fileInfoList.add(new FileInfo(newFile.getName(), readFileContent(newFile), false)); // 添加文件内容 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify path-validation path vs extraction path usage in processZipEntries.
rg -n 'Path targetPath = safeDir\.resolve\(entryName\)\.normalize\(\);|File newFile = new File\(tempDir, zipEntry\.getName\(\)\);|extractFile\(zis, newFile\);|readFileContent\(newFile\)' base/src/main/java/com/tinyengine/it/common/utils/Utils.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Get broader context around the issue area
sed -n '220,260p' base/src/main/java/com/tinyengine/it/common/utils/Utils.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 1695
🏁 Script executed:
#!/bin/bash
# Find where safeDir is defined/initialized
rg -n 'safeDir\s*=' base/src/main/java/com/tinyengine/it/common/utils/Utils.java -B 2 -A 2Repository: opentiny/tiny-engine-backend-java
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Find the method signature to understand the full context
rg -n 'void processZipEntries|Path.*processZipEntries|private.*processZipEntries' base/src/main/java/com/tinyengine/it/common/utils/Utils.java -A 15Repository: opentiny/tiny-engine-backend-java
Length of output: 878
🏁 Script executed:
#!/bin/bash
# Find extractFile and readFileContent method signatures
rg -n 'private.*extractFile\(|void extractFile\(|public.*extractFile\(' base/src/main/java/com/tinyengine/it/common/utils/Utils.java -A 3Repository: opentiny/tiny-engine-backend-java
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Find readFileContent method
rg -n 'private.*readFileContent\(|String readFileContent\(' base/src/main/java/com/tinyengine/it/common/utils/Utils.java -A 3Repository: opentiny/tiny-engine-backend-java
Length of output: 289
Use the validated targetPath as the only extraction/read path.
You validate targetPath against safeDir with path normalization and a security check, but then extract/read via new File(tempDir, zipEntry.getName()). Since safeDir is created via toRealPath() (which resolves symlinks), while newFile uses the raw tempDir, these paths can diverge if tempDir is a symlink, allowing the validation to be bypassed.
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 {
// 确保父目录存在
- Files.createDirectories(targetPath.getParent());
+ Path parent = targetPath.getParent();
+ if (parent != null) {
+ Files.createDirectories(parent);
+ }
extractFile(zis, newFile); // 解压文件
fileInfoList.add(new FileInfo(newFile.getName(), readFileContent(newFile), false)); // 添加文件内容
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| File newFile = new File(tempDir, zipEntry.getName()); | |
| if (zipEntry.isDirectory()) { | |
| // 创建目录（同时确保父目录存在） | |
| Files.createDirectories(targetPath); | |
| fileInfoList.add(new FileInfo(newFile.getName(), "", true)); // 添加目录 | |
| } else { | |
| // 确保父目录存在 | |
| Files.createDirectories(targetPath.getParent()); | |
| extractFile(zis, newFile); // 解压文件 | |
| fileInfoList.add(new FileInfo(newFile.getName(), readFileContent(newFile), false)); // 添加文件内容 | |
| File newFile = targetPath.toFile(); | |
| if (zipEntry.isDirectory()) { | |
| // 创建目录（同时确保父目录存在） | |
| Files.createDirectories(targetPath); | |
| fileInfoList.add(new FileInfo(newFile.getName(), "", true)); // 添加目录 | |
| } else { | |
| // 确保父目录存在 | |
| Path parent = targetPath.getParent(); | |
| if (parent != null) { | |
| Files.createDirectories(parent); | |
| } | |
| 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
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.
| List<FileInfo> fileInfoList = new ArrayList<>(); | ||
| ZipEntry zipEntry; | ||
|
|
||
| // 将 tempDir 转为规范路径（例如解析符号链接、父目录等） |
2 participants
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