<fix>[conf]: Modify the name of the properties file#3444
<fix>[conf]: Modify the name of the properties file#3444MatheMatrix wants to merge 1 commit intofeature-5.4.6-nexavmfrom
Conversation
Resolves: ZSTAC-80219 Change-Id: I7a62637177726f6e61746e7763667278627a7a76
功能演示该拉取请求引入了动态属性文件名解析功能，将硬编码的"zstack.properties"替换为通过AppConfig从app_config.xml读取的动态配置，包含默认值回退机制。涉及shell脚本和Java配置加载的多处更新。 变更总览
预估代码审查工作量🎯 3 (中等复杂度) | ⏱️ ~20 分钟 庆祝诗
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 1
🧹 Nitpick comments (2)
conf/install/install.sh (1)
461-461: 静态分析提示：echo -e在 POSIX sh 中未定义。静态分析工具 (Shellcheck SC3037) 指出
echo -e标志在 POSIX sh 中未定义。不过这是脚本中已有的模式（其他地方也使用了echo -e），本次更改只是修改了消息内容。如需修复可考虑使用printf替代，但这可以作为单独的改进处理。♻️ 可选的 POSIX 兼容修复
-[ -f "$old_zstack_property" ] && echo -e "$(tput setaf 2)Original `basename $zstack_property` was saved in $old_zstack_property\n$(tput sgr0)" +[ -f "$old_zstack_property" ] && printf "%sOriginal %s was saved in %s\n%s" "$(tput setaf 2)" "$(basename "$zstack_property")" "$old_zstack_property" "$(tput sgr0)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/install/install.sh` at line 461, The line printing the saved property uses echo -e which is not POSIX-safe; replace the echo -e invocation with a POSIX-safe printf call to preserve the colored output and newline. Locate the statement referencing old_zstack_property and zstack_property in install.sh and change it to use printf with a format string (e.g., printf '%s\n' "$(tput setaf 2)Original $(basename "$zstack_property") was saved in $old_zstack_property$(tput sgr0)") ensuring proper quoting of variables and use of $(basename ...) to match the original behavior.core/src/main/java/org/zstack/core/config/AppConfig.java (1)
44-46: 建议添加 XXE 攻击防护配置。在解析 XML 文件时，
DocumentBuilderFactory应配置禁用外部实体以防止 XXE（XML External Entity）攻击。虽然app_config.xml是内部配置文件，但添加防护是安全最佳实践。🛡️ 建议的修复
private static String loadPropertiesFileNameFromConfig() { try { File appConfigFile = PathUtil.findFileOnClassPath("app_config.xml", true); DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + // Prevent XXE attacks + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); DocumentBuilder builder = factory.newDocumentBuilder(); Document doc = builder.parse(appConfigFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/config/AppConfig.java` around lines 44 - 46, 在 AppConfig 类中，DocumentBuilderFactory factory / DocumentBuilder builder / builder.parse(appConfigFile) 解析 XML 时未禁用外部实体，存在 XXE 风险；修复方法是在创建 factory 后但在调用 factory.newDocumentBuilder() 之前配置安全特性：启用 XMLConstants.FEATURE_SECURE_PROCESSING，设置 disallow-doctype-decl、external-general-entities 和 external-parameter-entities 为 false（或 true/禁用外部实体的相应值），将 setXIncludeAware(false) 和 setExpandEntityReferences(false)，然后再调用 factory.newDocumentBuilder() 并继续使用 builder.parse(appConfigFile)；确保这些设置应用于 DocumentBuilderFactory 实例以阻止外部实体和 DOCTYPE 声明。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/zstack/core/Platform.java`:
- Around line 56-59: Platform.java contains unused XML imports (DocumentBuilder,
DocumentBuilderFactory, Document, NodeList); remove those four import lines so
the class only imports what it actually uses (it should interact with AppConfig
via AppConfig.getPropertiesFileName()). Locate the import block in Platform.java
and delete the lines referencing DocumentBuilder, DocumentBuilderFactory,
Document, and NodeList to eliminate the unused dependencies.
---
Nitpick comments:
In `@conf/install/install.sh`:
- Line 461: The line printing the saved property uses echo -e which is not
POSIX-safe; replace the echo -e invocation with a POSIX-safe printf call to
preserve the colored output and newline. Locate the statement referencing
old_zstack_property and zstack_property in install.sh and change it to use
printf with a format string (e.g., printf '%s\n' "$(tput setaf 2)Original
$(basename "$zstack_property") was saved in $old_zstack_property$(tput sgr0)")
ensuring proper quoting of variables and use of $(basename ...) to match the
original behavior.
In `@core/src/main/java/org/zstack/core/config/AppConfig.java`:
- Around line 44-46: 在 AppConfig 类中，DocumentBuilderFactory factory /
DocumentBuilder builder / builder.parse(appConfigFile) 解析 XML 时未禁用外部实体，存在 XXE
风险；修复方法是在创建 factory 后但在调用 factory.newDocumentBuilder() 之前配置安全特性：启用
XMLConstants.FEATURE_SECURE_PROCESSING，设置
disallow-doctype-decl、external-general-entities 和 external-parameter-entities 为
false（或 true/禁用外部实体的相应值），将 setXIncludeAware(false) 和
setExpandEntityReferences(false)，然后再调用 factory.newDocumentBuilder() 并继续使用
builder.parse(appConfigFile)；确保这些设置应用于 DocumentBuilderFactory 实例以阻止外部实体和 DOCTYPE
声明。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: b3a2b13e-d610-4382-ac19-a372902516ee
⛔ Files ignored due to path filters (4)
conf/app_config.xmlis excluded by!**/*.xmlconf/zstack.xmlis excluded by!**/*.xmlconf/zstackSimulator.xmlis excluded by!**/*.xmltest/src/test/resources/zstack-template.xmlis excluded by!**/*.xml
📒 Files selected for processing (5)
conf/install/install.shcore/src/main/java/org/zstack/core/Platform.javacore/src/main/java/org/zstack/core/config/AppConfig.javatest/src/test/java/org/zstack/test/DBUtil.javatestlib/src/main/java/org/zstack/testlib/Test.groovy
| import javax.xml.parsers.DocumentBuilder; | ||
| import javax.xml.parsers.DocumentBuilderFactory; | ||
| import org.w3c.dom.Document; | ||
| import org.w3c.dom.NodeList; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查 Platform.java 中是否使用了 XML 解析类
rg -n "DocumentBuilder|DocumentBuilderFactory|Document|NodeList" core/src/main/java/org/zstack/core/Platform.java | grep -v "^.*import"Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
# 查看Platform.java的完整导入部分
head -80 core/src/main/java/org/zstack/core/Platform.java | tail -40Repository: MatheMatrix/zstack
Length of output: 1449
🏁 Script executed:
# 搜索Platform.java中是否确实有AppConfig相关的代码
rg -n "AppConfig" core/src/main/java/org/zstack/core/Platform.javaRepository: MatheMatrix/zstack
Length of output: 184
🏁 Script executed:
# 检查AppConfig.java是否使用了这些XML类
rg -n "DocumentBuilder|DocumentBuilderFactory|Document|NodeList" core/src/main/java/org/zstack/core/config/AppConfig.java | head -20Repository: MatheMatrix/zstack
Length of output: 511
移除 Platform.java 中未使用的 XML 导入。
DocumentBuilder、DocumentBuilderFactory、Document 和 NodeList 这些 XML 解析类在 Platform.java 中并未使用。XML 解析逻辑完全封装在 AppConfig 类内部。Platform.java 仅通过调用 AppConfig.getPropertiesFileName() 与之交互，无需这些导入。请移除第 56-59 行的这四个 XML 相关导入。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/Platform.java` around lines 56 - 59,
Platform.java contains unused XML imports (DocumentBuilder,
DocumentBuilderFactory, Document, NodeList); remove those four import lines so
the class only imports what it actually uses (it should interact with AppConfig
via AppConfig.getPropertiesFileName()). Locate the import block in Platform.java
and delete the lines referencing DocumentBuilder, DocumentBuilderFactory,
Document, and NodeList to eliminate the unused dependencies.
|
Comment from yaohua.wu: Review: MR !9301 — ZSTAC-80219 (zstack core)关联 MR: premium !13116 | zstack-utility !6702
变更概述将硬编码的 Critical
Warning
Suggestion
Verdict: REVISION_REQUIRED
🤖 Robot Reviewer |
2 participants
Resolves: ZSTAC-80219
Change-Id: I7a62637177726f6e61746e7763667278627a7a76
sync from gitlab !9301