diff --git a/CHANGELOG.md b/CHANGELOG.md index d40af7e6ccd..288d17f14a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai - Android/security: stop `ASK_OPENCLAW` intents from auto-sending injected prompts, so external app actions only prefill the draft instead of dispatching it immediately. (#70714) Thanks @vincentkoc. - Control UI/chat: queue Stop-button aborts across Gateway reconnects so a disconnected active run is canceled on reconnect instead of only clearing local UI state. (#70673) Thanks @chinar-amrutkar. +- Secrets/Windows: strip UTF-8 BOMs from file-backed secrets and keep unavailable ACL checks fail-closed unless trusted file or exec providers explicitly opt into `allowInsecurePath`. (#70662) Thanks @zhanggpcsu. - QQBot/security: require framework auth for `/bot-approve` so unauthorized QQ senders cannot change exec approval settings through the unauthenticated pre-dispatch slash-command path. (#70706) Thanks @vincentkoc. - MCP/tools: stop the ACPX OpenClaw tools bridge from listing or invoking owner-only tools such as `cron`, closing a privilege-escalation path for non-owner MCP callers. (#70698) Thanks @vincentkoc. - Feishu/onboarding: load Feishu setup surfaces through a setup-only barrel so first-run setup no longer imports Feishu's Lark SDK before bundled runtime deps are staged. (#70339) Thanks @andrejtr. diff --git a/docs/cli/config.md b/docs/cli/config.md index 15e5923a84e..fd7356b40a8 100644 --- a/docs/cli/config.md +++ b/docs/cli/config.md @@ -203,6 +203,7 @@ File provider (`--provider-source file`): - `--provider-path ` (required) - `--provider-mode ` - `--provider-max-bytes ` +- `--provider-allow-insecure-path` Exec provider (`--provider-source exec`): diff --git a/docs/gateway/configuration-reference.md b/docs/gateway/configuration-reference.md index 0890d6e50c9..f006e0d24b8 100644 --- a/docs/gateway/configuration-reference.md +++ b/docs/gateway/configuration-reference.md @@ -3473,6 +3473,7 @@ Validation: Notes: - `file` provider supports `mode: "json"` and `mode: "singleValue"` (`id` must be `"value"` in singleValue mode). +- File and exec provider paths fail closed when Windows ACL verification is unavailable. Set `allowInsecurePath: true` only for trusted paths that cannot be verified. - `exec` provider requires an absolute `command` path and uses protocol payloads on stdin/stdout. - By default, symlink command paths are rejected. Set `allowSymlinkCommand: true` to allow symlink paths while validating the resolved target path. - If `trustedDirs` is configured, the trusted-dir check applies to the resolved target path. diff --git a/src/cli/config-cli.test.ts b/src/cli/config-cli.test.ts index cfee81328a9..abbb49aeb3b 100644 --- a/src/cli/config-cli.test.ts +++ b/src/cli/config-cli.test.ts @@ -885,6 +885,7 @@ describe("config cli", () => { "/tmp/vault.json", "--provider-mode", "json", + "--provider-allow-insecure-path", ]); expect(mockWriteConfigFile).toHaveBeenCalledTimes(1); @@ -893,6 +894,7 @@ describe("config cli", () => { source: "file", path: "/tmp/vault.json", mode: "json", + allowInsecurePath: true, }); }); diff --git a/src/cli/config-cli.ts b/src/cli/config-cli.ts index 007fab65c97..62d68a28d54 100644 --- a/src/cli/config-cli.ts +++ b/src/cli/config-cli.ts @@ -712,6 +712,7 @@ function buildProviderFromBuilder(opts: ConfigSetOptions): SecretProviderConfig ...(mode ? { mode } : {}), ...(timeoutMs !== undefined ? { timeoutMs } : {}), ...(maxBytes !== undefined ? { maxBytes } : {}), + ...(opts.providerAllowInsecurePath ? { allowInsecurePath: true } : {}), }; } else { const command = opts.providerCommand?.trim(); @@ -1599,7 +1600,7 @@ export function registerConfigCli(program: Command) { ) .option( "--provider-allow-insecure-path", - "Provider builder (exec): bypass strict path permission checks", + "Provider builder (file|exec): bypass strict path permission checks", false, ) .option( diff --git a/src/config/config.secrets-schema.test.ts b/src/config/config.secrets-schema.test.ts index 5703af844fb..a9ea0eb2bc4 100644 --- a/src/config/config.secrets-schema.test.ts +++ b/src/config/config.secrets-schema.test.ts @@ -30,6 +30,7 @@ describe("config secret refs schema", () => { path: "~/.openclaw/secrets.json", mode: "json", timeoutMs: 10_000, + allowInsecurePath: true, }, vault: { source: "exec", diff --git a/src/config/schema.base.generated.ts b/src/config/schema.base.generated.ts index 0be269987b9..8e32bf31bc2 100644 --- a/src/config/schema.base.generated.ts +++ b/src/config/schema.base.generated.ts @@ -824,6 +824,9 @@ export const GENERATED_BASE_CONFIG_SCHEMA: BaseConfigSchemaResponse = { exclusiveMinimum: 0, maximum: 20971520, }, + allowInsecurePath: { + type: "boolean", + }, }, required: ["source", "path"], additionalProperties: false, diff --git a/src/config/types.secrets.ts b/src/config/types.secrets.ts index bbf46a97061..00d9dec700a 100644 --- a/src/config/types.secrets.ts +++ b/src/config/types.secrets.ts @@ -233,6 +233,7 @@ export type FileSecretProviderConfig = { mode?: FileSecretProviderMode; timeoutMs?: number; maxBytes?: number; + allowInsecurePath?: boolean; }; export type ExecSecretProviderConfig = { diff --git a/src/config/zod-schema.core.ts b/src/config/zod-schema.core.ts index 3d61f244bef..41b2e499b5d 100644 --- a/src/config/zod-schema.core.ts +++ b/src/config/zod-schema.core.ts @@ -102,6 +102,7 @@ const SecretsFileProviderSchema = z .positive() .max(20 * 1024 * 1024) .optional(), + allowInsecurePath: z.boolean().optional(), }) .strict(); diff --git a/src/secrets/configure.ts b/src/secrets/configure.ts index 8d04efd7217..27f640e0ea7 100644 --- a/src/secrets/configure.ts +++ b/src/secrets/configure.ts @@ -446,6 +446,13 @@ async function promptFileProvider( initialValue: base?.maxBytes, max: 20 * 1024 * 1024, }); + const allowInsecurePath = assertNoCancel( + await confirm({ + message: "Allow insecure file path checks?", + initialValue: base?.allowInsecurePath ?? false, + }), + "Secrets configure cancelled.", + ); return { source: "file", @@ -453,6 +460,7 @@ async function promptFileProvider( mode, ...(timeoutMs ? { timeoutMs } : {}), ...(maxBytes ? { maxBytes } : {}), + ...(allowInsecurePath ? { allowInsecurePath: true } : {}), }; } diff --git a/src/secrets/resolve.test.ts b/src/secrets/resolve.test.ts index 7f906d00919..9debb9f4ef6 100644 --- a/src/secrets/resolve.test.ts +++ b/src/secrets/resolve.test.ts @@ -54,6 +54,7 @@ describe("secret ref resolver", () => { path: string; mode: "json" | "singleValue"; timeoutMs?: number; + allowInsecurePath?: boolean; }; function createExecProviderConfig( @@ -454,4 +455,112 @@ describe("secret ref resolver", () => { ).rejects.toThrow(/Exec secret reference id must match|Secret reference id is empty/); } }); + + it("strips UTF-8 BOM from file provider payload before JSON parse", async () => { + const dir = await createCaseDir("bom-file"); + const filePath = path.join(dir, "secrets-with-bom.json"); + // Write JSON with UTF-8 BOM prefix (EF BB BF) + const bom = "\uFEFF"; + await writeSecureFile(filePath, `${bom}{"apiKey":"sk-test-123"}`); + + const value = await resolveSecretRefString( + { source: "file", provider: "filemain", id: "/apiKey" }, + { + config: { + secrets: { + providers: { + filemain: createFileProviderConfig(filePath), + }, + }, + }, + }, + ); + expect(value).toBe("sk-test-123"); + }); + + it("strips UTF-8 BOM from file provider singleValue mode", async () => { + const dir = await createCaseDir("bom-single"); + const filePath = path.join(dir, "secret-with-bom.txt"); + const bom = "\uFEFF"; + await writeSecureFile(filePath, `${bom}my-secret-value\n`); + + const value = await resolveSecretRefString( + { source: "file", provider: "filemain", id: "value" }, + { + config: { + secrets: { + providers: { + filemain: createFileProviderConfig(filePath, { mode: "singleValue" }), + }, + }, + }, + }, + ); + expect(value).toBe("my-secret-value"); + }); + + it("fails closed on Windows when file provider ACL source is unknown", async () => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32" as unknown as NodeJS.Platform); + + try { + const dir = await createCaseDir("win-acl"); + const filePath = path.join(dir, "secrets.json"); + await writeSecureFile(filePath, '{"token":"abc123"}'); + + await expect( + resolveSecretRefString( + { source: "file", provider: "filemain", id: "/token" }, + { + config: { + secrets: { + providers: { + filemain: createFileProviderConfig(filePath), + }, + }, + }, + }, + ), + ).rejects.toThrow(/ACL verification unavailable on Windows/); + } finally { + vi.restoreAllMocks(); + } + }); + + it("allows trusted file provider opt-out when Windows ACL source is unknown", async () => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32" as unknown as NodeJS.Platform); + + try { + const dir = await createCaseDir("win-acl-opt-out"); + const filePath = path.join(dir, "secrets.json"); + await writeSecureFile(filePath, '{"token":"abc123"}'); + + const value = await resolveSecretRefString( + { source: "file", provider: "filemain", id: "/token" }, + { + config: { + secrets: { + providers: { + filemain: createFileProviderConfig(filePath, { allowInsecurePath: true }), + }, + }, + }, + }, + ); + expect(value).toBe("abc123"); + } finally { + vi.restoreAllMocks(); + } + }); + + it("fails closed on Windows when exec provider ACL source is unknown", async () => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32" as unknown as NodeJS.Platform); + + try { + await expect(resolveExecSecret(execProtocolV1ScriptPath)).rejects.toThrow( + /ACL verification unavailable on Windows/, + ); + } finally { + vi.restoreAllMocks(); + } + }); }); diff --git a/src/secrets/resolve.ts b/src/secrets/resolve.ts index fe2e2eeb1e5..02efffb168e 100644 --- a/src/secrets/resolve.ts +++ b/src/secrets/resolve.ts @@ -286,6 +286,7 @@ async function readFileProviderPayload(params: { const secureFilePath = await assertSecurePath({ targetPath: filePath, label: `secrets.providers.${params.providerName}.path`, + allowInsecurePath: params.providerConfig.allowInsecurePath, }); const timeoutMs = normalizePositiveInt( params.providerConfig.timeoutMs, @@ -309,7 +310,7 @@ async function readFileProviderPayload(params: { if (payload.byteLength > maxBytes) { throw new Error(`File provider "${params.providerName}" exceeded maxBytes (${maxBytes}).`); } - const text = payload.toString("utf8"); + const text = payload.toString("utf8").replace(/^\uFEFF/, ""); if (params.providerConfig.mode === "singleValue") { return text.replace(/\r?\n$/, ""); } diff --git a/src/security/windows-acl.test.ts b/src/security/windows-acl.test.ts index 01d104a4568..3da006d578a 100644 --- a/src/security/windows-acl.test.ts +++ b/src/security/windows-acl.test.ts @@ -440,7 +440,7 @@ Successfully processed 1 files`; expectInspectSuccess(result, 2); // /sid is passed so that account names are printed as SIDs, making the // audit locale-independent (fixes #35834). - expect(mockExec).toHaveBeenCalledWith("icacls", ["C:\\test\\file.txt", "/sid"]); + expect(mockExec).toHaveBeenCalledWith("icacls.exe", ["C:\\test\\file.txt", "/sid"]); }); it("classifies *S-1-5-18 (SID form of SYSTEM from /sid) as trusted", async () => { @@ -485,8 +485,8 @@ Successfully processed 1 files`; expectInspectSuccess(result, 2); expect(result.trusted).toHaveLength(2); expect(result.untrustedGroup).toHaveLength(0); - expect(mockExec).toHaveBeenNthCalledWith(1, "icacls", ["C:\\test\\file.txt", "/sid"]); - expect(mockExec).toHaveBeenNthCalledWith(2, "whoami", ["/user", "/fo", "csv", "/nh"]); + expect(mockExec).toHaveBeenNthCalledWith(1, "icacls.exe", ["C:\\test\\file.txt", "/sid"]); + expect(mockExec).toHaveBeenNthCalledWith(2, "whoami.exe", ["/user", "/fo", "csv", "/nh"]); }); it("returns error state on exec failure", async () => { @@ -534,6 +534,36 @@ Successfully processed 1 files`; expect(result.untrustedGroup).toHaveLength(1); expect(mockExec).toHaveBeenCalledTimes(2); }); + + it("uses SystemRoot for Windows system commands when available", async () => { + const mockExec = vi + .fn() + .mockResolvedValueOnce({ + stdout: "C:\\test\\file.txt *S-1-5-21-111-222-333-1001:(F)", + stderr: "", + }) + .mockResolvedValueOnce({ + stdout: '"mock-host\\\\MockUser","S-1-5-21-111-222-333-1001"\r\n', + stderr: "", + }); + + const result = await inspectWindowsAcl("C:\\test\\file.txt", { + exec: mockExec, + env: { SystemRoot: "C:\\Windows" }, + }); + + expectInspectSuccess(result, 1); + expect(mockExec).toHaveBeenNthCalledWith(1, "C:\\Windows\\System32\\icacls.exe", [ + "C:\\test\\file.txt", + "/sid", + ]); + expect(mockExec).toHaveBeenNthCalledWith(2, "C:\\Windows\\System32\\whoami.exe", [ + "/user", + "/fo", + "csv", + "/nh", + ]); + }); }); describe("formatWindowsAclSummary", () => { diff --git a/src/security/windows-acl.ts b/src/security/windows-acl.ts index 9f269df4e99..bac804feaf3 100644 --- a/src/security/windows-acl.ts +++ b/src/security/windows-acl.ts @@ -1,4 +1,5 @@ import os from "node:os"; +import path from "node:path"; import { runExec } from "../process/exec.js"; import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js"; @@ -100,6 +101,15 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set { return trusted; } +function resolveWindowsSystemCommand(command: string, env?: NodeJS.ProcessEnv): string { + const root = + env?.SystemRoot?.trim() || + env?.SYSTEMROOT?.trim() || + env?.windir?.trim() || + env?.WINDIR?.trim(); + return root ? path.win32.join(root, "System32", command) : command; +} + function classifyPrincipal( principal: string, trustedPrincipals: Set, @@ -270,9 +280,17 @@ export function summarizeWindowsAcl( return { trusted, untrustedWorld, untrustedGroup }; } -async function resolveCurrentUserSid(exec: ExecFn): Promise { +async function resolveCurrentUserSid( + exec: ExecFn, + env?: NodeJS.ProcessEnv, +): Promise { try { - const { stdout, stderr } = await exec("whoami", ["/user", "/fo", "csv", "/nh"]); + const { stdout, stderr } = await exec(resolveWindowsSystemCommand("whoami.exe", env), [ + "/user", + "/fo", + "csv", + "/nh", + ]); const match = `${stdout}\n${stderr}`.match(/\*?S-\d+-\d+(?:-\d+)+/i); return match ? normalizeSid(match[0]) : null; } catch (err) { @@ -297,7 +315,10 @@ export async function inspectWindowsAcl( // Windows (Russian, Chinese, etc.) where icacls prints Cyrillic / CJK // characters that may be garbled when Node reads them in the wrong code // page. Fixes #35834. - const { stdout, stderr } = await exec("icacls", [targetPath, "/sid"]); + const { stdout, stderr } = await exec(resolveWindowsSystemCommand("icacls.exe", opts?.env), [ + targetPath, + "/sid", + ]); const output = `${stdout}\n${stderr}`.trim(); const entries = parseIcaclsOutput(output, targetPath); let effectiveEnv = opts?.env; @@ -307,7 +328,7 @@ export async function inspectWindowsAcl( !effectiveEnv?.USERSID && untrustedGroup.some((entry) => SID_RE.test(normalize(entry.principal))); if (needsUserSidResolution) { - const currentUserSid = await resolveCurrentUserSid(exec); + const currentUserSid = await resolveCurrentUserSid(exec, effectiveEnv); if (currentUserSid) { effectiveEnv = { ...effectiveEnv, USERSID: currentUserSid }; ({ trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, effectiveEnv));