fix(models): avoid non-env SecretRef plaintext persistence by ADKcodeXD · Pull Request #34362 · openclaw/openclaw

ADKcodeXD · 2026-03-04T11:31:28Z

Summary

This PR prevents plaintext key material from being serialized to disk when normalizing provider configs that use non-env SecretRef values.

Problem

For non-env SecretRef inputs, the normalize-keys path could carry through plaintext fields in serialized output, risking accidental persistence of sensitive material.

Fix

Ensure normalization for non-env SecretRef strips plaintext key fields before serialization.
Keep expected behavior for env-backed SecretRef unchanged.
Add/adjust tests to lock this behavior.

Verification

node node_modules/vitest/vitest.mjs run src/agents/models-config.providers.normalize-keys.test.ts
Result: 1 file passed, 4 tests passed.

Closes #34335.

greptile-apps · 2026-03-04T11:35:00Z

Greptile Summary

This PR fixes a security issue where plaintext key material from non-env SecretRef credentials (e.g. exec, file, keychain sources) could be inadvertently serialized into models.json via the resolveApiKeyFromProfiles helper in models-config.providers.ts.

The root cause was evaluation order: the old code checked the raw cred.key / cred.token fields first, so any co-located plaintext value would be returned before the keyRef was ever inspected. The fix inverts the priority — keyRef is now checked first. If it resolves to a non-env source, the loop immediately continues without touching the plaintext field; if it resolves to an env source, the env-var name is returned as before; if no keyRef exists at all, the plaintext field is still used (unchanged, intentional path for manually entered keys).

Key changes:

resolveApiKeyFromProfiles in src/agents/models-config.providers.ts: reordered checks for both api_key and token credential types so that a present non-env SecretRef suppresses the plaintext fallback.
Two new test cases added to models-config.providers.normalize-keys.test.ts verifying the non-env SecretRef suppression and the env SecretRef pass-through.
The token-type credential path receives the identical fix but has no corresponding new test case, leaving that branch's behavior unverified by tests.

Confidence Score: 4/5

This PR is safe to merge. It is a targeted, minimal security fix with correct logic and no functional regressions.
The fix correctly inverts the evaluation order in resolveApiKeyFromProfiles to prevent plaintext SecretRef values from being serialized. The logic is sound for both api_key and token branches, and new tests verify the critical api_key path. The only gap is the lack of symmetrical tests for the token credential type, which carries the same code change but is untested.
src/agents/models-config.providers.normalize-keys.test.ts (missing test cases for token credential type with non-env and env SecretRefs)

_{Last reviewed commit: 4610b17}

greptile-apps · 2026-03-04T11:35:04Z

src/agents/models-config.providers.normalize-keys.test.ts

+  it("does not copy plaintext api keys from non-env SecretRefs into providers", async () => {
+    const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-agent-"));
+    try {
+      await fs.writeFile(
+        path.join(agentDir, "auth-profiles.json"),
+        JSON.stringify({
+          version: 1,
+          profiles: {
+            "openai:default": {
+              id: "openai:default",
+              provider: "openai",
+              type: "api_key",
+              key: "sk-live-from-exec",
+              keyRef: { source: "exec", provider: "default", id: "keychain:openai" },
+            },
+          },
+          providerOrder: { openai: ["openai:default"] },
+        }),
+      );
+      const providers: NonNullable<NonNullable<OpenClawConfig["models"]>["providers"]> = {
+        openai: {
+          baseUrl: "https://api.openai.com/v1",
+          api: "openai-completions",
+          models: [{ id: "gpt-4.1-mini", name: "GPT", input: ["text"], reasoning: false }],
+        },
+      };
+
+      const normalized = normalizeProviders({ providers, agentDir });
+      expect(normalized?.openai?.apiKey).toBeUndefined();
+    } finally {
+      await fs.rm(agentDir, { recursive: true, force: true });
+    }
+  });
+
+  it("uses env var name when auth profile uses env SecretRef", async () => {
+    const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-agent-"));
+    try {
+      await fs.writeFile(
+        path.join(agentDir, "auth-profiles.json"),
+        JSON.stringify({
+          version: 1,
+          profiles: {
+            "openai:default": {
+              id: "openai:default",
+              provider: "openai",
+              type: "api_key",
+              keyRef: { source: "env", provider: "default", id: "OPENAI_API_KEY" },
+            },
+          },
+          providerOrder: { openai: ["openai:default"] },
+        }),
+      );
+      const providers: NonNullable<NonNullable<OpenClawConfig["models"]>["providers"]> = {
+        openai: {
+          baseUrl: "https://api.openai.com/v1",
+          api: "openai-completions",
+          models: [{ id: "gpt-4.1-mini", name: "GPT", input: ["text"], reasoning: false }],
+        },
+      };
+
+      const normalized = normalizeProviders({ providers, agentDir });
+      expect(normalized?.openai?.apiKey).toBe("OPENAI_API_KEY");
+    } finally {
+      await fs.rm(agentDir, { recursive: true, force: true });
+    }
+  });
 });


The fix applies identical logic to both the api_key branch (checking keyRef/key) and the token branch (checking tokenRef/token) in resolveApiKeyFromProfiles. However, the test cases only exercise the api_key path. For a security-relevant change like this, adding parallel test cases for a token-type credential with a non-env tokenRef alongside a populated token field would confirm the token branch is equally protected and lock the behavior against future regressions.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/agents/models-config.providers.normalize-keys.test.ts Line: 77-143 Comment: The fix applies identical logic to both the `api_key` branch (checking `keyRef`/`key`) and the `token` branch (checking `tokenRef`/`token`) in `resolveApiKeyFromProfiles`. However, the test cases only exercise the `api_key` path. For a security-relevant change like this, adding parallel test cases for a `token`-type credential with a non-env `tokenRef` alongside a populated `token` field would confirm the `token` branch is equally protected and lock the behavior against future regressions. 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: b64d2b023e

ℹ️ 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-05T09:32:32Z

src/agents/models-config.providers.ts

+      const keyRef = coerceSecretRef(cred.keyRef);
+      if (keyRef) {
+        if (keyRef.source === "env" && keyRef.id.trim()) {
+          return keyRef.id.trim();
+        }


Use resolved profile key before env SecretRef id

When secrets runtime is active, profiles can legitimately carry both plaintext and ref fields (src/agents/auth-profiles.runtime-snapshot-save.test.ts:45-50 shows key + keyRef together). This change now returns keyRef.id for env refs before checking cred.key, so callers lose the already-resolved secret. In src/agents/models-config.providers.ts:1085-1092, vLLM discovery passes vllmProfileKey directly as a Bearer token; with env refs that use custom var names, this sends the literal env var name instead of the resolved key and breaks model discovery/auth even though the runtime snapshot has the valid key.

Useful? React with 👍 / 👎.

joshavant · 2026-03-07T17:32:49Z

Thanks for the work on this PR and for contributing to this fix path.

Closing this as superseded by #38955 (merged), which integrated the relevant fixes from this thread and completed the broader hardening scope for the secrets/models persistence cluster.

The merged work now covers source-snapshot persistence, marker/provenance auth handling, audit/apply expansion (including models.json/header residues), and write-mode hardening/tests/docs in one end-to-end fix.

Appreciate the contribution here. If there’s a remaining unique change from this PR that should still land, please call it out in a follow-up issue and reference #38955.

fix(models): avoid serializing plaintext keys from non-env SecretRefs

4610b17

openclaw-barnacle bot added agents Agent runtime and tooling size: S labels Mar 4, 2026

greptile-apps bot reviewed Mar 4, 2026

View reviewed changes

Merge branch 'main' into fix-34335-no-plaintext-secretref-leak

b64d2b0

chatgpt-codex-connector bot reviewed Mar 5, 2026

View reviewed changes

This was referenced Mar 7, 2026

Models/Secrets: harden SecretRef-managed models.json persistence and audit #38470

Closed

Secrets: harden SecretRef-safe models.json persistence #38955

Merged

joshavant closed this Mar 7, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(models): avoid non-env SecretRef plaintext persistence#34362

fix(models): avoid non-env SecretRef plaintext persistence#34362
ADKcodeXD wants to merge 2 commits intoopenclaw:mainfrom
ADKcodeXD:fix-34335-no-plaintext-secretref-leak

ADKcodeXD commented Mar 4, 2026

Uh oh!

greptile-apps bot commented Mar 4, 2026

Uh oh!

greptile-apps bot Mar 4, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 5, 2026

Uh oh!

joshavant commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ADKcodeXD commented Mar 4, 2026

Summary

Problem

Fix

Verification

Uh oh!

greptile-apps bot commented Mar 4, 2026

Greptile Summary

Confidence Score: 4/5

Uh oh!

greptile-apps bot Mar 4, 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 5, 2026

Choose a reason for hiding this comment

Uh oh!

joshavant commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants