From b61556df06e5561b5e93fb879fa3beaebdeaeffb Mon Sep 17 00:00:00 2001 From: koshaji Date: Fri, 1 May 2026 00:00:47 +1000 Subject: [PATCH] fix(config-audit): expand secret-flag detection + cap argv length Address PR #75095 review: - Add a suffix-based heuristic so any `--<...>-(token|secret|password| passwd|api[-_]?key|api[-_]?secret|webhook|credential|bearer|pat)` is treated as a secret flag in addition to the explicit list. This catches `--custom-api-key`, `--alibaba-model-studio-api-key`, plugin-defined `cliFlag` values, and similar flags that ship in OpenClaw + bundled plugins today and were previously falling through to the per-element token-shape fallback (which can miss arbitrary provider keys with no recognizable prefix). - Expand the explicit list with adjacent secret flags surfaced by Copilot (`--app-token`, `--remote-token`, `--push-token`, `--bearer-token`, `--id-token`, `--identity-token`, `--session-token`, `--service-token`, `--pat`, `--personal-access-token`, `--oauth-token`, `--webhook-token`). - Stop masking the next arg when a secret flag is followed by another option (`--token --port 8080` should not mask `--port`). Treat that as a missing value and keep the following arg intact. - Cap caller-supplied processInfo argv/execArgv at 8 entries via a new capArgv helper so a future caller can't bypass the snapshot length bound and persist arbitrarily large argv to the audit log. 4 new tests cover: heuristic flag detection (`--custom-api-key`, `--alibaba-model-studio-api-key`, `--app-token`, `--frobnicate-credential`), no-mask when secret flag is followed by another option, no-mask when secret flag is the final arg without a value, and the slice(0, 8) cap with a long argv that smuggles a sensitive value past index 8 into a confirmed-not-present assertion. --- src/config/io.audit.test.ts | 83 ++++++++++++++++++++++++++++++++ src/config/io.audit.ts | 95 +++++++++++++++++++++++++++++-------- 2 files changed, 157 insertions(+), 21 deletions(-) diff --git a/src/config/io.audit.test.ts b/src/config/io.audit.test.ts index 5290d39f335..fba2a66cd8d 100644 --- a/src/config/io.audit.test.ts +++ b/src/config/io.audit.test.ts @@ -250,6 +250,89 @@ describe("config io audit helpers", () => { expect(redactConfigAuditArgv(argv)).toEqual(argv); }); + it("redacts unknown but credential-suffixed flags via the heuristic classifier", () => { + const argv = [ + "node", + "openclaw", + "--custom-api-key", + "real-tenant-key-AB12CD34EF56GH78", + "--alibaba-model-studio-api-key=plain-value-xyz-12345", + "--app-token", + "another-secret-value", + "--frobnicate-credential=hidden", + ]; + const result = redactConfigAuditArgv(argv); + expect(result).toEqual([ + "node", + "openclaw", + "--custom-api-key", + "***", + "--alibaba-model-studio-api-key=***", + "--app-token", + "***", + "--frobnicate-credential=***", + ]); + }); + + it("does not mask the next arg when a secret flag is followed by another option", () => { + const argv = ["openclaw", "--token", "--port", "8080"]; + expect(redactConfigAuditArgv(argv)).toEqual(["openclaw", "--token", "--port", "8080"]); + }); + + it("does not mask when a secret flag is the final arg with no value", () => { + const argv = ["openclaw", "--token"]; + expect(redactConfigAuditArgv(argv)).toEqual(["openclaw", "--token"]); + }); + + it("caps caller-supplied processInfo argv at 8 entries before redaction", () => { + const longArgv = [ + "node", + "openclaw", + "--api-key", + "secret", + "--port", + "8080", + "--bind", + "lan", + "--leaks-here-token", + "this-must-not-land-in-audit-1234567890", + ]; + const base = createConfigWriteAuditRecordBase({ + configPath: "/tmp/openclaw.json", + env: {} as NodeJS.ProcessEnv, + existsBefore: true, + previousHash: "prev", + nextHash: "next", + previousBytes: 1, + nextBytes: 2, + previousMetadata: { + dev: null, + ino: null, + mode: null, + nlink: null, + uid: null, + gid: null, + }, + changedPathCount: 0, + hasMetaBefore: true, + hasMetaAfter: true, + gatewayModeBefore: "local", + gatewayModeAfter: "local", + suspicious: [], + now: "2026-04-30T00:00:00.000Z", + processInfo: { + pid: 1, + ppid: 1, + cwd: "/work", + argv: longArgv, + execArgv: [], + }, + }); + expect(base.argv).toHaveLength(8); + expect(base.argv).not.toContain("this-must-not-land-in-audit-1234567890"); + expect(base.argv).not.toContain("--leaks-here-token"); + }); + it("redacts processInfo.argv when explicitly supplied to createConfigWriteAuditRecordBase", () => { const base = createConfigWriteAuditRecordBase({ configPath: "/tmp/openclaw.json", diff --git a/src/config/io.audit.ts b/src/config/io.audit.ts index f7db6c79f46..23c9a3646fd 100644 --- a/src/config/io.audit.ts +++ b/src/config/io.audit.ts @@ -2,6 +2,12 @@ import path from "node:path"; import { redactToolPayloadText } from "../logging/redact.js"; import { resolveStateDir } from "./paths.js"; +const CONFIG_AUDIT_ARGV_CAP = 8; + +// Conservative list of credential-bearing flags. The heuristic suffix +// classifier below catches the long tail (`--custom-api-key`, +// `--alibaba-model-studio-api-key`, plugin-defined `cliFlag` values, etc.) +// without needing every name enumerated here. const SECRET_FLAG_NAMES = new Set([ "--token", "--api-key", @@ -16,11 +22,41 @@ const SECRET_FLAG_NAMES = new Set([ "--hook-token", "--gateway-token", "--bot-token", + "--app-token", + "--remote-token", + "--push-token", "--webhook-secret", + "--webhook-token", "--service-account-token", "--op-service-account-token", + "--bearer", + "--bearer-token", + "--pat", + "--personal-access-token", + "--oauth-token", + "--id-token", + "--identity-token", + "--session-token", + "--service-token", ]); +// Suffix-based heuristic. Any `--…-(token|secret|password|passwd|api-key| +// apikey|api-secret|webhook|credential|bearer|pat)` is treated as a secret +// flag in addition to the explicit list. The leading `--` is required so we +// don't mismatch arbitrary positional arguments. +const SECRET_FLAG_SUFFIX_PATTERN = + /^--(?:[a-z0-9]+(?:-[a-z0-9]+)*-)?(?:token|secret|password|passwd|api[-_]?key|api[-_]?secret|webhook|credential|bearer|pat)$/; + +function isSecretFlagName(flagName: string | null): boolean { + if (flagName === null) { + return false; + } + if (SECRET_FLAG_NAMES.has(flagName)) { + return true; + } + return SECRET_FLAG_SUFFIX_PATTERN.test(flagName); +} + function parseFlagName(arg: string): string | null { if (typeof arg !== "string" || !arg.startsWith("--")) { return null; @@ -30,36 +66,46 @@ function parseFlagName(arg: string): string | null { } // Redacts CLI argv before it lands in the persistent config-audit log. -// Three layers, applied per element: -// 1. `--flag=value` form for known secret flag names — mask the value half. -// 2. value following a bare `--flag` form — emit `***` instead of the next arg. +// Layers, applied per element: +// 1. `--flag=value` form for any name matching the explicit list or the +// suffix heuristic — mask the value half. +// 2. value following a bare `--flag` form — emit `***` instead of the +// next arg, but only when the candidate value does not itself look +// like another option (`-`/`--` prefix). A bare `--token --port 8080` +// is treated as a missing value, leaving `--port` and `8080` intact. // 3. fall back to redactToolPayloadText for everything else, which catches // `KEY=VALUE` env-style assignments, raw token shapes (sk-, ghp_, xox*, // gsk_, AIza*, npm_, Telegram bot tokens, PEM blocks, Bearer headers, // URL query secrets) using the shared redaction patterns. export function redactConfigAuditArgv(argv: readonly string[]): string[] { const result: string[] = []; + let redactNext = false; for (let i = 0; i < argv.length; i++) { const current = argv[i]; if (typeof current !== "string") { result.push(current); + redactNext = false; continue; } + if (redactNext) { + redactNext = false; + if (!current.startsWith("-")) { + result.push("***"); + continue; + } + // The previous arg looked like a secret flag but was followed by + // another option — treat as a missing value and fall through so the + // current arg is processed normally. + } const currentFlag = parseFlagName(current); - if (currentFlag !== null && SECRET_FLAG_NAMES.has(currentFlag) && current.includes("=")) { - const eq = current.indexOf("="); - result.push(`${current.slice(0, eq + 1)}***`); - continue; - } - const previous = i > 0 ? argv[i - 1] : undefined; - const previousFlag = typeof previous === "string" ? parseFlagName(previous) : null; - if ( - previousFlag !== null && - SECRET_FLAG_NAMES.has(previousFlag) && - typeof previous === "string" && - !previous.includes("=") - ) { - result.push("***"); + if (currentFlag !== null && isSecretFlagName(currentFlag)) { + if (current.includes("=")) { + const eq = current.indexOf("="); + result.push(`${current.slice(0, eq + 1)}***`); + continue; + } + result.push(current); + redactNext = true; continue; } result.push(redactToolPayloadText(current)); @@ -67,13 +113,20 @@ export function redactConfigAuditArgv(argv: readonly string[]): string[] { return result; } +function capArgv(argv: readonly string[] | undefined): string[] { + if (!Array.isArray(argv)) { + return []; + } + return argv.slice(0, CONFIG_AUDIT_ARGV_CAP); +} + export function snapshotConfigAuditProcessInfo(): ConfigAuditProcessInfo { return { pid: process.pid, ppid: process.ppid, cwd: process.cwd(), - argv: redactConfigAuditArgv(process.argv.slice(0, 8)), - execArgv: redactConfigAuditArgv(process.execArgv.slice(0, 8)), + argv: redactConfigAuditArgv(capArgv(process.argv)), + execArgv: redactConfigAuditArgv(capArgv(process.execArgv)), }; } @@ -241,8 +294,8 @@ function resolveConfigAuditProcessInfo( if (processInfo) { return { ...processInfo, - argv: redactConfigAuditArgv(processInfo.argv), - execArgv: redactConfigAuditArgv(processInfo.execArgv), + argv: redactConfigAuditArgv(capArgv(processInfo.argv)), + execArgv: redactConfigAuditArgv(capArgv(processInfo.execArgv)), }; } return snapshotConfigAuditProcessInfo();