fix[storage]: desensitize addonInfo password fields in ExternalPrimaryStorageInventory#4117
Closed
zstack-robot-2 wants to merge 1 commit into
Closed
Conversation
WalkthroughPR在ExternalPrimaryStorageInventory类中引入了敏感信息脱敏机制，新增MASK常量和多个private静态脱敏方法，在构造函数中对addonInfo进行脱敏处理，调整了config的脱敏逻辑，并新增了8个测试方法覆盖脱敏行为和边界场景。 Changes密码脱敏与安全处理
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 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.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java (1)
104-113: ⚡ Quick win建议提取脱敏掩码常量，避免魔法值。
这里直接写死
"******"，后续若掩码规则调整，容易出现实现与测试不一致。建议抽成常量并统一复用。♻️ 建议修改
public class ExternalPrimaryStorageInventory extends PrimaryStorageInventory { + private static final String PASSWORD_MASK = "******"; private String identity; @@ Map mds = (Map) item; if (mds.containsKey("password")) { - mds.put("password", "******"); + mds.put("password", PASSWORD_MASK); } } } }As per coding guidelines, “避免使用魔法值（Magic Value）：直接使用未经定义的数值或字符串应替换为枚举或常量。”
🤖 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 `@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java` around lines 104 - 113, The code in desensitizeAddonInfo currently uses the magic string "******" to mask passwords; extract that literal into a well-named constant (e.g., PASSWORD_MASK or DESENSITIZE_MASK) declared on the ExternalPrimaryStorageInventory class and replace occurrences in desensitizeAddonInfo where mds.put("password", "******") is used so callers/tests can reuse the same constant and any future mask changes are centralized.
🤖 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.
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java`:
- Around line 104-113: The code in desensitizeAddonInfo currently uses the magic
string "******" to mask passwords; extract that literal into a well-named
constant (e.g., PASSWORD_MASK or DESENSITIZE_MASK) declared on the
ExternalPrimaryStorageInventory class and replace occurrences in
desensitizeAddonInfo where mds.put("password", "******") is used so
callers/tests can reuse the same constant and any future mask changes are
centralized.
ℹ️ 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: 6363e681-afa0-4ada-a1cf-021502b1e9e9
📒 Files selected for processing (2)
header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.javatest/src/test/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventoryTest.java
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v2
branch
from
61e4ec1 to
0aa6577
Compare
…yStorageInventory Add desensitizeAddonInfo() to mask password, sshPassword, and sshUsername fields in addonInfo.mdsInfos entries. Also fix desensitizeConfig to use Map-based desensitizeMdsInfos instead of URL-based desensitizeUrlList for mdsInfos, since MdsInfo objects serialize with Gson field names (sshPassword/sshUsername), not embedded in URL strings. Resolves: ZSTAC-80822 Change-Id: I85a8eb039d7b77ec837521de624da88553d68678
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v2
branch
from
0aa6577 to
0205a9d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java (1)
105-132: ⚡ Quick win把
mdsInfos的断言收紧到精确掩码值。这里现在只校验
sshPassword不等于原始明文；如果后续把字段删掉、置空，或换成其他占位符，这个测试仍然会通过，但 API 返回契约已经变了。建议直接断言为"******"，这样既能保证“不泄漏”，也能保证“字段仍存在且按约定脱敏”。As per coding guidelines “之前的代码产生的行为不要直接去改动，即使是 bug 也可能用户有依赖”.建议修改
- Assert.assertNotEquals("shouldBeMasked", ((Map) item).get("sshPassword")); + Assert.assertEquals("******", ((Map) item).get("sshPassword"));🤖 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 `@test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java` around lines 105 - 132, The test method testConfigWithoutMdsInfosDoesNotLeak should tighten its assertion for the mdsInfos sshPassword to expect the exact mask value instead of only asserting it differs from the original; update the assertions in ExternalPrimaryStorageInventoryTest (inside testConfigWithoutMdsInfosDoesNotLeak) to check that each item's "sshPassword" entry equals the canonical mask string (e.g., "******") and still verify mdsUrls masking as before, ensuring the field remains present and is masked to the agreed contract rather than merely non-equal to the plaintext.
🤖 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.
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java`:
- Around line 105-132: The test method testConfigWithoutMdsInfosDoesNotLeak
should tighten its assertion for the mdsInfos sshPassword to expect the exact
mask value instead of only asserting it differs from the original; update the
assertions in ExternalPrimaryStorageInventoryTest (inside
testConfigWithoutMdsInfosDoesNotLeak) to check that each item's "sshPassword"
entry equals the canonical mask string (e.g., "******") and still verify mdsUrls
masking as before, ensuring the field remains present and is masked to the
agreed contract rather than merely non-equal to the plaintext.
ℹ️ 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: bbc6a22f-e2a5-463b-b9af-21e0c63c8bd7
📒 Files selected for processing (2)
header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.javatest/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java
zstack-robot-1
deleted the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v2
branch
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.
根因分析
ExternalPrimaryStorageInventory构造器 (line 65) 对addonInfo字段遗漏脱敏处理。已有desensitizeConfig()对config做 URL 密码脱敏 (line 64)，但addonInfo.mdsInfos[].password未脱敏，密码明文泄漏到 API 响应中。ExternalPrimaryStorageInventory.java:65修复方案
desensitizeAddonInfo(addonInfo)调用desensitizeAddonInfo(Map)方法：遍历 addonInfo 中mdsInfos列表，将每个元素的password字段替换为"******"测试
4 个单元测试覆盖：密码脱敏、null 安全、无 mdsInfos、mdsInfo 无 password 字段
影响范围
仅影响 API 响应输出，不影响 DB 存储和内部逻辑。风险级别：低。
🤖 Generated with Claude Code
sync from gitlab !10015