fix(status): resolve SecretRef tolerantly in channel token resolution#34726
fix(status): resolve SecretRef tolerantly in channel token resolution#34726Sid-Qin wants to merge 1 commit intoopenclaw:mainfrom
Conversation
openclaw status crashed on unresolved SecretRef objects because normalizeResolvedSecretInputString threw for non-string inputs. Meanwhile openclaw doctor probed the already-resolved gateway process and reported OK. Add normalizeSecretInputStringTolerant that returns undefined for unresolved SecretRefs, and use it in Telegram/Discord/Slack token resolution so status degrades gracefully instead of crashing. Closes openclaw#34292
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🔵 Fail-open token resolution when config contains unresolved SecretRef (falls back to env)
DescriptionToken normalization was changed to use Previously,
Now
Security impact:
Vulnerable flow:
RecommendationFail closed when a Options:
import { normalizeResolvedSecretInputString } from "../config/types.secrets.js";
export function normalizeDiscordToken(raw: unknown, path: string): string | undefined {
const trimmed = normalizeResolvedSecretInputString({ value: raw, path });
return trimmed?.replace(/^Bot\s+/i, "");
}
const { value, hasRef } = normalizeSecretInputStringTolerantWithRefInfo(...);
if (hasRef) throw new Error(`${path}: unresolved SecretRef`);Also keep/use the 2. 🔵 Slack tokens: unresolved SecretRef now silently ignored, enabling env-var credential fallback
DescriptionSlack token parsing was changed from strict
Vulnerable code (new behavior): export function resolveSlackBotToken(raw?: unknown, _path?: string): string | undefined {
return normalizeSecretInputStringTolerant({ value: raw });
}Security impact example (data flow):
RecommendationAvoid tolerant normalization for runtime authentication credentials, or explicitly detect unresolved SecretRefs and fail closed. Option A (recommended): keep strict behavior for Slack tokens import { normalizeResolvedSecretInputString } from "../config/types.secrets.js";
export function resolveSlackBotToken(raw?: unknown, path = "channels.slack.botToken") {
return normalizeResolvedSecretInputString({ value: raw, path });
}Option B: if env fallback is desired, only allow it when the config value is truly absent (not a SecretRef) import { normalizeSecretInputString, resolveSecretInputRef } from "../config/types.secrets.js";
export function resolveSlackBotToken(raw?: unknown, path = "channels.slack.botToken") {
const direct = normalizeSecretInputString(raw);
if (direct) return direct;
const { ref } = resolveSecretInputRef({ value: raw });
if (ref) throw new Error(`${path}: unresolved SecretRef`);
return undefined;
}This prevents silently switching credential sources when a SecretRef is present but unresolved. Analyzed PR: #34726 at commit Last updated on: 2026-03-04T17:16:36Z |
openclaw-barnacle
bot
added
channel: discord
Greptile SummaryThis PR fixes a crash in The fix is minimal, well-scoped, and backward-compatible. The core logic change is correct: diagnostic/status code paths now degrade gracefully rather than crashing.
Confidence Score: 4/5
Last reviewed commit: e66c6d9 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { ref } = resolveSecretInputRef({ | ||
| value: params.value, | ||
| refValue: params.refValue, | ||
| defaults: params.defaults, | ||
| }); | ||
| if (ref) { | ||
| return undefined; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
Dead code — both branches of if (ref) return undefined
The resolveSecretInputRef call and the if (ref) branch below it serve no purpose: both paths return undefined. This means the entire function is functionally equivalent to return normalizeSecretInputString(params.value). The extra branching is misleading — a future reader might assume the ref-truthy and ref-falsy paths behave differently when they do not.
| const { ref } = resolveSecretInputRef({ | |
| value: params.value, | |
| refValue: params.refValue, | |
| defaults: params.defaults, | |
| }); | |
| if (ref) { | |
| return undefined; | |
| } | |
| return undefined; | |
| return undefined; | |
| } |
Or simplify the whole function body to:
export function normalizeSecretInputStringTolerant(params: {
value: unknown;
refValue?: unknown;
defaults?: SecretDefaults;
}): string | undefined {
return normalizeSecretInputString(params.value);
}If the resolveSecretInputRef call is kept intentionally (e.g. for future logging of which ref was skipped), that intent should be documented with a comment so the dead-code appearance is explained.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/types.secrets.ts
Line: 168-176
Comment:
**Dead code — both branches of `if (ref)` return `undefined`**
The `resolveSecretInputRef` call and the `if (ref)` branch below it serve no purpose: both paths return `undefined`. This means the entire function is functionally equivalent to `return normalizeSecretInputString(params.value)`. The extra branching is misleading — a future reader might assume the `ref`-truthy and `ref`-falsy paths behave differently when they do not.
```suggestion
return undefined;
}
```
Or simplify the whole function body to:
```ts
export function normalizeSecretInputStringTolerant(params: {
value: unknown;
refValue?: unknown;
defaults?: SecretDefaults;
}): string | undefined {
return normalizeSecretInputString(params.value);
}
```
If the `resolveSecretInputRef` call is kept intentionally (e.g. for future logging of which ref was skipped), that intent should be documented with a comment so the dead-code appearance is explained.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e66c6d95fb
ℹ️ 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".
| const accountToken = normalizeSecretInputStringTolerant({ | ||
| value: accountCfg?.botToken, | ||
| path: `channels.telegram.accounts.${accountId}.botToken`, | ||
| }); |
There was a problem hiding this comment.
Using normalizeSecretInputStringTolerant here makes an unresolved per-account botToken look the same as “not configured”, so resolveTelegramToken now falls through to top-level channels.telegram.botToken for non-default accounts instead of stopping on the explicit account override. In the context I checked (src/telegram/token.ts resolution order), a config like accounts.work.botToken: {source:"env",...} plus a top-level token will now run work with the wrong bot token, whereas before this commit it threw on unresolved SecretRef; that can misroute sends/status for multi-account Telegram setups.
Useful? React with 👍 / 👎.
2 participants
Summary
openclaw statuscrashed on unresolved SecretRef objects in channel token config, whileopenclaw doctorreported OK because it probed the already-resolved gateway process.normalizeSecretInputStringTolerantthat returns undefined for unresolved SecretRefs instead of throwing. Used it in Telegram/Discord/Slack token resolution so status degrades gracefully.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
openclaw statusno longer crashes on unresolved SecretRefs — shows "none" or falls back to environment variableSecurity Impact (required)
NoNo(tolerant resolution, same underlying logic)NoNoNoRepro + Verification
Environment
Steps
openclaw status(uses config before resolution)Expected
Actual
Evidence
Updated tests verify unresolved SecretRef returns undefined instead of throwing
Files changed (7 files)
src/config/types.secrets.ts— Added tolerant normalizersrc/telegram/token.ts— Use tolerant normalizersrc/discord/token.ts— Use tolerant normalizersrc/slack/token.ts— Use tolerant normalizersrc/plugin-sdk/index.ts— Export tolerant normalizersrc/telegram/token.test.ts— Updated testssrc/discord/token.test.ts— Updated testsHuman Verification (required)
Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
Risks and Mitigations