feat: 全局变量支持密码类型的变量 --story=1070120217135666950#790
Conversation
There was a problem hiding this comment.
Code Review 总结
本 PR 实现全局密码变量支持(加解密、掩码),整体设计思路清晰,从标准运维迁移的 crypto 模块结构合理。以下是需要关注的问题:
Critical/Important:
- 🔒 默认私钥硬编码在源码中提交到公开仓库,存在安全风险
- 🔒
_mask_meta_system_mask_info中直接存储了解密后的明文数据到data.inputs,若 inputs 被引擎持久化或日志输出,密码明文可能泄露 - ⚡
_get_raw_password_map中 while 循环逐层查询 state 查找根流程 ID,深层嵌套子流程时有 N+1 风险 - ⚡ 每次 execute/schedule 都将全部 inputs 做 JSON 序列化仅为检测是否包含密码类型,性能开销不必要
Minor:
⚠️ context_processors 中 bareexcept Exception: pass可能掩盖配置错误- 前端 password.js 中残留 TODO 注释应在合入前处理
| 4bWmKaRPAkEAzr1S9q1rx/bV6iJOoEeeNGddWizWatTjLCT9XcXkETfIDc3YTpVD | ||
| bESXAcagtf6PbkNP1TG1MeXlTVhYp4tw6QJAa+frtnxkH+ILsf7FtNtkpV6nySo8 | ||
| NC9qzL2/taVUs8YjMtQPfaRtoADZoXelCQpLwV5/prIfLKjJmBUD7he3BQJAVKYs | ||
| XBhx8zRcLjvR2cq5OlfAX3XQbXmxcpfKriSi13HxlcVc9gAj1SbYdb+wehQ7AjjJ |
There was a problem hiding this comment.
🔒 默认 RSA/SM2 私钥硬编码在源码中。虽然注释说明仅用于本地/测试环境,但提交到公开仓库后任何人都能用这个私钥解密对应公钥加密的数据。建议移除默认私钥,改为启动时检测环境变量是否配置,未配置则抛出明确错误或禁用加密功能。
|
|
||
| if need_password_handle: | ||
| input_password_refs = self._get_raw_password_map() # 获取密码变量key-对应的加密后的value | ||
|
|
There was a problem hiding this comment.
⚡ while state.parent_id 循环会为每一层子流程嵌套调用一次 runtime.get_state()(数据库查询)。对于深层嵌套的子流程场景,这是 N+1 查询模式。建议缓存 top_pipeline_id 或使用引擎提供的直接获取根流程 ID 的方法(如果有的话)。
| import logging | ||
|
|
||
| from django.apps import apps | ||
| from django.conf import settings |
There was a problem hiding this comment.
⚡ 每次插件 execute 都将 data + parent_data 的全部 inputs 序列化为 JSON 字符串,仅为检测是否包含 password_value 关键字。对于大型 inputs 这是不必要的开销。考虑在流程启动时标记是否引用了密码变量,避免热路径上的全量序列化。
| copy_data_mask = None | ||
| copy_parent_data_mask = None | ||
|
|
||
| if need_password_handle: |
There was a problem hiding this comment.
🔒 _mask_meta_system_mask_info 将解密后的明文 inputs 直接存储在 data.inputs 中。如果 bamboo_engine 在 schedule 间隙会持久化 data 到数据库,明文密码可能被写入存储。请确认引擎对 _mask_meta_system_* 前缀字段的持久化行为,或考虑仅存储加密后的值。
|
|
||
| # 另外拷贝一份出来做掩码处理 | ||
| copy_data_mask = copy.deepcopy(copy_data) | ||
| copy_parent_data_mask = copy.deepcopy(copy_parent_data) |
There was a problem hiding this comment.
json.dumps(encrypted_password).replace('"', "'") 做引号替换来匹配 str 中的密码结构体——这依赖于变量渲染后 dict 被 repr/str 化时使用单引号的假设。如果渲染引擎行为变化(如输出双引号 JSON),此匹配会失效,建议增加双引号格式的匹配或使用更可靠的模式。
| ctx["ASYMMETRIC_CIPHER_TYPE"] = settings.BKCRYPTO_ASYMMETRIC_CIPHER_TYPE | ||
| ctx["ASYMMETRIC_PUBLIC_KEY"] = json.dumps(asymmetric_key_config.public_key_string)[1:-1] | ||
| ctx["ASYMMETRIC_PREFIX"] = f"{settings.BKCRYPTO_ASYMMETRIC_CIPHER_TYPE.lower()}_str:::" | ||
| except Exception: |
There was a problem hiding this comment.
except Exception: pass 会吞掉所有错误(如密钥格式错误、import 失败等),导致排查困难。建议至少 logger.exception(...) 记录异常。
| attrs: { | ||
| name: gettext("密码"), | ||
| hookable: true, | ||
| // TODO 如果是 false,应该是允许输密码? |
There was a problem hiding this comment.
✨ 残留的 // TODO 如果是 false,应该是允许输密码? 注释应在合入前明确并清理,避免留下含糊的技术债务。
# Reviewed, transaction id: 82923
495c5cb to
4893410
Compare
There was a problem hiding this comment.
Code Review 总结(增量审查)
本次增量审查对比 commit 7c5860b5 → 48934108,代码结构与逻辑未发生实质变化,上一轮审查中指出的所有问题仍然存在:
未解决的前次问题:
- 🔒 默认私钥硬编码(config/default.py:503-518)
- 🔒
_mask_meta_system_mask_info存储明文数据到 inputs(base.py:232-235) - ⚡ while 循环逐层查 state 的 N+1 风险(base.py:364-369)
- ⚡ 全量 JSON 序列化做预检(base.py:183)
⚠️ json.dumps(...).replace('"', "'")引号替换假设脆弱(base.py:454)⚠️ context_processors 中 bareexcept Exception: pass(context_processors.py:72)- ✨ password.js 残留 TODO 注释(password.js:9)
新发现的问题(本次补充):
- ⚡
need_password_handle=True但无实际密码变量时,仍执行 4 次 deepcopy,应在_get_raw_password_map()返回空后提前跳过 ⚠️ execute 和 schedule 中密码处理逻辑几乎完全重复(~60 行),违反 DRY 原则,后续维护容易遗漏同步修改
There was a problem hiding this comment.
增量 Code Review 总结
本次 push 代码内容与上次审查基本一致,先前报告的问题 均未修复,此处不再重复。以下为新发现的问题:
新发现:
⚠️ _mask_meta_system_mask_info写入使用属性赋值,读取使用 dict.get(),两者访问方式不一致,依赖data.inputs类型的实现细节- ⚡
schedule()每次调度周期都重新调用_get_raw_password_map()做多次 DB 查询,对于高频轮询的 schedule 场景开销可观
先前未修复的问题仍然存在(7 条):
- 🔒 默认私钥硬编码(config/default.py)
- 🔒 明文密码存储在 data.inputs(可能持久化)
- ⚡ while 循环查找根流程 ID(N+1)
- ⚡ 每次 execute 全量 JSON 序列化检测密码
⚠️ 引号替换匹配依赖 Python repr 行为⚠️ context_processors bare except- ✨ password.js TODO 注释残留
| _mask_meta_system_mask_info = { | ||
| 'decrypt_input_data': copy_data.inputs, | ||
| } | ||
| data.inputs._mask_meta_system_mask_info = _mask_meta_system_mask_info |
There was a problem hiding this comment.
data.inputs._mask_meta_system_mask_info = ...,但 schedule() 读取时用 data.inputs.get("_mask_meta_system_mask_info")。如果 data.inputs 被引擎反序列化为普通 dict,属性赋值不会体现在 dict items 中,.get() 将返回 None 导致 schedule 无法恢复解密数据。建议统一使用 dict 赋值 data.inputs["_mask_meta_system_mask_info"] = ...。
| input_password_refs = self._get_raw_password_map() # 获取密码变量key-对应的加密后的value | ||
|
|
||
| # 自动解密 inputs 中引用的全局密码变量,使所有插件都支持输入全局密码变量 | ||
| copy_data = copy.deepcopy(data) |
There was a problem hiding this comment.
⚡ 当 _get_raw_password_map() 返回空 dict 时(inputs 中包含 password_value 字符串但实际无密码变量),后续 4 次 deepcopy 和掩码逻辑全部白跑。建议在 input_password_refs 为空时 early-return,避免无意义的深拷贝开销。
| self._auto_decrypt_password_inputs(copy_parent_data_mask, input_password_refs=input_password_refs, | ||
| mask_flag=True) | ||
|
|
||
| result = False |
There was a problem hiding this comment.
execute 和 schedule 中的密码解密/掩码/恢复逻辑几乎完全重复(~60 行相同结构)。建议提取为一个私有方法(如 _wrap_with_password_handling(method, data, parent_data, ...)),避免后续修改一处漏另一处。
| copy_parent_data_mask = None | ||
|
|
||
| if need_password_handle: | ||
| input_password_refs = self._get_raw_password_map() # 获取密码变量key-对应的加密后的value |
There was a problem hiding this comment.
⚡ _get_raw_password_map() 在每次 schedule 周期都会被调用,每次执行 3+ 次 DB 查询(get_data、get_state 循环、get_context_values)。对于高频轮询的插件(如 2s 间隔),这会造成大量重复查询。建议在 execute 阶段将 password_map 存入 data outputs,schedule 直接复用。
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #790 +/- ##
==========================================
- Coverage 83.10% 82.35% -0.76%
==========================================
Files 307 309 +2
Lines 18167 18452 +285
==========================================
+ Hits 15098 15196 +98
- Misses 3069 3256 +187 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
Reviewed, transaction id: 82921