fix(gateway): use runtime config for secret-backed talk#73286
Conversation
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟠 Channel plugin logout handler receives full resolved runtime config (possible secret exposure)
DescriptionThe Implications:
Vulnerable flow:
RecommendationMinimize secrets passed to plugins. Options (prefer least-privilege):
const runtimeCfg = context.getRuntimeConfig();
const channelCfg = {
channels: {
[channelId]: runtimeCfg.channels?.[channelId],
},
};
await logoutChannelAccount({
channelId,
accountId,
cfg: channelCfg as OpenClawConfig,
context,
plugin,
});If channel plugins genuinely must access other parts of config, document that as a trust boundary and ensure only trusted plugins can be installed/enabled. 2. 🟡 DoS in talk.config when base TTS provider config contains unresolved SecretRef token
Description
Impact:
Evidence of strict token normalization in a speech provider:
Vulnerable code (only strips const selectedBaseTts =
Object.keys(runtimeBaseTts).length > 0
? runtimeBaseTts
: stripUnresolvedSecretApiKeysFromBaseTtsProviders(sourceBaseTts);
...
function stripUnresolvedSecretApiKeyFromRecord(config: Record<string, unknown>) {
if (config.apiKey === undefined || typeof config.apiKey === "string") {
return config;
}
const { apiKey: _omit, ...rest } = config;
return rest;
}RecommendationStrip all secret-input fields that may be For example, restore the previous behavior by stripping both const BASE_TTS_PROVIDER_SECRET_INPUT_KEYS = ["apiKey", "token"] as const;
function stripUnresolvedSecretInputsFromRecord(config: Record<string, unknown>) {
let next: Record<string, unknown> | undefined;
for (const key of BASE_TTS_PROVIDER_SECRET_INPUT_KEYS) {
const v = config[key];
if (v === undefined || typeof v === "string") continue;
next ??= { ...config };
delete next[key];
}
return next ?? config;
}
function stripUnresolvedSecretInputsFromBaseTtsProviders(base: Record<string, unknown>) {
// keep the existing null-prototype hardening
...
const next = stripUnresolvedSecretInputsFromRecord(cfg);
...
}Additionally, consider applying this stripping regardless of whether Analyzed PR: #73286 at commit Last updated on: 2026-04-28T05:09:10Z |
PR SummaryMedium Risk Overview Refactors shared runtime selection into Adds/updates unit and suite tests to assert strict Talk provider resolvers receive runtime-resolved Reviewed by Cursor Bugbot for commit 10a5c84. Bugbot is set up for automated code reviews on this repo. Configure here. |
e33be98 to
10a5c84
Compare
Greptile SummaryThis PR fixes two related SecretRef-resolution bugs: Confidence Score: 4/5Safe to merge with awareness of the token-stripping regression in the fallback path. Both headline bugs are correctly fixed and covered by new tests. The only finding is a P2 regression: the replacement for stripUnresolvedSecretInputsFromProviderConfig only strips apiKey, not token, in the fallback path used when runtimeBaseTts is empty. The primary runtime path is unaffected, but the removed websocket test explicitly covered the token case — the new unit tests do not. src/gateway/server-methods/talk.ts — specifically the stripUnresolvedSecretApiKeyFromRecord fallback and whether token stripping should be restored there. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server-methods/talk.ts
Line: 436-444
Comment:
**`token` no longer stripped in the fallback path**
The old `stripUnresolvedSecretInputsFromProviderConfig` stripped both `apiKey` and `token` (via `BASE_TTS_PROVIDER_SECRET_INPUT_KEYS`). The replacement `stripUnresolvedSecretApiKeyFromRecord` only handles `apiKey`. The fallback branch in `resolveTalkResponseFromConfig` (when `Object.keys(runtimeBaseTts).length === 0`) now passes an unresolved `token` SecretRef directly to `speechProvider.resolveTalkConfig`. If a provider reads `baseTtsConfig.providers[id].token` and passes it to `normalizeResolvedSecretInputString`, it will throw — exactly the scenario the removed websocket test verified (that test explicitly set both `apiKey` and `token` as SecretRefs and confirmed no throw).
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(gateway): clarify talk config r..." | Re-trigger Greptile |
| function stripUnresolvedSecretApiKeyFromRecord( | ||
| config: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| if (config.apiKey === undefined || typeof config.apiKey === "string") { | ||
| return config; | ||
| } | ||
| const { apiKey: _omit, ...rest } = config; | ||
| return rest; | ||
| } |
There was a problem hiding this comment.
token no longer stripped in the fallback path
The old stripUnresolvedSecretInputsFromProviderConfig stripped both apiKey and token (via BASE_TTS_PROVIDER_SECRET_INPUT_KEYS). The replacement stripUnresolvedSecretApiKeyFromRecord only handles apiKey. The fallback branch in resolveTalkResponseFromConfig (when Object.keys(runtimeBaseTts).length === 0) now passes an unresolved token SecretRef directly to speechProvider.resolveTalkConfig. If a provider reads baseTtsConfig.providers[id].token and passes it to normalizeResolvedSecretInputString, it will throw — exactly the scenario the removed websocket test verified (that test explicitly set both apiKey and token as SecretRefs and confirmed no throw).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/talk.ts
Line: 436-444
Comment:
**`token` no longer stripped in the fallback path**
The old `stripUnresolvedSecretInputsFromProviderConfig` stripped both `apiKey` and `token` (via `BASE_TTS_PROVIDER_SECRET_INPUT_KEYS`). The replacement `stripUnresolvedSecretApiKeyFromRecord` only handles `apiKey`. The fallback branch in `resolveTalkResponseFromConfig` (when `Object.keys(runtimeBaseTts).length === 0`) now passes an unresolved `token` SecretRef directly to `speechProvider.resolveTalkConfig`. If a provider reads `baseTtsConfig.providers[id].token` and passes it to `normalizeResolvedSecretInputString`, it will throw — exactly the scenario the removed websocket test verified (that test explicitly set both `apiKey` and `token` as SecretRefs and confirmed no throw).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e33be98dd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (config.apiKey === undefined || typeof config.apiKey === "string") { | ||
| return config; | ||
| } | ||
| const { apiKey: _omit, ...rest } = config; | ||
| return rest; |
There was a problem hiding this comment.
Strip unresolved token refs before calling talk resolvers
stripUnresolvedSecretApiKeyFromRecord now removes only apiKey, but stripUnresolvedSecretApiKeysFromBaseTtsProviders uses it for messages.tts.providers.* entries that can also carry token SecretRefs. If a speech plugin’s resolveTalkConfig normalizes baseTtsConfig.providers.<id>.token with strict helpers, talk.config can throw on unresolved wrappers (especially when runtime config does not provide a resolved token for that provider), which regresses the previous behavior that stripped both secret fields from the base TTS map.
Useful? React with 👍 / 👎.
Summary
talk.configand channel logout could hand provider/channel callbacks the source config snapshot, leaving SecretRef wrappers unresolved on runtime paths.talk.configread scope.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
messages.ttscredentials.speech-corehelpers can be called with either the active source snapshot or runtime snapshot, so the runtime selection logic belongs in the shared runtime snapshot seam.Regression Test Plan (if applicable)
src/gateway/server-methods/talk.test.ts,src/gateway/server-methods/channels.start.test.ts,extensions/speech-core/src/tts.test.ts,src/gateway/server.talk-config.test.tsmessages.tts.providers.<id>.apiKeyand runtime timeout; channel logout receives runtime config; read-scopetalk.configstays redacted.User-visible / Behavior Changes
Talk Mode now discovers SecretRef-backed speech providers through
talk.configinstead of falling back to local speech when the provider resolver requires a concrete runtime API key.Diagram (if applicable)
Security Impact (required)
Yes, explain risk + mitigation: provider callbacks now receive the runtime-resolved config they already receive on execution paths;talk.configread-scope responses restore the source SecretRef shape and pass through existing redaction so credential material is not returned to clients.Repro + Verification
Environment
messages.tts.providers.<id>.apiKeyas SecretRefSteps
messages.tts.providers.<id>.apiKeythrough a SecretRef.talk.configwith read scope against a strict provider resolver.Expected
talk.configresponse remains redacted for read scope.Actual
Evidence
Human Verification (required)
pnpm test src/gateway/server-methods/talk.test.ts src/gateway/server.talk-config.test.ts; Blacksmith Testboxpnpm test src/gateway/server-methods/talk.test.ts src/gateway/server.talk-config.test.ts; Blacksmith Testboxpnpm check:changed.__proto__provider-key hardening.Review Conversations
Compatibility / Migration
Risks and Mitigations