Conversation
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
1. 🟠 Arbitrary environment variable read via models.json SecretRef for custom provider API keys
Description
Vulnerable code: const envVarName = apiKeyRef.id.trim();
...
const envValue = normalizeOptionalSecretInput((params.env ?? process.env)[envVarName]);
...
return { apiKey: envValue, source: ... };RecommendationRequire an explicit allowlist (or a strict env-var-name policy) before resolving env SecretRefs from Concrete options:
function canResolveEnvSecretRefInReadOnlyPath({ cfg, provider, id }: ...): boolean {
const providerConfig = cfg?.secrets?.providers?.[provider];
if (!providerConfig) return false; // do not auto-allow "default"
if (providerConfig.source !== "env") return false;
return Array.isArray(providerConfig.allowlist) && providerConfig.allowlist.includes(id);
}
This prevents a crafted 2. 🟠 Plaintext secrets forwarded to plugin registration during read-only CLI status scan
DescriptionThe CLI now forwards an explicit Because plugin loading executes third-party plugin code at import/registration time, this causes plaintext secret values to be exposed to any installed plugin during registry preload, even when the command intends Data flow:
Vulnerable code paths: // status scan: forwards resolved snapshot
await ensureCliPluginRegistryLoaded({
scope: "configured-channels",
routeLogsToStderr: true,
config: overview.cfg,
activationSourceConfig: overview.sourceConfig,
});// plugin loader: passes cfg into plugin API and executes plugin code
const api = buildPluginApi({
...,
config: cfg,
...,
});
await register(api);Impact:
RecommendationAvoid providing plugins a fully resolved config (with secret values) during read-only/preload flows. Options (pick one based on intended design):
await ensureCliPluginRegistryLoaded({
scope: "configured-channels",
routeLogsToStderr: true,
// do NOT pass resolved snapshot here
activationSourceConfig: overview.sourceConfig,
});
Also ensure plugins are never handed secret values unless the command explicitly requires them and the user has opted into secret exposure. 3. 🟡 Brittle error-message substring matching suppresses plugin failures in provider status resolution
Description
Vulnerable code: function isUnresolvedSecretRefResolutionError(error: unknown): boolean {
return (
error instanceof Error &&
typeof error.message === "string" &&
/unresolved SecretRef/i.test(error.message)
);
}
...
} catch (error) {
if (!isUnresolvedSecretRefResolutionError(error)) {
throw error;
}
map.set(..., { state: "not configured", configured: false });
continue;
}RecommendationAvoid classifying resolution failures by Option A (recommended): use the existing typed error class import { SecretRefResolutionError } from "../secrets/resolve.js";
function isUnresolvedSecretRefResolutionError(error: unknown): boolean {
return error instanceof SecretRefResolutionError;
}Option B: If cross-package Also consider logging/including diagnostics when suppressing the error (at least at debug level), so unexpected failures aren’t silently hidden. Analyzed PR: #66818 at commit Last updated on: 2026-04-14T23:06:32Z |
openclaw-barnacle
Bot
added
docs
![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: bc8df503fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (ref.source === "env") { | ||
| const envId = ref.id.trim(); | ||
| return envId || NON_ENV_SECRETREF_MARKER; |
There was a problem hiding this comment.
When models.providers.<id>.apiKey is a SecretRef object with source: "env", this branch returns the raw env var name (for example MY_CUSTOM_KEY). In resolveUsableCustomProviderApiKey, unknown env names are not recognized by isNonSecretApiKeyMarker, so they are treated as literal credentials and sent as the provider API key instead of reading process.env[MY_CUSTOM_KEY]. This breaks custom providers that use non-built-in env var names and causes authentication failures whenever the env-ref id is outside the known marker list.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces a shared Confidence Score: 5/5Safe to merge; both findings are P2 style and edge-case behavioral notes that do not block the main use cases. All findings are P2: one is dead code after a guaranteed throw (Telegram), and the other is a behavioral edge case for non-standard env var names in custom provider SecretRefs (model-auth) that was already broken before this PR. The primary paths — status resiliency, Slack route registration, Firecrawl, xAI, Telegram env refs — are well-implemented and tested. src/agents/model-auth.ts — the getCustomProviderApiKey env-ref branch for non-standard env var names and the corresponding test coverage gap. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/telegram/src/token.ts
Line: 34-39
Comment:
**Dead code after strict-mode SecretRef throw**
`resolveSecretInputString` with `mode: "strict"` will always throw here because we already know the value is `configured_unavailable` from the first inspect-mode call on the same input — the ref is still unresolved. The `return undefined` on line 39 is unreachable.
If `createUnresolvedSecretInputError` were exported from `types.secrets.ts`, throwing it directly using the `ref` already in scope from the inspect call would be cleaner than a double-parse. As-is, the dead-code line should simply be removed.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/model-auth.ts
Line: 80-88
Comment:
**Non-standard env var IDs returned as literal values**
For env SecretRefs whose `id` is not in the known-marker set, this branch returns the raw env var name string. In `resolveUsableCustomProviderApiKey`, `isNonSecretApiKeyMarker` returns `false` for unknown names, so the function returns the env var name as if it were a literal API credential — not the resolved value.
The new test uses `OPENAI_API_KEY`, which is already in `listKnownEnvApiKeyMarkers()` and therefore takes the correct env-lookup path. A user with a custom env var name not in that set would get the var name itself used as the credential.
Before this PR, `normalizeOptionalSecretInput` stripped all SecretRef objects, so `getCustomProviderApiKey` returned `undefined` and the caller saw `null` (not configured). The behavior for this edge case has changed.
Consider returning `NON_ENV_SECRETREF_MARKER` for env refs whose ID is not in the known list, matching the non-env branch, to keep the "managed but unresolvable here" semantics uniform.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Telegram: resolve env SecretRef tokens a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| resolveSecretInputString({ | ||
| value: params.value, | ||
| path: params.path, | ||
| mode: "strict", | ||
| }); | ||
| return undefined; |
There was a problem hiding this comment.
resolveSecretInputString with mode: "strict" will always throw here because we already know the value is configured_unavailable from the first inspect-mode call on the same input — the ref is still unresolved. The return undefined on line 39 is unreachable.
If createUnresolvedSecretInputError were exported from types.secrets.ts, throwing it directly using the ref already in scope from the inspect call would be cleaner than a double-parse. As-is, the dead-code line should simply be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/telegram/src/token.ts
Line: 34-39
Comment:
**Dead code after strict-mode SecretRef throw**
`resolveSecretInputString` with `mode: "strict"` will always throw here because we already know the value is `configured_unavailable` from the first inspect-mode call on the same input — the ref is still unresolved. The `return undefined` on line 39 is unreachable.
If `createUnresolvedSecretInputError` were exported from `types.secrets.ts`, throwing it directly using the `ref` already in scope from the inspect call would be cleaner than a double-parse. As-is, the dead-code line should simply be removed.
How can I resolve this? If you propose a fix, please make it concise.| const ref = coerceSecretRef(entry?.apiKey); | ||
| if (!ref) { | ||
| return undefined; | ||
| } | ||
| if (ref.source === "env") { | ||
| const envId = ref.id.trim(); | ||
| return envId || NON_ENV_SECRETREF_MARKER; | ||
| } | ||
| return NON_ENV_SECRETREF_MARKER; |
There was a problem hiding this comment.
For env SecretRefs whose id is not in the known-marker set, this branch returns the raw env var name string. In resolveUsableCustomProviderApiKey, isNonSecretApiKeyMarker returns false for unknown names, so the function returns the env var name as if it were a literal API credential — not the resolved value.
The new test uses OPENAI_API_KEY, which is already in listKnownEnvApiKeyMarkers() and therefore takes the correct env-lookup path. A user with a custom env var name not in that set would get the var name itself used as the credential.
Before this PR, normalizeOptionalSecretInput stripped all SecretRef objects, so getCustomProviderApiKey returned undefined and the caller saw null (not configured). The behavior for this edge case has changed.
Consider returning NON_ENV_SECRETREF_MARKER for env refs whose ID is not in the known list, matching the non-env branch, to keep the "managed but unresolvable here" semantics uniform.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-auth.ts
Line: 80-88
Comment:
**Non-standard env var IDs returned as literal values**
For env SecretRefs whose `id` is not in the known-marker set, this branch returns the raw env var name string. In `resolveUsableCustomProviderApiKey`, `isNonSecretApiKeyMarker` returns `false` for unknown names, so the function returns the env var name as if it were a literal API credential — not the resolved value.
The new test uses `OPENAI_API_KEY`, which is already in `listKnownEnvApiKeyMarkers()` and therefore takes the correct env-lookup path. A user with a custom env var name not in that set would get the var name itself used as the credential.
Before this PR, `normalizeOptionalSecretInput` stripped all SecretRef objects, so `getCustomProviderApiKey` returned `undefined` and the caller saw `null` (not configured). The behavior for this edge case has changed.
Consider returning `NON_ENV_SECRETREF_MARKER` for env refs whose ID is not in the known list, matching the non-env branch, to keep the "managed but unresolvable here" semantics uniform.
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: f4003cde86
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (resolved.ref.source === "env") { | ||
| return normalizeSecretInputString(process.env[resolved.ref.id]); |
There was a problem hiding this comment.
This new runtime branch resolves env-backed SecretRefs by directly reading process.env[resolved.ref.id], which skips provider validation and secrets.providers.<name>.allowlist checks that the normal secret resolver enforces. As a result, a config can point channels.telegram.botToken at any env var name (including disallowed ids or providers that should not resolve env refs) and the value is accepted if present, which can silently bypass secret policy and leak unintended env values in Telegram API calls. The same direct-lookup pattern was also added in the xAI/Firecrawl helpers, so this should be fixed as a policy-consistent SecretRef resolution path rather than a raw env read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86fac767a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!envVarName) { | ||
| return null; | ||
| } | ||
| const envValue = normalizeOptionalSecretInput((params.env ?? process.env)[envVarName]); |
There was a problem hiding this comment.
resolveUsableCustomProviderApiKey now reads process.env[ref.id] directly for env SecretRefs, but this path never validates ref.provider (configured provider, source match, or allowlist). That means a config like models.providers.custom.apiKey = {source:"env", provider:"restricted", id:"ANY_VAR"} will be accepted whenever ANY_VAR is set, bypassing the secret-provider policy checks that src/secrets/resolve.ts enforces for SecretRef resolution.
Useful? React with 👍 / 👎.
| if (envVarName !== XAI_API_KEY_ENV_VAR) { | ||
| return { status: "blocked" }; | ||
| } | ||
| const envValue = normalizeSecretInputString(process.env[envVarName]); |
There was a problem hiding this comment.
readConfiguredRuntimeApiKey resolves env SecretRefs by directly reading process.env[envVarName] once the id matches XAI_API_KEY, but it does not verify that the referenced secret provider exists, is source: "env", or allowlists that id. In runtime paths without a resolved snapshot, this lets plugins.entries.xai.config.webSearch.apiKey bypass secrets.providers.* policy and still consume an env value.
Useful? React with 👍 / 👎.
| const envVarName = resolved.ref.id.trim(); | ||
| if (envVarName !== FIRECRAWL_API_KEY_ENV_VAR) { | ||
| return { status: "blocked" }; | ||
| } | ||
| const envValue = normalizeSecretInput(process.env[envVarName]); |
There was a problem hiding this comment.
resolveConfiguredSecret reads process.env[envVarName] for env SecretRefs (when the id is FIRECRAWL_API_KEY) without validating provider existence/source/allowlist. As a result, plugins.entries.firecrawl.config.webSearch.apiKey (or related Firecrawl key fields) can bypass secrets.providers restrictions in source-config runtime flows and still resolve from environment state.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/secretref-inspect-strict-runtime-snapshots
branch
from
48bf142 to
70f12cd
Compare
joshavant
force-pushed
the
feat/secretref-inspect-strict-runtime-snapshots
branch
from
70f12cd to
3ea81d6
Compare
|
Landed via temp rebase onto main. Thanks @joshavant! |
…runtime paths (openclaw#66818) * Config: add inspect/strict SecretRef string resolver * CLI: pass resolved/source config snapshots to plugin preload * Slack: keep HTTP route registration config-only * Providers: normalize SecretRef handling for auth and web tools * Secrets: add Exa web search target to registry and docs * Telegram: resolve env SecretRef tokens at runtime * Agents: resolve custom provider env SecretRef ids * Providers: fail closed on blocked SecretRef fallback * Telegram: enforce env SecretRef policy for runtime token refs * Status/Providers/Telegram: tighten SecretRef preload and fallback handling * Providers: enforce env SecretRef policy checks in fallback auth paths * fix: add SecretRef lifecycle changelog entry (openclaw#66818) (thanks @joshavant)
…runtime paths (openclaw#66818) * Config: add inspect/strict SecretRef string resolver * CLI: pass resolved/source config snapshots to plugin preload * Slack: keep HTTP route registration config-only * Providers: normalize SecretRef handling for auth and web tools * Secrets: add Exa web search target to registry and docs * Telegram: resolve env SecretRef tokens at runtime * Agents: resolve custom provider env SecretRef ids * Providers: fail closed on blocked SecretRef fallback * Telegram: enforce env SecretRef policy for runtime token refs * Status/Providers/Telegram: tighten SecretRef preload and fallback handling * Providers: enforce env SecretRef policy checks in fallback auth paths * fix: add SecretRef lifecycle changelog entry (openclaw#66818) (thanks @joshavant)
…runtime paths (openclaw#66818) * Config: add inspect/strict SecretRef string resolver * CLI: pass resolved/source config snapshots to plugin preload * Slack: keep HTTP route registration config-only * Providers: normalize SecretRef handling for auth and web tools * Secrets: add Exa web search target to registry and docs * Telegram: resolve env SecretRef tokens at runtime * Agents: resolve custom provider env SecretRef ids * Providers: fail closed on blocked SecretRef fallback * Telegram: enforce env SecretRef policy for runtime token refs * Status/Providers/Telegram: tighten SecretRef preload and fallback handling * Providers: enforce env SecretRef policy checks in fallback auth paths * fix: add SecretRef lifecycle changelog entry (openclaw#66818) (thanks @joshavant)
…runtime paths (openclaw#66818) * Config: add inspect/strict SecretRef string resolver * CLI: pass resolved/source config snapshots to plugin preload * Slack: keep HTTP route registration config-only * Providers: normalize SecretRef handling for auth and web tools * Secrets: add Exa web search target to registry and docs * Telegram: resolve env SecretRef tokens at runtime * Agents: resolve custom provider env SecretRef ids * Providers: fail closed on blocked SecretRef fallback * Telegram: enforce env SecretRef policy for runtime token refs * Status/Providers/Telegram: tighten SecretRef preload and fallback handling * Providers: enforce env SecretRef policy checks in fallback auth paths * fix: add SecretRef lifecycle changelog entry (openclaw#66818) (thanks @joshavant)
1 participant
Summary
src/config/types.secrets.ts, exported viasrc/plugin-sdk/secret-input.ts.src/cli/plugin-registry-loader.ts,src/commands/status.scan.fast-json.ts).inspectAccountand skip throwing accounts (src/commands/agents.providers.ts).extensions/slack/src/http/plugin-routes.ts).Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
openclaw statusfails on file-backed SecretRef configs because route/plugin loading re-reads unresolved raw config #57272Root Cause (if applicable)
Regression Test Plan (if applicable)
src/config/types.secrets.resolution.test.tssrc/cli/plugin-registry-loader.test.tssrc/commands/status.scan.test.tssrc/commands/agents.providers.test.tsextensions/slack/src/http/plugin-routes.test.tssrc/agents/model-auth.test.tsextensions/xai/src/tool-auth-shared.test.tsextensions/firecrawl/src/firecrawl-tools.test.tssrc/secrets/target-registry.test.tssrc/cli/command-secret-targets.test.tsextensions/telegram/src/token.test.tsUser-visible / Behavior Changes
openclaw agents listand other read-only/status paths are more resilient when unresolved SecretRefs exist in channel/provider configs.Diagram (if applicable)
Security Impact (required)
No)Yes)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm build).pnpm plugin-sdk:api:check,pnpm plugin-sdk:api:gen, re-check pass).pnpm testdid not finish green due unrelated failing shards outside this PR surface.Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations
Risk:
Risk: