Greptile Summary

This PR adds a security hardening layer to prevent provider authentication environment variables (such as OPENAI_API_KEY, GITHUB_TOKEN, and HF_TOKEN) from leaking into child ACP and ACPX processes. The fix is well-scoped: stripping is only applied on the default OpenClaw bridge path and the bundled acpx binary, while explicit custom server commands preserve their inherited environment. The three previously-flagged concerns (redundant OPENCLAW_API_KEY guard, allowPluginLocalInstall security coupling, and quoteCommandPart escaping) have all been addressed in earlier commits on this branch.

Key changes:

• src/secrets/provider-env-vars.ts — new EXTRA_PROVIDER_AUTH_ENV_VARS list, listKnownProviderAuthEnvVarNames(), and omitEnvKeysCaseInsensitive() centralise the strip logic with case-insensitive matching.
• extensions/acpx/src/config.ts — explicit stripProviderAuthEnvVars field added to ResolvedAcpxPluginConfig, derived cleanly from command === ACPX_BUNDLED_BIN.
• extensions/acpx/src/runtime.ts — stripProviderAuthEnvVars threaded through all four spawn call sites.
• src/acp/client.ts — shouldStripProviderAuthEnvVarsForAcpServer correctly compares the full (command, args) tuple so an explicit custom command that happens to use the same executable but different args is not stripped.
• One undocumented side effect worth noting: listKnownSecretEnvVarNames now delegates to listKnownProviderAuthEnvVarNames, expanding the set of tokens recognised by secrets audit and secrets apply (see inline comment on provider-env-vars.ts).

Confidence Score: 4/5

• Safe to merge with one minor undocumented behavioral change to note for operators
• The core env-stripping logic is correct, case-insensitive, and well-tested at all four ACPX spawn sites and the ACP client path. The explicit stripProviderAuthEnvVars config field properly decouples the security decision from allowPluginLocalInstall. The shouldStripProviderAuthEnvVarsForAcpServer comparison is reliable for both the entryPath and non-entryPath cases. Score is 4 rather than 5 because listKnownSecretEnvVarNames silently expanding its returned set has an unannounced behavioral effect on secrets audit and secrets apply for tokens like GITHUB_TOKEN and COPILOT_GITHUB_TOKEN — not a correctness bug but potentially surprising for existing users of those features.
• src/secrets/provider-env-vars.ts — the expansion of listKnownSecretEnvVarNames affects secrets audit/apply outside the stated ACP scope and should be explicitly documented for operators.

_{Last reviewed commit: 9c7ed9c}

Follow-up in 44e2192.

Addressed the review points in the minimal patch:

• ACP client stripping now keys off the resolved default-bridge command, so an explicit --server openclaw still strips provider auth env vars.
• Secret handling uses one shared provider-auth env list now, so audit/apply pick up extra tokens like GITHUB_TOKEN, GH_TOKEN, and ANTHROPIC_OAUTH_TOKEN too.
• Env stripping is now case-insensitive in both the ACP client path and the ACPX child-spawn path, which closes the Windows mixed-case bypass.
• ACPX child stripping is now opt-in at the runtime callsites instead of default-on, so hidden/custom callers do not silently lose inherited provider creds.

Verification:

• pnpm exec oxfmt --check src/secrets/provider-env-vars.ts src/secrets/provider-env-vars.test.ts src/plugin-sdk/acpx.ts src/plugin-sdk/subpaths.test.ts src/acp/client.ts src/acp/client.test.ts extensions/acpx/src/runtime-internals/process.ts extensions/acpx/src/runtime-internals/process.test.ts extensions/acpx/src/runtime.ts
• pnpm test -- src/secrets/provider-env-vars.test.ts src/acp/client.test.ts extensions/acpx/src/runtime-internals/process.test.ts extensions/acpx/src/runtime.test.ts

Local note: src/plugin-sdk/subpaths.test.ts still fails in this checkout on the unrelated stale bluebubbles.parseFiniteNumber export mismatch; the ACPX helper assertion itself is not the failing line.

@greptileai review now

@greptileai review now

Follow-up in 9c7ed9c.

Greptile was right about the unrelated Windows quoting regression in extensions/acpx/src/runtime-internals/mcp-agent-command.ts: the replacement string had been over-escaped (\\$& instead of \$&), which would corrupt quoted proxy paths.

Fixed:

• restored the original escaping in quoteCommandPart()
• added a focused regression test in extensions/acpx/src/runtime-internals/mcp-agent-command.test.ts

Verification:

• pnpm exec oxfmt --check extensions/acpx/src/runtime-internals/mcp-agent-command.ts extensions/acpx/src/runtime-internals/mcp-agent-command.test.ts
• pnpm test -- extensions/acpx/src/runtime-internals/mcp-agent-command.test.ts

@greptileai review now

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Brittle command string equality can disable provider auth env var stripping for the bundled acpx binary

Description

stripProviderAuthEnvVars is derived solely from a string equality check against ACPX_BUNDLED_BIN:

• resolveConfiguredCommand() can resolve rawConfig.command to many semantically equivalent paths (different casing on case-insensitive filesystems, symlinks, alternate absolute paths) that still execute the same bundled binary.
• If command !== ACPX_BUNDLED_BIN (string compare), stripProviderAuthEnvVars becomes false.
• When stripProviderAuthEnvVars is false, spawned acpx / npm child processes inherit provider authentication environment variables (API keys/tokens) because spawnWithResolvedCommand() only strips when this flag is set.

This makes the stripping policy easy to accidentally (or intentionally) bypass while still running the bundled binary, resulting in unexpected exposure of provider credentials to child processes.

Vulnerable code:

const command = resolveConfiguredCommand({ configured: normalized.command, workspaceDir: params.workspaceDir });
const stripProviderAuthEnvVars = command === ACPX_BUNDLED_BIN;

Recommendation

Make the decision based on the canonical resolved executable rather than raw string equality, and/or allow an explicit config override with a secure default.

Option A (canonicalize paths):

import fs from "node:fs";

function sameExecutable(a: string, b: string): boolean {
  try {
    return fs.realpathSync(a) === fs.realpathSync(b);
  } catch {
    return false;
  }
}

const isBundled = sameExecutable(command, ACPX_BUNDLED_BIN);
const stripProviderAuthEnvVars = isBundled;

Option B (explicit config): add stripProviderAuthEnvVars?: boolean to the config schema, default it to true (or at minimum true for bundled), and document the security implications of disabling it.

Also consider normalizing case on Windows/macOS case-insensitive filesystems if relying on string comparison anywhere else.

Analyzed PR: #42250 at commit 0f67c17

_{Last updated on: 2026-03-10T21:59:29Z}