fix: mark env-ref providers as SecretRef-managed to prevent plaintext persistence (#42355) by uninhibite-scholar · Pull Request #42426 · openclaw/openclaw

uninhibite-scholar · 2026-03-10T18:12:34Z

Summary

This PR fixes a security issue where custom provider API keys configured via environment variables were being persisted as plaintext in agent-local models.json during heartbeat-triggered agent activation.

Problem

The secretRefManagedProviders set was only populated when:

API key came from auth profile with non-plaintext source

But it missed the case where:

API key came from environment variable (env-ref)

This caused:

Heartbeat-triggered activation → wrote plaintext API key to models.json
Other activation paths → correctly wrote env var name
Inconsistent behavior depending on activation path

Root Cause

In src/agents/models-config.providers.ts, the logic only checked profileApiKey.source !== "plaintext", but when API key is resolved from environment variable, profileApiKey may be undefined or have source env-ref.

Solution

Updated the SecretRef-managed detection to include env-ref sources:

const isSecretRefManaged =
  (profileApiKey && profileApiKey.source !== "plaintext") ||
  (fromEnv && profileApiKey?.source === "env-ref");

Changes

src/agents/models-config.providers.ts:
- Updated SecretRef-managed detection logic
- Added env-ref source check
- Added explanatory comment

Impact

Prevents plaintext API key persistence in models.json
Ensures consistent SecretRef behavior across all activation paths
Fixes security issue reported in [Bug]: [Security] [2026.3.8]Heartbeat-triggered agent activation persists custom provider API key as plaintext in agent-local models.json #42355
Backward compatible (only affects internal marking logic)

- Extract path from image content blocks in tool results - This fixes Feishu image delivery when using the read tool - Previously, media paths were only extracted from MEDIA: tokens in text content, but read tool returns paths in image blocks Fixes issue where read tool image results lost media before final outbound payload assembly in Feishu channel. Closes openclaw#41744

- Extract MEDIA: tokens even when wrapped in code blocks - LLMs frequently format paths in code fences - Previously these were silently ignored causing delivery failures - Now tokens are extracted and delivered correctly Fixes openclaw#41966

… persistence - Add env-ref source to SecretRef-managed detection - Prevents API keys from being written as plaintext in models.json - Fixes heartbeat-triggered agent activation path - Ensures consistent SecretRef behavior across all activation paths Fixes openclaw#42355

openclaw-barnacle · 2026-03-10T18:13:01Z

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

greptile-apps · 2026-03-10T18:15:04Z

Greptile Summary

This PR attempts to fix a security issue where provider API keys from environment variables were being persisted as plaintext in models.json, and also extends media path extraction to read paths directly from image content blocks. However, two critical bugs were introduced that need to be resolved before merging.

Key issues:

src/media/parse.ts — Runtime crash (syntax error): The refactor of splitMediaFromOutput introduced a duplicate const matches declaration in the same block scope (an identical Array.from(line.matchAll(MEDIA_TOKEN_RE)) call appears at both line 134 and line 146). JavaScript/TypeScript will throw a SyntaxError at parse time, making the entire splitMediaFromOutput function unusable for any input with a MEDIA: token. This is a regression that will break media delivery.
src/agents/models-config.providers.ts — Incomplete security fix: The new isSecretRefManaged condition (fromEnv && profileApiKey?.source === "env-ref") does not cover the case the PR description identifies as the primary scenario: profileApiKey being undefined when the key comes purely from an env var. In that case the clause short-circuits to false, the provider is never added to secretRefManagedProviders, and the plaintext-persistence bug remains.
src/agents/pi-embedded-subscribe.tools.ts — Looks correct: The new fallback to extract path from image content blocks is logically sound.

Confidence Score: 0/5

This PR introduces a runtime crash in parse.ts and leaves the security vulnerability it claims to fix still exploitable — it is not safe to merge.
Two critical defects block merge: (1) a duplicate const matches declaration in src/media/parse.ts will throw a SyntaxError at module load time, breaking all media delivery; (2) the security fix in src/agents/models-config.providers.ts fails for the main scenario (undefined profileApiKey) due to an incorrect guard expression, meaning plaintext API keys can still be persisted.
src/media/parse.ts (syntax error / crash) and src/agents/models-config.providers.ts (incomplete security fix) both require fixes before this PR is mergeable.

Comments Outside Diff (1)

src/media/parse.ts, line 146-151 (link)

Duplicate const matches declaration — will crash at runtime

matches is already declared with const at line 134, and is declared again with const at line 146 in the same block scope. JavaScript/TypeScript will throw a SyntaxError: Identifier 'matches' has already been declared at parse time, making splitMediaFromOutput completely broken for any input containing a MEDIA: token.

The second block (lines 146–151) is a leftover copy-paste from the original code that should have been removed when the first early-exit block was added. The matches variable computed at line 134 is already correct; the duplicate check here is redundant.

(Remove lines 146–151 entirely; they duplicate the identical check already performed at lines 134–139.)

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/media/parse.ts
Line: 146-151

Comment:
**Duplicate `const matches` declaration — will crash at runtime**

`matches` is already declared with `const` at line 134, and is declared again with `const` at line 146 in the same block scope. JavaScript/TypeScript will throw a `SyntaxError: Identifier 'matches' has already been declared` at parse time, making `splitMediaFromOutput` completely broken for any input containing a `MEDIA:` token.

The second block (lines 146–151) is a leftover copy-paste from the original code that should have been removed when the first early-exit block was added. The `matches` variable computed at line 134 is already correct; the duplicate check here is redundant.



(Remove lines 146–151 entirely; they duplicate the identical check already performed at lines 134–139.)

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 124d1d8}

greptile-apps · 2026-03-10T18:15:11Z

src/agents/models-config.providers.ts

+          const isSecretRefManaged =
+            (profileApiKey && profileApiKey.source !== "plaintext") ||
+            (fromEnv && profileApiKey?.source === "env-ref");


Incomplete security fix — env-ref case not marked when profileApiKey is undefined

The PR description explicitly states: "when API key is resolved from environment variable, profileApiKey may be undefined or have source env-ref". However, the new second clause only marks the provider as SecretRef-managed when profileApiKey?.source === "env-ref". If profileApiKey is undefined (the common env-var-only path), profileApiKey?.source evaluates to undefined, which does not equal "env-ref", so the whole clause is false:

fromEnv is truthy (an env var name string)

profileApiKey is undefined

fromEnv && profileApiKey?.source === "env-ref" evaluates to true && false → false

The provider is therefore not added to secretRefManagedProviders, leaving the original plaintext-persistence bug intact for the most common env-var-only scenario.

The fix should mark as SecretRef-managed whenever fromEnv resolves to a value, regardless of whether profileApiKey is present:

Suggested change

const isSecretRefManaged =

(profileApiKey && profileApiKey.source !== "plaintext") ||

(fromEnv && profileApiKey?.source === "env-ref");

const isSecretRefManaged =

(profileApiKey && profileApiKey.source !== "plaintext") ||

Boolean(fromEnv);

Prompt To Fix With AI

This is a comment left during a code review. Path: src/agents/models-config.providers.ts Line: 392-394 Comment: **Incomplete security fix — env-ref case not marked when `profileApiKey` is `undefined`** The PR description explicitly states: _"when API key is resolved from environment variable, `profileApiKey` may be **undefined** or have source `env-ref`"_. However, the new second clause only marks the provider as SecretRef-managed when `profileApiKey?.source === "env-ref"`. If `profileApiKey` is `undefined` (the common env-var-only path), `profileApiKey?.source` evaluates to `undefined`, which does **not** equal `"env-ref"`, so the whole clause is `false`: - `fromEnv` is truthy (an env var name string) - `profileApiKey` is `undefined` - `fromEnv && profileApiKey?.source === "env-ref"` evaluates to `true && false` → `false` The provider is therefore **not** added to `secretRefManagedProviders`, leaving the original plaintext-persistence bug intact for the most common env-var-only scenario. The fix should mark as SecretRef-managed whenever `fromEnv` resolves to a value, regardless of whether `profileApiKey` is present: ```suggestion const isSecretRefManaged = (profileApiKey && profileApiKey.source !== "plaintext") || Boolean(fromEnv); ``` How can I resolve this? If you propose a fix, please make it concise.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 124d1d8327

ℹ️ 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".

chatgpt-codex-connector · 2026-03-10T18:18:36Z

src/agents/models-config.providers.ts

+            (profileApiKey && profileApiKey.source !== "plaintext") ||
+            (fromEnv && profileApiKey?.source === "env-ref");


Mark env-only API keys as secret-managed

This condition still misses the env-only case the patch is trying to fix. When a provider key comes from fromEnv but there is no auth profile, profileApiKey is undefined, so (fromEnv && profileApiKey?.source === "env-ref") is false and the provider is not added to secretRefManagedProviders; because the first clause already covers all non-plaintext profile sources, the new second clause is effectively redundant. In heartbeat-triggered normalization, that leaves env-resolved keys eligible to be persisted as plaintext in models.json.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-10T18:18:36Z

src/media/parse.ts

+      foundMediaInFence = true;
+    }
+
    const matches = Array.from(line.matchAll(MEDIA_TOKEN_RE));


Remove duplicate matches declaration

The same block declares const matches twice, which is a duplicate lexical declaration in one scope. This causes a parse/type-check failure (Identifier 'matches' has already been declared) before runtime, so consumers of splitMediaFromOutput cannot load this module and reply/tool media parsing paths break.

Useful? React with 👍 / 👎.

joshavant · 2026-03-10T23:50:00Z

Thanks @uninhibite-scholar for the contribution and for moving this forward.

This work is now covered by the merged superseding PR #42554:
#42554

Appreciate the help here.

uninhibite-scholar added 3 commits March 10, 2026 13:25

openclaw-barnacle bot added agents Agent runtime and tooling size: S r: too-many-prs labels Mar 10, 2026

openclaw-barnacle bot closed this Mar 10, 2026

greptile-apps bot reviewed Mar 10, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 10, 2026

View reviewed changes

joshavant mentioned this pull request Mar 10, 2026

SecretRef: harden custom/provider secret persistence and reuse #42554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: mark env-ref providers as SecretRef-managed to prevent plaintext persistence (#42355)#42426

fix: mark env-ref providers as SecretRef-managed to prevent plaintext persistence (#42355)#42426
uninhibite-scholar wants to merge 3 commits intoopenclaw:mainfrom
uninhibite-scholar:fix/secretref-env-ref-marking-42355

uninhibite-scholar commented Mar 10, 2026

Uh oh!

openclaw-barnacle bot commented Mar 10, 2026

Uh oh!

greptile-apps bot commented Mar 10, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

greptile-apps bot Mar 10, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 10, 2026

Uh oh!

chatgpt-codex-connector bot Mar 10, 2026

Uh oh!

joshavant commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		(profileApiKey && profileApiKey.source !== "plaintext") \|\|
		(fromEnv && profileApiKey?.source === "env-ref");

Uh oh!

Conversation

uninhibite-scholar commented Mar 10, 2026

Summary

Problem

Root Cause

Solution

Changes

Impact

Related

Uh oh!

openclaw-barnacle bot commented Mar 10, 2026

Uh oh!

greptile-apps bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 0/5

Comments Outside Diff (1)

Uh oh!

greptile-apps bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

joshavant commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps bot commented Mar 10, 2026 •

edited

Loading