🔒 Aisle Security Analysis

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

1. 🔵 Incomplete secret env var enumeration leaves new provider auth tokens unsanitized in .env audit/scrub flows

Description

src/secrets/provider-env-vars.ts introduces EXTRA_PROVIDER_AUTH_ENV_VARS and listKnownProviderAuthEnvVarNames(), but does not update listKnownSecretEnvVarNames() to include these additional secret-bearing environment variables.

Impact (grounded call sites):

• src/secrets/audit.ts uses listKnownSecretEnvVarNames() to detect plaintext secrets in a user's .env file. New auth env vars like GITHUB_TOKEN, GH_TOKEN, COPILOT_GITHUB_TOKEN, ANTHROPIC_OAUTH_TOKEN, etc. are not flagged, reducing the effectiveness of secrets auditing.
• src/secrets/apply.ts uses listKnownSecretEnvVarNames() as the allowlist for scrubEnvRaw(...) when attempting to remove migrated secret values from the .env file. Lines for any env var present only in EXTRA_PROVIDER_AUTH_ENV_VARS will be skipped and retained, leaving credentials on disk even after a migration/apply flow intended to scrub them.

Vulnerable code (root cause):

export function listKnownSecretEnvVarNames(): string[] {
  return [...new Set(Object.values(PROVIDER_ENV_VARS).flatMap((keys) => keys))];
}

This divergence between “provider auth env vars” and “secret env vars” is a concrete gap introduced by the new enumeration: some provider auth secrets are now recognized for spawn-time stripping, but still omitted from secret audit/scrub mechanisms.

Recommendation

Unify the two lists so all known provider auth secrets are treated as “known secret env vars” for auditing/scrubbing.

Option A (simple): make listKnownSecretEnvVarNames() delegate to the new function:

export function listKnownSecretEnvVarNames(): string[] {
  return listKnownProviderAuthEnvVarNames();
}

Option B (explicit): include EXTRA_PROVIDER_AUTH_ENV_VARS in listKnownSecretEnvVarNames().

Also update any secret-audit/scrub call sites (src/secrets/audit.ts, src/secrets/apply.ts) to use the unified function, ensuring .env detection/removal covers GITHUB_TOKEN, GH_TOKEN, COPILOT_GITHUB_TOKEN, ANTHROPIC_OAUTH_TOKEN, and other extras.

2. 🔵 Provider API keys not stripped when default ACP server command is provided explicitly

Description

The new env-stripping logic decides whether to remove provider auth environment variables based solely on whether serverCommand is present (truthy), not whether the spawned server is the trusted/default OpenClaw bridge.

As a result:

• If a caller explicitly sets serverCommand to the default bridge command (e.g., "openclaw"), shouldStripProviderAuthEnvVarsForAcpServer() returns false.
• buildAcpClientStripKeys() will therefore not add known provider auth env vars (e.g., OPENAI_API_KEY, GITHUB_TOKEN, etc.) to stripKeys.
• The spawned child process will inherit these secrets via env: spawnEnv.

This can lead to unexpected secret exposure, especially because "openclaw" is resolved via PATH and could be a different binary than intended (e.g., PATH hijacking), defeating the goal of default-bridge env stripping.

Vulnerable code:

export function shouldStripProviderAuthEnvVarsForAcpServer(serverCommand?: string): boolean {
  return !serverCommand;
}

​// ...
const stripKeys = buildAcpClientStripKeys({
  serverCommand: opts.serverCommand,
  activeSkillEnvKeys: getActiveSkillEnvKeys(),
});

Recommendation

Treat the default OpenClaw bridge as the boundary to strip provider auth env vars, regardless of whether the caller specified the default command explicitly.

Suggested approach:

1. Normalize/trim serverCommand.
2. Consider openclaw (and the self-hosted Node entrypoint path when applicable) as default bridge and strip provider auth env vars for those values.
3. If you still need to allow opting into passing provider auth env vars to custom servers, introduce an explicit option (opt-in) rather than inferring trust from presence.

Example fix (conceptual):

function isDefaultBridgeCommand(resolvedServerCommand: string, entryPath: string | null): boolean {
  const cmd = resolvedServerCommand.trim();
  if (!cmd) return true;
  if (cmd === "openclaw") return true;
  ​// When using the bundled entrypoint, default bridge uses node + entry.js
  if (entryPath && cmd === process.execPath) return true;
  return false;
}

​// in createAcpClient
const isDefaultBridge = isDefaultBridgeCommand(serverCommand, entryPath);
const stripKeys = buildAcpClientStripKeys({
  ​// pass a boolean instead of raw user input, or pass the resolved command
  serverCommand: isDefaultBridge ? undefined : serverCommand,
  activeSkillEnvKeys: getActiveSkillEnvKeys(),
});

This prevents accidental leaks when callers pass serverCommand: "openclaw" and makes the trust boundary explicit.

Analyzed PR: #41615 at commit 9131150

_{Last updated on: 2026-03-10T15:46:53Z}

Greptile Summary

This PR strips provider API key env vars from ACP child process environments on both CLI and gateway/acpx spawn paths, which correctly addresses the root cause of tools like Codex CLI switching auth modes when inheriting parent process credentials. The core env-var stripping logic in client.ts and process.ts is correct and well-tested.

However, the PR introduces two significant data-loss regressions in extensions/acpx/src/runtime.ts:

• resumeSessionId silently dropped: Session resumption support added in feat(acp): add resumeSessionId to sessions_spawn for ACP session resume #41847 is removed without documentation, so any caller attempting to resume a session will create a new one instead.
• Image attachments silently dropped: Attachment forwarding added in acp: forward attachments into ACP runtime sessions #41427 is removed, causing image content to be discarded.

Additionally, the env-var stripping intent is obscured by reusing allowPluginLocalInstall (a flag about binary selection) as the proxy for stripping behavior, creating implicit semantic coupling that could be broken by future changes.

Not safe to merge as-is without either restoring these features or removing the now-unused input fields from the API contracts.

Confidence Score: 2/5

• Not safe to merge — the PR silently drops session resumption and image attachment support, creating data-loss regressions for live callers.
• The core API key stripping logic is correct and well-tested. However, two intentional features (resumeSessionId from feat(acp): add resumeSessionId to sessions_spawn for ACP session resume #41847 and attachments from acp: forward attachments into ACP runtime sessions #41427) are silently removed without documentation or cleanup of their input fields. This creates subtle data-loss bugs: session resumption attempts will silently create new sessions, and image content will silently be discarded. Additionally, the env-var stripping uses a semantically unclear proxy flag.
• extensions/acpx/src/runtime.ts — must resolve resumeSessionId and attachments regressions, and clarify the env-var stripping intent.

_{Last reviewed commit: 992a770}

Addressed the remaining bot comments in d24f695.

What changed:

• The acpx runtime now uses a shared listKnownProviderAuthEnvVarNames() helper, so it strips the same non-_API_KEY auth vars as the CLI ACP path, including token-style env vars such as GITHUB_TOKEN and HF_TOKEN.
• The CLI ACP path now only strips provider auth env vars for the default OpenClaw bridge path. Explicit custom ACP servers passed via --server keep their env-based auth contract.
• Added focused coverage in src/acp/client.test.ts, extensions/acpx/src/runtime-internals/process.test.ts, and src/plugin-sdk/subpaths.test.ts.

Verification:

• pnpm test -- src/acp/client.test.ts extensions/acpx/src/runtime-internals/process.test.ts src/plugin-sdk/subpaths.test.ts
• pnpm build

@greptileai please review

Closing this in favor of #42250.

That replacement PR rewrites the fix as a minimal patch on top of current main, keeping only the original provider-auth env stripping changes and their targeted tests.