<fix>[core]: add SshFileChecker - #4649
Open
zstack-robot-2 wants to merge 1 commit into
Open
Conversation
Resolves: ZSTAC-46801 Change-Id: I756c7a75796f6676766678696d786f616f617676
Walkthrough新增 ChangesSSH 文件检查
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@core/src/main/java/org/zstack/core/ansible/SshFileChecker.java`:
- Around line 48-50: Implement deleteDestFile() in SshFileChecker to remove the
remote filePath after a failed deployment, reusing the existing SSH
configuration and safely escaping the path without deleting its parent
directory. Record any cleanup failure while preserving the method’s current
contract.
- Line 33: Update the command construction in SshFileChecker to
POSIX-shell-quote both dirPath and filePath before interpolation, using the
standard single-quote escaping that safely handles embedded quotes, spaces,
substitutions, and shell metacharacters. Add or reuse a helper such as
quoteForPosixShell and preserve the existing directory/file checks.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00d57050-21b7-48d7-9f2d-9968c272e0d0
📒 Files selected for processing (1)
core/src/main/java/org/zstack/core/ansible/SshFileChecker.java
| try { | ||
| String dirPath = new File(filePath).getParent(); | ||
|
|
||
| ssh.command(String.format("if [ -d %s ] && [ -f %s ]; then exit 0; else exit 1; fi", dirPath, filePath)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
对远端路径进行 POSIX shell 转义，避免远程命令注入。
Line 33 直接将可通过 setFilePath() 设置的值拼入 shell；包含分号、命令替换或空格的路径可执行额外远端命令，普通含空格路径也会误判。请对每个参数做单引号转义后再构造命令。
建议修改
- ssh.command(String.format("if [ -d %s ] && [ -f %s ]; then exit 0; else exit 1; fi", dirPath, filePath));
+ ssh.command(String.format("if [ -d %s ] && [ -f %s ]; then exit 0; else exit 1; fi",
+ quoteForPosixShell(dirPath), quoteForPosixShell(filePath)));private static String quoteForPosixShell(String value) {
return "'" + value.replace("'", "'\"'\"'") + "'";
}📝 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.
Suggested change
| ssh.command(String.format("if [ -d %s ] && [ -f %s ]; then exit 0; else exit 1; fi", dirPath, filePath)); | |
| ssh.command(String.format("if [ -d %s ] && [ -f %s ]; then exit 0; else exit 1; fi", | |
| quoteForPosixShell(dirPath), quoteForPosixShell(filePath))); | |
| private static String quoteForPosixShell(String value) { | |
| return "'" + value.replace("'", "'\"'\"'") + "'"; | |
| } |
🤖 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 `@core/src/main/java/org/zstack/core/ansible/SshFileChecker.java` at line 33,
Update the command construction in SshFileChecker to POSIX-shell-quote both
dirPath and filePath before interpolation, using the standard single-quote
escaping that safely handles embedded quotes, spaces, substitutions, and shell
metacharacters. Add or reuse a helper such as quoteForPosixShell and preserve
the existing directory/file checks.
Comment on lines
+48
to
+50
| public void deleteDestFile() { | ||
| // do nothing. | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
失败后删除远端残留文件。
AnsibleRunner.cleanup() 会在 playbook 失败后调用此方法，而 needDeploy() 只要发现该文件存在就返回 false。当前空实现会保留可能不完整的 filePath，使下一次调用错误跳过部署。请使用相同 SSH 配置以安全转义的路径执行远端文件删除（不删除父目录），并记录清理失败。
🤖 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 `@core/src/main/java/org/zstack/core/ansible/SshFileChecker.java` around lines
48 - 50, Implement deleteDestFile() in SshFileChecker to remove the remote
filePath after a failed deployment, reusing the existing SSH configuration and
safely escaping the path without deleting its parent directory. Record any
cleanup failure while preserving the method’s current contract.
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
2 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.
Resolves: ZSTAC-46801
Change-Id: I756c7a75796f6676766678696d786f616f617676
sync from gitlab !5310