fix[storage]: desensitize addonInfo password fields in ExternalPrimaryStorageInventory#4120
Closed
zstack-robot-2 wants to merge 1 commit into
Closed
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:
Walkthrough在 ExternalPrimaryStorageInventory 中新增对 addonInfo 的递归脱敏（基于敏感后缀/包含子串掩码、嵌套 Map/List 递归、以及按 URL 规则脱敏包含凭证的字符串值），并新增单元测试覆盖多种敏感/非敏感命名模式、嵌套与边界场景。 改动详情敏感字段脱敏与测试验证
🎯 代码审查工作量评估🎯 4 (Complex) | ⏱️ ~45 分钟 诗句
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 unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v3
branch
from
d3b2567 to
77eae8f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java (1)
136-143: ⚡ Quick win断言强度不足，建议使用 assertEquals 验证掩码值
当前使用
assertNotEquals只验证了值不是原始密码，但没有验证它确实是预期的掩码值"******"。如果脱敏逻辑出现意外行为（如返回 null 或其他字符串），测试仍可能通过。♻️ 建议的修改
Object mdsInfos = config.get("mdsInfos"); if (mdsInfos instanceof List) { for (Object item : (List) mdsInfos) { if (item instanceof Map) { - 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 136 - 143, Replace the weak negative assertion with a strict equality check so the test verifies the password was actually masked: in ExternalPrimaryStorageInventoryTest where you iterate mdsInfos from config and inspect ((Map) item).get("sshPassword"), change the Assert.assertNotEquals check to Assert.assertEquals("******", ((Map) item).get("sshPassword")) (optionally add an Assert.assertNotNull prior to equality if you want a clearer failure when the key is missing).
🤖 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 136-143: Replace the weak negative assertion with a strict
equality check so the test verifies the password was actually masked: in
ExternalPrimaryStorageInventoryTest where you iterate mdsInfos from config and
inspect ((Map) item).get("sshPassword"), change the Assert.assertNotEquals check
to Assert.assertEquals("******", ((Map) item).get("sshPassword")) (optionally
add an Assert.assertNotNull prior to equality if you want a clearer failure when
the key is missing).
ℹ️ 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: c0f91138-cad2-4169-a6a3-193a03c71291
📒 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
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v3
branch
from
77eae8f to
34611d8
Compare
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
`@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java`:
- Line 79: 在 ExternalPrimaryStorageInventory 中补充 SENSITIVE_KEYS 集合，加入
"sshUsername"（当前只有 "password" 与 "sshPassword"），以确保 mdsInfos 等字段中的 sshUsername
不会明文返回；修改 SENSITIVE_KEYS 的初始化（保留现有项）添加 "sshUsername" 并确认序列化/脱敏逻辑仍使用该常量集合（引用
SENSITIVE_KEYS 的方法无需变更）。
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 0a8d9d23-7f00-4eb0-b5d4-ecf052ff5c19
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java
| } | ||
|
|
||
| private static final String MASK = "******"; | ||
| private static final Set<String> SENSITIVE_KEYS = new HashSet<>(Arrays.asList("password", "sshPassword")); |
There was a problem hiding this comment.
补全敏感键集合，避免 sshUsername 明文返回。
当前仅覆盖 password 与 sshPassword，mdsInfos 中的 sshUsername 仍会透出，和本次修复目标不一致，存在敏感信息泄露风险。
🔧 建议修改
- private static final Set<String> SENSITIVE_KEYS = new HashSet<>(Arrays.asList("password", "sshPassword"));
+ private static final Set<String> SENSITIVE_KEYS = new HashSet<>(Arrays.asList("password", "sshPassword", "sshUsername"));📝 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
| private static final Set<String> SENSITIVE_KEYS = new HashSet<>(Arrays.asList("password", "sshPassword")); | |
| private static final Set<String> SENSITIVE_KEYS = new HashSet<>(Arrays.asList("password", "sshPassword", "sshUsername")); |
🤖 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`
at line 79, 在 ExternalPrimaryStorageInventory 中补充 SENSITIVE_KEYS 集合，加入
"sshUsername"（当前只有 "password" 与 "sshPassword"），以确保 mdsInfos 等字段中的 sshUsername
不会明文返回；修改 SENSITIVE_KEYS 的初始化（保留现有项）添加 "sshUsername" 并确认序列化/脱敏逻辑仍使用该常量集合（引用
SENSITIVE_KEYS 的方法无需变更）。
f07ed64 to
27ba92b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java (1)
311-336: ⚡ Quick win若实现补充
privateKey/secretKey脱敏，此测试需同步更新。此测试明确验证
secretKey和privateKey不被脱敏。如果采纳实现层的修复建议（将"privatekey"、"secretkey"加入敏感后缀集合），则需将这两个字段的断言修改为期望"******"：- Assert.assertEquals("sk-abc123", item.get("secretKey")); - Assert.assertEquals("-----BEGIN RSA-----", item.get("privateKey")); + Assert.assertEquals("******", item.get("secretKey")); + Assert.assertEquals("******", item.get("privateKey"));🤖 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 311 - 336, The test testBareKeySuffixNotMasked assumes secretKey/privateKey remain unmasked, but if the implementation now treats "privatekey"/"secretkey" as sensitive suffixes the expected values should be updated; modify the assertions in ExternalPrimaryStorageInventoryTest.testBareKeySuffixNotMasked to expect the masked value ("******") for item.get("secretKey") and item.get("privateKey") (while keeping other assertions unchanged), ensuring the test aligns with ExternalPrimaryStorageInventory.valueOf's masking behavior and the sensitive suffix collection used by the code that masks addonInfo fields.
🤖 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
`@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java`:
- Around line 79-90: The current suffix-based detection in
SENSITIVE_KEY_SUFFIXES and isSensitiveKey misses keys like "privateKey" and
"secretKey"; update the logic by either adding "privatekey" (lowercase) to
SENSITIVE_KEY_SUFFIXES and/or introduce a new exact-match set (e.g.,
SENSITIVE_KEY_EXACTS) containing names like "secretkey" and "privatekey", then
modify isSensitiveKey to check the exact-match set first (using
String.valueOf(key).toLowerCase()) and fall back to suffix checks; ensure you
reference and update the existing SENSITIVE_KEY_SUFFIXES and isSensitiveKey
symbols so both "privateKey" and "secretKey" are masked.
---
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java`:
- Around line 311-336: The test testBareKeySuffixNotMasked assumes
secretKey/privateKey remain unmasked, but if the implementation now treats
"privatekey"/"secretkey" as sensitive suffixes the expected values should be
updated; modify the assertions in
ExternalPrimaryStorageInventoryTest.testBareKeySuffixNotMasked to expect the
masked value ("******") for item.get("secretKey") and item.get("privateKey")
(while keeping other assertions unchanged), ensuring the test aligns with
ExternalPrimaryStorageInventory.valueOf's masking behavior and the sensitive
suffix collection used by the code that masks addonInfo fields.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 2cf72a50-b8f2-4a38-93c4-33b0f384984c
📒 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
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v3
branch
6 times, most recently
from
39bca1a to
d7dbbbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java (1)
306-320: ⚡ Quick win建议补一条“无凭证但包含 @”的 URL 回归用例。
当前只覆盖了带凭证 URL 的脱敏，建议增加
http://10.0.0.1/path@tag这类样例并断言原值不变，以防后续再次引入误脱敏。可参考新增用例
+ `@Test` + public void testUrlWithoutUserInfoShouldNotBeMasked() { + ExternalPrimaryStorageVO vo = createVO(); + vo.setAddonInfo("{" + + "\"endpoint\": \"http://10.0.0.1/path@tag\"," + + "\"clusterName\": \"prod\"" + + "}"); + + ExternalPrimaryStorageInventory inv = ExternalPrimaryStorageInventory.valueOf(vo); + Map addonInfo = inv.getAddonInfo(); + Assert.assertEquals("http://10.0.0.1/path@tag", addonInfo.get("endpoint")); + }🤖 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 306 - 320, Add a new unit test that covers URLs containing "@" but without credentials to ensure no desensitization; create an ExternalPrimaryStorageVO, call vo.setAddonInfo with an endpoint like "http://10.0.0.1/path@tag" (and other addon fields), convert it via ExternalPrimaryStorageInventory.valueOf(vo), then inspect inv.getAddonInfo() and assert the endpoint still contains the original "`@tag`" (and does not contain "***@") and other fields (e.g., "clusterName") remain unchanged; place this alongside testUrlInMapValueDesensitized and reuse the same helper createVO() pattern.
🤖 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
`@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageInventory.java`:
- Around line 119-120: The current logic in ExternalPrimaryStorageInventory
calls desensitizeUrl for any string containing "://", which incorrectly trims
strings like "http://host/path@tag" that have '@' in the path, not as credential
user-info; update the desensitization so it only strips credentials when
user-info is present: modify the desensitizeUrl implementation (or the call
site) to parse the string as a URI (using java.net.URI), check uri.getUserInfo()
(or uri.getRawAuthority()) and only remove the user-info portion when
getUserInfo() != null; keep other strings intact. Ensure the change targets
ExternalPrimaryStorageInventory and desensitizeUrl so only URLs with actual
authority user-info are altered.
---
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/header/storage/addon/ExternalPrimaryStorageInventoryTest.java`:
- Around line 306-320: Add a new unit test that covers URLs containing "@" but
without credentials to ensure no desensitization; create an
ExternalPrimaryStorageVO, call vo.setAddonInfo with an endpoint like
"http://10.0.0.1/path@tag" (and other addon fields), convert it via
ExternalPrimaryStorageInventory.valueOf(vo), then inspect inv.getAddonInfo() and
assert the endpoint still contains the original "`@tag`" (and does not contain
"***@") and other fields (e.g., "clusterName") remain unchanged; place this
alongside testUrlInMapValueDesensitized and reuse the same helper createVO()
pattern.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 5bb4a22d-e377-4271-ad3a-307c833dac49
📒 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
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v3
branch
2 times, most recently
from
0a3ff3d to
c2904c1
Compare
…yStorageInventory Use exact whitelist (case-insensitive) for sensitive key detection: password/sshPassword/privateKey/token/secret/accessKey/apiSecret/credential. URL credential masking in config.mdsUrls/mdsInfos with colon-before-at guard. No suffix/contains heuristic, no exclude lists. Resolves: ZSTAC-80822 Change-Id: I80822-addon-password-desensitize
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v3
branch
from
8080e47 to
30e5a46
Compare
zstack-robot-1
deleted the
sync/songtao.liu/fix/ZSTAC-80822-addon-password-mask-v3
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
1 participant
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.
Problem
ExternalPrimaryStorageInventory constructor was not desensitizing password fields in the
addonInfofield. When ZBS primary storage addonInfo containedmdsInfosentries with fields likepassword,sshPassword, orsshUsername, these were exposed in plaintext via API responses.Root Cause
In
ExternalPrimaryStorageInventory.java:65-66, the constructor deserializedaddonInfobut never called desensitization:Compare with
configwhich already had desensitization applied.Fix
Added
desensitizeAddonInfo(addonInfo)call at line 66 and refactored to share a commondesensitizeMdsInfoshelper that maskspassword,sshPassword, andsshUsernamewithinmdsInfosarrays. BothconfigandaddonInfonow pass through the same masking logic for their mdsInfos entries.Files changed (2)
header/.../ExternalPrimaryStorageInventory.java— 1-line fix + extract common helpertest/.../ExternalPrimaryStorageInventoryTest.java— 8 unit tests with init helperTest Evidence
Command:
mvn test -f test/pom.xml -o -Dtest=ExternalPrimaryStorageInventoryTest🤖 Generated with Claude Code
sync from gitlab !10019