Skip to content

feat: 全局变量支持密码类型的变量 --story=1070120217135666950#790

Open
kaedePing wants to merge 1 commit into
TencentBlueKing:masterfrom
kaedePing:feat/support_password_variable_input
Open

feat: 全局变量支持密码类型的变量 --story=1070120217135666950#790
kaedePing wants to merge 1 commit into
TencentBlueKing:masterfrom
kaedePing:feat/support_password_variable_input

Conversation

@kaedePing

Copy link
Copy Markdown
Collaborator

Reviewed, transaction id: 82921

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 中 bare except Exception: pass 可能掩盖配置错误
  • 前端 password.js 中残留 TODO 注释应在合入前处理

Comment thread config/default.py
4bWmKaRPAkEAzr1S9q1rx/bV6iJOoEeeNGddWizWatTjLCT9XcXkETfIDc3YTpVD
bESXAcagtf6PbkNP1TG1MeXlTVhYp4tw6QJAa+frtnxkH+ILsf7FtNtkpV6nySo8
NC9qzL2/taVUs8YjMtQPfaRtoADZoXelCQpLwV5/prIfLKjJmBUD7he3BQJAVKYs
XBhx8zRcLjvR2cq5OlfAX3XQbXmxcpfKriSi13HxlcVc9gAj1SbYdb+wehQ7AjjJ

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 默认 RSA/SM2 私钥硬编码在源码中。虽然注释说明仅用于本地/测试环境,但提交到公开仓库后任何人都能用这个私钥解密对应公钥加密的数据。建议移除默认私钥,改为启动时检测环境变量是否配置,未配置则抛出明确错误或禁用加密功能。


if need_password_handle:
input_password_refs = self._get_raw_password_map() # 获取密码变量key-对应的加密后的value

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡ 每次插件 execute 都将 data + parent_data 的全部 inputs 序列化为 JSON 字符串,仅为检测是否包含 password_value 关键字。对于大型 inputs 这是不必要的开销。考虑在流程启动时标记是否引用了密码变量,避免热路径上的全量序列化。

copy_data_mask = None
copy_parent_data_mask = None

if need_password_handle:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 _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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bare except Exception: pass 会吞掉所有错误(如密钥格式错误、import 失败等),导致排查困难。建议至少 logger.exception(...) 记录异常。

attrs: {
name: gettext("密码"),
hookable: true,
// TODO 如果是 false,应该是允许输密码?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ 残留的 // TODO 如果是 false,应该是允许输密码? 注释应在合入前明确并清理,避免留下含糊的技术债务。

@kaedePing kaedePing force-pushed the feat/support_password_variable_input branch 2 times, most recently from 495c5cb to 4893410 Compare July 7, 2026 09:13

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review 总结(增量审查)

本次增量审查对比 commit 7c5860b548934108,代码结构与逻辑未发生实质变化,上一轮审查中指出的所有问题仍然存在:

未解决的前次问题:

  • 🔒 默认私钥硬编码(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 中 bare except 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 原则,后续维护容易遗漏同步修改

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量 Code Review 总结

本次 push 代码内容与上次审查基本一致,先前报告的问题 均未修复,此处不再重复。以下为新发现的问题:

新发现:

  • ⚠️ _mask_meta_system_mask_info 写入使用属性赋值,读取使用 dict .get(),两者访问方式不一致,依赖 data.inputs 类型的实现细节
  • schedule() 每次调度周期都重新调用 _get_raw_password_map() 做多次 DB 查询,对于高频轮询的 schedule 场景开销可观

先前未修复的问题仍然存在(7 条):

  1. 🔒 默认私钥硬编码(config/default.py)
  2. 🔒 明文密码存储在 data.inputs(可能持久化)
  3. ⚡ while 循环查找根流程 ID(N+1)
  4. ⚡ 每次 execute 全量 JSON 序列化检测密码
  5. ⚠️ 引号替换匹配依赖 Python repr 行为
  6. ⚠️ context_processors bare except
  7. ✨ 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 写入时使用属性赋值 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"] = ...

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查:补充 2 个新发现的问题,前次报告的问题仍未解决。

input_password_refs = self._get_raw_password_map() # 获取密码变量key-对应的加密后的value

# 自动解密 inputs 中引用的全局密码变量,使所有插件都支持输入全局密码变量
copy_data = copy.deepcopy(data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡ 当 _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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ executeschedule 中的密码解密/掩码/恢复逻辑几乎完全重复(~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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_get_raw_password_map() 在每次 schedule 周期都会被调用,每次执行 3+ 次 DB 查询(get_data、get_state 循环、get_context_values)。对于高频轮询的插件(如 2s 间隔),这会造成大量重复查询。建议在 execute 阶段将 password_map 存入 data outputs,schedule 直接复用。

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 37.12375% with 188 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.35%. Comparing base (9a63478) to head (4893410).
⚠️ Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
...ow/pipeline_plugins/components/collections/base.py 25.90% 163 Missing ⚠️
bkflow/utils/crypto.py 45.45% 18 Missing ⚠️
bkflow/interface/context_processors.py 60.00% 4 Missing ⚠️
config/default.py 87.50% 2 Missing ⚠️
...pipeline_plugins/variables/collections/password.py 93.33% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tencentblueking-adm

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants