fix(cli): oversized secrets plans consume unbounded memory (#109652)

* fix(cli): reject oversized secrets plan files

* fix(cli): keep generated secrets plans applyable

* fix(cli): reject special plan files without blocking

* docs(cli): refresh secrets plan documentation map
This commit is contained in:
xingzhou
2026-07-18 14:18:21 +08:00
committed by GitHub
parent ec3eb573bf
commit 75982be9a3
5 changed files with 187 additions and 8 deletions
+3
View File
@@ -110,6 +110,7 @@ Notes:
- Supports creating new `auth-profiles.json` mappings directly in the picker flow.
- Runs preflight resolution before apply.
- Generated plans default to scrub options enabled (`scrubEnv`, `scrubAuthProfilesForProviderTargets`, `scrubLegacyAuthJson`). Apply is one-way for scrubbed plaintext values.
- `--plan-out` refuses to create a plan whose UTF-8 serialized form exceeds 16 MiB (16,777,216 bytes), matching the `apply --from` input limit.
- Without `--apply`, the CLI still prompts `Apply this plan now?` after preflight.
- With `--apply` (and no `--yes`), the CLI prompts an extra irreversible-migration confirmation.
- `--json` prints the plan + preflight report, but still requires an interactive TTY.
@@ -130,6 +131,8 @@ openclaw secrets apply --from /tmp/openclaw-secrets-plan.json --json
`--dry-run` validates preflight without writing files; exec SecretRef checks are skipped by default in dry-run. Write mode rejects plans containing exec SecretRefs/providers unless `--allow-exec`. Use `--allow-exec` to opt in to exec provider checks/execution in either mode.
`--from` must point to a regular file no larger than 16 MiB (16,777,216 bytes). The byte limit applies to the complete serialized file, including whitespace.
What `apply` may update:
- `openclaw.json` (SecretRef targets + provider upserts/deletes)
+1
View File
@@ -3877,6 +3877,7 @@ Do not edit it by hand; run `pnpm docs:map:gen`.
- Route: /gateway/secrets-plan-contract
- Headings:
- H2: Plan file requirements
- H2: Plan file shape
- H2: Provider upserts and deletes
- H2: Supported target scope
+6
View File
@@ -9,6 +9,12 @@ title: "Secrets apply plan contract"
This page defines the strict contract enforced by `openclaw secrets apply`. If a target does not match these rules, apply fails before mutating any file.
## Plan file requirements
`openclaw secrets apply --from <plan.json>` accepts regular files up to 16 MiB (16,777,216 bytes). The limit applies to the complete serialized file, including whitespace. Directories, FIFOs, device files, and files larger than the limit are rejected before JSON parsing or target validation.
`openclaw secrets configure --plan-out <plan.json>` enforces the same limit on the UTF-8 serialized output before creating the file. Hand-written plans and external plan generators must also keep the serialized file within this boundary.
## Plan file shape
`openclaw secrets apply --from <plan.json>` expects a `targets` array of plan targets:
+141
View File
@@ -1,7 +1,9 @@
// Secrets CLI tests cover secret command registration, reads, writes, and redaction.
import { execFile } from "node:child_process";
import fs from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import { promisify } from "node:util";
import { Command } from "commander";
import { beforeEach, describe, expect, it, vi } from "vitest";
import {
@@ -11,6 +13,8 @@ import {
} from "../test-utils/mock-call-assertions.js";
import { registerSecretsCli } from "./secrets-cli.js";
const execFileAsync = promisify(execFile);
const mocks = await vi.hoisted(async () => {
const { createCliRuntimeMock } = await import("./test-runtime-mock.js");
const runtime = createCliRuntimeMock(vi);
@@ -105,6 +109,29 @@ function createConfigureInteractiveResult(options?: {
};
}
function createConfigureInteractiveResultWithPlanBytes(bytes: number) {
const configured = createConfigureInteractiveResult({
targets: [
{
type: "models.providers.apiKey",
path: "models.providers.openai.apiKey",
pathSegments: ["models", "providers", "openai", "apiKey"],
ref: {
source: "file",
provider: "default",
id: "",
},
providerId: "openai",
},
],
});
const target = configured.plan.targets[0] as { ref: { id: string } };
const emptyBytes = Buffer.byteLength(`${JSON.stringify(configured.plan, null, 2)}\n`, "utf8");
target.ref.id = "x".repeat(bytes - emptyBytes);
expect(Buffer.byteLength(`${JSON.stringify(configured.plan, null, 2)}\n`, "utf8")).toBe(bytes);
return configured;
}
function createSecretsApplyResult(options?: {
mode?: "dry-run" | "write";
changed?: boolean;
@@ -357,6 +384,120 @@ describe("secrets CLI", () => {
});
});
it("writes generated secrets plan files at the apply limit", async () => {
const planPath = path.join(
os.tmpdir(),
`openclaw-secrets-configure-test-${Date.now()}-${Math.random().toString(16).slice(2)}.json`,
);
runSecretsConfigureInteractive.mockResolvedValue(
createConfigureInteractiveResultWithPlanBytes(16 * 1024 * 1024),
);
confirm.mockResolvedValue(false);
try {
await createProgram().parseAsync(["secrets", "configure", "--plan-out", planPath], {
from: "user",
});
expect((await fs.stat(planPath)).size).toBe(16 * 1024 * 1024);
expect(runtimeLogs).toContain(`Plan written to ${planPath}`);
expect(runSecretsApply).not.toHaveBeenCalled();
} finally {
await fs.rm(planPath, { force: true });
}
});
it("rejects generated secrets plan files that exceed the apply limit", async () => {
const planPath = path.join(
os.tmpdir(),
`openclaw-secrets-configure-test-${Date.now()}-${Math.random().toString(16).slice(2)}.json`,
);
runSecretsConfigureInteractive.mockResolvedValue(
createConfigureInteractiveResultWithPlanBytes(16 * 1024 * 1024 + 1),
);
try {
await expect(
createProgram().parseAsync(["secrets", "configure", "--plan-out", planPath], {
from: "user",
}),
).rejects.toThrow("__exit__:1");
expect(runtimeErrors.at(-1)).toContain("Secrets plan exceeds 16777216 bytes");
await expect(fs.access(planPath)).rejects.toMatchObject({ code: "ENOENT" });
expect(confirm).not.toHaveBeenCalled();
expect(runSecretsApply).not.toHaveBeenCalled();
} finally {
await fs.rm(planPath, { force: true });
}
});
it("rejects oversized secrets plan files before parsing", async () => {
await withPlanFile(async (planPath) => {
await fs.truncate(planPath, 16 * 1024 * 1024 + 1);
await expect(
createProgram().parseAsync(["secrets", "apply", "--from", planPath, "--dry-run"], {
from: "user",
}),
).rejects.toThrow("__exit__:1");
expect(runSecretsApply).not.toHaveBeenCalled();
expect(runtimeErrors.at(-1)).toContain("Secrets plan file exceeds 16777216 bytes");
});
});
it.skipIf(process.platform === "win32")(
"rejects FIFO secrets plan paths without blocking",
async () => {
runSecretsApply.mockResolvedValue(createSecretsApplyResult());
await withPlanFile(async (planPath) => {
await createProgram().parseAsync(["secrets", "apply", "--from", planPath, "--dry-run"], {
from: "user",
});
});
runSecretsApply.mockReset();
runtimeLogs.length = 0;
runtimeErrors.length = 0;
const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-secrets-cli-fifo-"));
const fifoPath = path.join(tmpDir, "plan.json");
await execFileAsync("mkfifo", [fifoPath]);
let timedOut = false;
let timeout: NodeJS.Timeout | undefined;
const parse = createProgram().parseAsync(
["secrets", "apply", "--from", fifoPath, "--dry-run"],
{ from: "user" },
);
try {
await expect(
Promise.race([
parse,
new Promise<never>((_, reject) => {
timeout = setTimeout(() => {
timedOut = true;
reject(new Error("Timed out waiting for FIFO plan rejection"));
}, 1_000);
}),
]),
).rejects.toThrow("__exit__:1");
expect(runSecretsApply).not.toHaveBeenCalled();
expect(runtimeErrors.at(-1)).toContain("Secrets plan path is not a regular file");
} finally {
if (timeout) {
clearTimeout(timeout);
}
if (timedOut) {
const releaseWriter = execFileAsync("sh", ["-c", 'printf x > "$1"', "sh", fifoPath]);
await Promise.allSettled([parse, releaseWriter]);
}
await fs.rm(tmpDir, { recursive: true, force: true });
}
},
);
it("forwards --allow-exec to secrets apply dry-run", async () => {
await withPlanFile(async (planPath) => {
runSecretsApply.mockResolvedValue(createSecretsApplyResult());
+36 -8
View File
@@ -48,22 +48,50 @@ const secretsApplyLoader = createLazyImportLoader<SecretsApplyModule>(
class SecretsPlanFileNotFoundError extends Error {}
const SECRETS_PLAN_MAX_BYTES = 16 * 1024 * 1024;
function serializePlanFile(plan: SecretsApplyPlan, pathname: string): string {
const raw = `${JSON.stringify(plan, null, 2)}\n`;
if (Buffer.byteLength(raw, "utf8") > SECRETS_PLAN_MAX_BYTES) {
throw new RangeError(
`Secrets plan exceeds ${SECRETS_PLAN_MAX_BYTES} bytes and cannot be written: ${pathname}`,
);
}
return raw;
}
async function readPlanFile(pathname: string): Promise<SecretsApplyPlan> {
// Apply consumes a generated plan shape, not arbitrary JSON.
const [{ readFileSync }, { isSecretsApplyPlan }] = await Promise.all([
const [fsModule, { readFileDescriptorBounded }, { isSecretsApplyPlan }] = await Promise.all([
fsModuleLoader.load(),
import("../infra/file-descriptor-read.js"),
import("../secrets/plan.js"),
]);
let raw: string;
try {
raw = readFileSync(pathname, "utf8");
} catch (err) {
const fsConstants = fsModule.constants as typeof fsModule.constants & { O_NONBLOCK?: number };
// Non-blocking open lets descriptor stat reject special files without a FIFO stalling first.
const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NONBLOCK ?? 0);
const file = await fsModule.promises.open(pathname, openFlags).catch((err: unknown) => {
if (hasErrnoCode(err, "ENOENT")) {
throw new SecretsPlanFileNotFoundError(`Secrets plan file not found: ${pathname}`, {
cause: err,
});
}
throw err;
});
let raw: string;
try {
const stat = await file.stat();
if (!stat.isFile()) {
throw new Error(`Secrets plan path is not a regular file: ${pathname}`);
}
if (stat.size > SECRETS_PLAN_MAX_BYTES) {
throw new RangeError(
`Secrets plan file exceeds ${SECRETS_PLAN_MAX_BYTES} bytes: ${pathname}`,
);
}
raw = (await readFileDescriptorBounded(file.fd, SECRETS_PLAN_MAX_BYTES)).toString("utf8");
} finally {
await file.close();
}
let parsed: unknown;
try {
@@ -198,7 +226,7 @@ export function registerSecretsCli(program: Command): void {
"Allow exec SecretRef preflight checks (may execute provider commands)",
false,
)
.option("--plan-out <path>", "Write generated plan JSON to a file")
.option("--plan-out <path>", "Write generated plan JSON to a file (max 16 MiB)")
.option("--json", "Output JSON", false)
.action(async (opts: SecretsConfigureOptions) => {
try {
@@ -211,7 +239,7 @@ export function registerSecretsCli(program: Command): void {
});
if (opts.planOut) {
const { writeFileSync } = await fsModuleLoader.load();
writeFileSync(opts.planOut, `${JSON.stringify(configured.plan, null, 2)}\n`, "utf8");
writeFileSync(opts.planOut, serializePlanFile(configured.plan, opts.planOut), "utf8");
}
let shouldApply = Boolean(opts.apply || opts.yes);
@@ -307,7 +335,7 @@ export function registerSecretsCli(program: Command): void {
secrets
.command("apply")
.description("Apply a previously generated secrets plan")
.requiredOption("--from <path>", "Path to plan JSON")
.requiredOption("--from <path>", "Path to plan JSON (max 16 MiB)")
.option("--dry-run", "Validate/preflight only", false)
.option("--allow-exec", "Allow exec SecretRef checks (may execute provider commands)", false)
.option("--json", "Output JSON", false)