feat: support SecretInput for DingTalk clientSecret#528
Conversation
Greptile Summary此 PR 为 DingTalk Confidence Score: 5/5可安全合并，无 P0/P1 问题，核心逻辑健壮，测试覆盖充分。 所有主要代码路径（env/file/exec 解析、缓存命中跳过解析、失败时本地抛出错误）均有测试覆盖；async I/O 避免了事件循环阻塞；provider/id 的 schema 约束与 round-trip 逻辑一致；manifest JSON schema 与 Zod schema 保持同步。未发现新引入的 P0 或 P1 问题。 无需特别关注的文件。
|
| Filename | Overview |
|---|---|
| src/secret-input.ts | 新增核心 SecretInput 解析模块：支持 env/file/exec 三种来源，使用异步 I/O（readFile、execFileAsync）避免阻塞，解析失败时返回带结构信息的 failure 对象并写入 warn 日志。 |
| src/auth.ts | 在缓存未命中时调用 resolveRuntimeConfig；缓存命中时跳过 secret 解析，符合 PR 描述的优化目标。 |
| src/config.ts | 新增 resolveRuntimeConfig，对空 clientId/clientSecret 及解析失败均有明确本地错误；路径工具移至 path-utils.ts。 |
| src/gateway/channel-gateway.ts | startAccount 中替换原有同步 clientSecret 检查为 resolveRuntimeConfig，Stream 客户端使用解析后的 runtimeConfig。 |
| src/onboarding.ts | 向导 initialValue 使用 normalizeSecretInputString 展示占位符引用，提交时通过 parseSecretInputString 还原为对象，round-trip 有测试覆盖。 |
| src/types.ts | DingTalkConfig 与 DingTalkChannelConfig 中 clientSecret 类型从 string 更新为 SecretInput。 |
| openclaw.plugin.json | manifest JSON schema 中 clientSecret 更新为 anyOf[string, SecretInput 对象]，provider/id 均加入 pattern 限制，与 Zod schema 保持一致。 |
Reviews (7): Last reviewed commit: "docs(secret-input): document clientSecre..." | Re-trigger Greptile
|
Addressed the SecretInput review findings in e5e8112:\n\n- replaced synchronous file/exec secret resolution with async readFile + promisified execFile\n- kept the exec timeout and added warning logs for file/exec resolution failures\n- moved runtime config resolution to an async path and made it fail locally when clientId or resolved clientSecret is missing, before any DingTalk API call\n- updated tests for async exec resolution, local missing-secret failure, and formatted the SecretInput test file\n\nValidation run locally:\n- pnpm test tests/unit/secret-input.test.ts tests/unit/secret-input-exec.test.ts tests/unit/auth-secret-input-cache.test.ts\n- pnpm type-check |
zizhus-ai
left a comment
There was a problem hiding this comment.
Code Review
架构清晰、向后兼容、测试覆盖到位。greptile 提出的 P1 问题（execFileSync 阻塞、undefined 发送到 API）在第 2 个 commit 中已修复。以下是剩余问题：
CRITICAL（安全信任边界文档化）
C1. exec source 允许从配置文件执行任意命令
const result = await execFileAsync(value.provider, [value.id], {
encoding: "utf8",
timeout: SECRET_INPUT_EXEC_TIMEOUT_MS,
windowsHide: true,
});value.provider 直接作为可执行文件路径，value.id 作为参数，两者均来自插件配置 JSON。虽然使用了 execFile（非 exec，不走 shell 解析）避免了命令注入，但能写入配置文件的人可以让进程执行系统上任意二进制。
如果项目信任模型将插件配置视为可信输入（插件系统的标准假设），这是正确的。但必须显式文档化这个信任假设——在代码注释和 schema description 中说明："exec source 会执行指定的 provider 二进制，仅在受信配置中使用。"
如果配置可能来自非受信来源（如 Web UI），则需要增加白名单或沙箱。
C2. file source 可读取任意文件系统路径
readFile 读取 value.id 指定的任何路径，包括 /etc/shadow（如果进程有权限）。同样的信任边界分析适用。建议同 C1 一并文档化。
注：这两个 CRITICAL 是文档化要求而非代码缺陷。如果信任模型已明确，代码本身是正确的。
HIGH
H1. hasConfiguredSecretInput 对 env 源立即检查环境变量，对 file/exec 源乐观返回 true — 行为不对称
if (value.source === "env") {
return Boolean(process.env[value.id]?.trim());
}
return true; // file 和 exec- env 源：变量未设置 → 返回
false（即使后续可能设置） - file 源：指向不存在的文件 → 返回
true，运行时才失败 - exec 源：指向不存在的命令 → 返回
true，运行时才失败
这是合理的设计选择（env 检查无副作用，file/exec 不是），但用户可能困惑为什么 env 显示"未配置"而 file 显示"已配置"。建议在代码中加注释说明原因。
MEDIUM
M1. resolveDingTalkAccount 现在返回 <exec:helper:id> 占位符而非实际密钥
clientSecret: normalizeSecretInputString(config.clientSecret) || "",这对 display/status 路径是正确的，但语义变了——任何之前依赖 resolveDingTalkAccount 获取可用 clientSecret 的代码现在拿到的是占位符。当前 startAccount 正确地单独调用 resolveRuntimeConfig，但建议确认没有其他消费方直接使用这个返回值调用 API。
M2. startAccount 删除了早期校验，改为延迟到 resolveRuntimeConfig
旧代码在函数入口处立即检查 !config.clientId || !config.clientSecret，新代码在 accountStorePath 和 pluginLog 初始化之后才调用 resolveRuntimeConfig 抛错。虽然这些前置步骤看起来轻量，但错误反馈变慢了。
M3. 缺少 file source 的集成测试
测试覆盖了 env（isConfigured）和 exec（mock），但 file source 的完整解析路径没有测试。建议加一个：用 node:fs/promises 写临时文件，验证 resolveSecretInputString 能正确读取。
LOW
openclaw.plugin.json中clientSecret的 SecretInput schema 在 root 和 account 级别各写了一份，未使用$ref共享buildSecretInputSchema()是工厂函数但始终返回相同 schema，可以改为常量- schema 中
provider和id有minLength: 1但无maxLength，建议加maxLength: 1024防止畸形输入
Positive
src/secret-input.ts单一职责、128 行、完全自包含- 向后兼容：
anyOf: [string, object]让现有字符串配置无需迁移 - 懒解析策略正确：仅在
getAccessToken（cache miss）和startAccount时解析，display/status 路径使用normalizeSecretInputString输出占位符 - cache-hit 避免 exec 调用，有专门测试验证
resolveRuntimeConfig只被调用一次 SECRET_INPUT_EXEC_TIMEOUT_MS = 5000防止 helper 挂起，常量导出便于测试断言- 第 2 个 commit 及时修复了自动审查发现的 P1 问题
Summary
| Severity | Count | Key Items |
|---|---|---|
| CRITICAL | 2 | exec/file 信任边界需文档化（非代码缺陷） |
| HIGH | 1 | env vs file/exec 配置检查行为不对称 |
| MEDIUM | 3 | 占位符语义变更、延迟校验、缺 file source 测试 |
| LOW | 3 | schema 重复、工厂函数、maxLength |
结论：Approve。实现质量高，关键问题是信任边界需要显式文档化。建议在合入前加上安全信任模型的注释。
zizhus-ai
left a comment
There was a problem hiding this comment.
Code Review — Request Changes
架构清晰、向后兼容、测试覆盖到位。greptile 提出的 P1 问题（execFileSync 阻塞、undefined 发送到 API）在第 2 个 commit 中已修复。但以下安全和健壮性问题需要在合入前解决：
CRITICAL（必须修复）
C1. exec source 允许从配置文件执行任意命令 — 缺少信任边界文档
const result = await execFileAsync(value.provider, [value.id], {
encoding: "utf8",
timeout: SECRET_INPUT_EXEC_TIMEOUT_MS,
windowsHide: true,
});value.provider 直接作为可执行文件路径，来自插件配置 JSON。虽然使用 execFile 避免了 shell 注入，但能写入配置的人可以让进程执行系统上任意二进制。
要求：在 resolveSecretInputString 的 exec 分支上方加注释，在 openclaw.plugin.json 的 schema description 中说明："exec source 会执行指定的 provider 二进制，仅在受信配置环境中使用。"
C2. file source 可读取任意文件系统路径 — 同上
readFile 读取 value.id 指定的任何路径。同样需要文档化信任假设。
HIGH（建议修复）
H1. hasConfiguredSecretInput 对 env/file/exec 行为不对称
if (value.source === "env") {
return Boolean(process.env[value.id]?.trim());
}
return true; // file 和 exec 乐观返回env 未设置 → false；file 指向不存在的文件 → true。用户会困惑。至少加注释解释原因。
H2. 缺少 file source 集成测试
env 和 exec 都有测试，file source 的完整解析路径没有覆盖。建议用临时文件验证 resolveSecretInputString 读取。
MEDIUM
resolveDingTalkAccount现在返回<exec:helper:id>占位符，语义变更需确认无其他消费方直接拿clientSecret调 APIstartAccount删除了早期校验，错误反馈延后到resolveRuntimeConfigopenclaw.plugin.json中 SecretInput schema 在 root 和 account 级别重复，建议用$ref- schema 中
provider/id无maxLength，建议加maxLength: 1024
Positive
src/secret-input.ts单一职责、128 行、自包含- 向后兼容：
anyOf: [string, object]无需迁移 - 懒解析策略正确：仅在 cache miss 和 startAccount 时解析
- cache-hit 跳过 exec 调用，有测试验证
- exec 超时 5s 防挂起
要求合入前至少解决 C1/C2（文档化安全信任边界）和 H2（补 file source 测试）。
|
Addressed the new review feedback in e86a60d:
Validation run locally:
|
zizhus-ai
left a comment
There was a problem hiding this comment.
Code Review — Request Changes
HIGH
hasConfiguredSecretInput 对 file/exec 源乐观返回 true，运行时失败时错误信息缺乏上下文
hasConfiguredSecretInput 中 env 源会立即检查环境变量是否存在，不存在则返回 false；但 file/exec 源只要 ref 结构合法就返回 true：
if (value.source === "env") {
return Boolean(process.env[value.id]?.trim());
}
// file/exec: 乐观返回 true
return true;问题：
- UX 不一致 — 用户配了一个不存在的 file 路径，状态页面显示"已配置"，运行时才报错。env 源缺失时则直接显示"未配置"。
- 错误信息不够具体 —
resolveRuntimeConfig抛出的错误是"DingTalk clientId and resolved clientSecret are required"，没有指出是哪种 source 解析失败（file 读不到？exec 跑失败了？路径不对？），用户需要去翻日志才能定位。
建议（任选其一）：
- 在
resolveRuntimeConfig的错误信息中带上 source 类型和失败原因，例如"DingTalk clientSecret resolution failed: file source could not read /path/to/secret" - 或者让
resolveSecretInputString在返回undefined前通过 log 输出 warn（exec 分支已经有了，但 env 分支没有），并在resolveRuntimeConfig的 throw 中包含 secret source 类型
当前 catch 块中的 warn 日志只覆盖了 file 和 exec 的异常路径，env 变量不存在时完全静默。
其余 MEDIUM/LOW 问题不阻塞合并，可后续处理：
resolveDingTalkAccount返回 placeholder 字符串的语义变更，建议确认无其他调用方直接用clientSecret调 APIstartAccount删除了早期校验，错误反馈延后openclaw.plugin.json中 SecretInput schema 在 root 和 account 级别重复
|
Addressed the latest SecretInput review feedback in f2d930f:
Validation run locally:
|
soimy
left a comment
There was a problem hiding this comment.
我这边看下来，建议这个 PR 先不要合并，主要有两点：
-
现在如果配置里已经用了
SecretInput（比如 env / file / exec），再进一次 setup wizard 并保存，配置会被写成类似<env:...>这样的普通字符串。下次启动时，程序会把这段字符串当成真正的 secret 去用，结果就是拿 token 失败。这个不是展示层的小问题，而是会把原本可用的配置“保存坏”。 -
file类型的SecretInput目前没有复用仓库里现成的路径解析规则。像 Windows 下常见的~\\secret.txt、Users\\name\\...这类写法，行为可能会和项目里其他路径配置不一致。这个在跨平台环境里比较容易埋坑。
我建议：
- setup wizard 里不要把
SecretInput对象回写成占位字符串 - 补一个“已有 SecretInput 配置 -> 进入 wizard -> 保存后配置仍然保持对象结构”的测试
file路径解析尽量复用现有的通用路径解析逻辑，并补上对应测试
我本地复核过这次 PR 自带的 SecretInput 相关测试和补充的 onboarding/status 测试，它们目前都是绿的；也正因为这样，说明上面这两个场景现在还没有被测试覆盖到。
| message: "Client Secret (AppSecret)", | ||
| placeholder: "xxx-xxx-xxx-xxx", | ||
| initialValue: resolved.clientSecret ?? undefined, | ||
| initialValue: normalizeSecretInputString(resolved.clientSecret), |
There was a problem hiding this comment.
这里有个回写问题：resolveDingTalkAccount() 已经把对象型 SecretInput 格式化成展示字符串了，这里再作为文本初值给 prompter，后面保存时拿到的就只会是普通字符串。结果就是原来 { source: "env" | "file" | "exec", ... } 的配置，只要进一次 wizard 再保存，就会变成字面量 <env:...> / <file:...>，下次启动会把它当成真正的 secret 去用，拿 token 会失败。建议这里区分“展示值”和“落库值”，不要让已有 SecretInput 走字符串 round-trip。
| try { | ||
| // Trust boundary: file SecretInput reads the configured local path. Use it | ||
| // only with trusted plugin configuration. | ||
| const filePath = value.id.startsWith("~/") |
There was a problem hiding this comment.
file 这里建议不要自己再写一套路径归一化。仓库里现成入口是 resolveRelativePath() / resolveUserPath()，现在在 src/config.ts，测试已经覆盖了 ~、~/、~\\、Windows root path、混合分隔符这些情况（tests/unit/config.test.ts、tests/unit/config-advanced.test.ts）。这边如果复用那套行为会更稳，落地上可以是类似 const filePath = resolveRelativePath(value.id) 这样的调用。唯一要注意的是 config.ts 现在已经 import 了 secret-input.ts，这里不要直接反向 import 形成循环；比较顺的做法是把这两个 helper 抽到更底层的小模块，再让 config.ts 和这里共用，config.ts 继续保留导出也可以。
|
已按 maintainer 反馈继续收口，并把分支同步到最新 本轮处理：
刻意未做：
本地验证：
GitHub 现在显示分支已可合并（ |
|
补充：刚才 GitHub CI 第一次因为最新 验证更新：
因此目前贡献者侧已处理完 maintainer 提到的 setup 保存、file 路径解析、以及最新 main 冲突/CI 问题，等待 maintainer 复核即可。 |
|
最近的PR #537 对onboarding流程做了一定优化，还得再重新对齐下仓库的代码，麻烦了 |
367be68 to
57cccbe
Compare
|
Rebased this PR onto the latest Current head: Local validation:
GitHub now reports the PR as mergeable/clean, and the |
|
已基于当前 PR 实现追加两批收口提交：
本地验证：
GitHub Actions 已重新触发，等待 CI 结果。 |
|
继续按最新 Greptile 审核意见追加修复：
评估但未改：Greptile 提到的 本地验证：
已推送到 PR 分支，等待 GitHub Actions 重新跑。 |
|
已按 SecretInput 新功能补充用户文档：
本地验证：
已推送到 PR 分支，等待 GitHub Actions 重新跑。 |
Summary\n- allow DingTalk clientSecret to be provided as a SecretInput string or env/file/exec reference\n- resolve SecretInput only on runtime paths that need the raw secret, while status/onboarding display normalized references\n- bound exec helper calls with a timeout and avoid resolving exec-backed secrets on access-token cache hits\n\nThis is split out from the closed combined PR #526 so SecretInput support can be reviewed independently from Gateway RPC compatibility. The approach is based on the official connector's SecretInput pattern while preserving existing string clientSecret compatibility.\n\n## Validation\n- pnpm test tests/unit/secret-input.test.ts tests/unit/secret-input-exec.test.ts tests/unit/auth-secret-input-cache.test.ts\n- pnpm type-check