From b067a4dce343c239d3bc183a8f471ef65c7b5ea5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 11 Aug 2026 02:30:04 -0700 Subject: [PATCH] fix(secrets): reject empty secret values and classify oversized input as validation (#121947) --- docs/cli/secrets.md | 2 +- docs/gateway/secrets.md | 2 +- src/cli/secrets-store-cli.test.ts | 26 ++++++++++++++++++++++++- src/cli/secrets-store-cli.ts | 3 ++- src/cli/secrets-store-input.ts | 18 ++++++++++++++--- src/secrets/store/secret-store.test.ts | 27 ++++++++++++++++++++++++++ src/secrets/store/secret-store.ts | 19 +++++++++++++++--- 7 files changed, 87 insertions(+), 10 deletions(-) diff --git a/docs/cli/secrets.md b/docs/cli/secrets.md index 81483569f31a..370364567316 100644 --- a/docs/cli/secrets.md +++ b/docs/cli/secrets.md @@ -53,7 +53,7 @@ openclaw secrets store rm ... openclaw secrets store import [--from ] ``` -Names must match `^[A-Z][A-Z0-9_]{0,127}$`. Values are limited to 64 KiB (65,536 UTF-8 bytes). `--kind secret|env` overrides automatic kind detection; otherwise names ending in common credential suffixes such as `_API_KEY`, `_TOKEN`, `_PASSWORD`, `_PRIVATE_KEY`, or `_SECRET` become `secret`, and other names become `env`. +Names must match `^[A-Z][A-Z0-9_]{0,127}$`. Values are limited to 64 KiB (65,536 UTF-8 bytes); an oversized value is rejected with exit code 2 whether it arrives from stdin, `--value`, or `--value-file`. A `secret` entry may not be empty, because an empty credential cannot be diagnosed later (`get` refuses secret kinds and listings mask them); `env` entries may be empty. `--kind secret|env` overrides automatic kind detection; otherwise names ending in common credential suffixes such as `_API_KEY`, `_TOKEN`, `_PASSWORD`, `_PRIVATE_KEY`, or `_SECRET` become `secret`, and other names become `env`. ### Set values safely diff --git a/docs/gateway/secrets.md b/docs/gateway/secrets.md index ae440ca8c9c2..a05de68086c1 100644 --- a/docs/gateway/secrets.md +++ b/docs/gateway/secrets.md @@ -282,7 +282,7 @@ Entries have a `secret` or `env` kind. The kind controls CLI disclosure, not Sec `secret` entries are never injected into subprocess environments. They remain available only through `store` SecretRefs because plaintext env injection would bypass the store disclosure boundary; safe secret injection requires a future egress-substitution mechanism. -Names use the same uppercase grammar as env SecretRefs, and each UTF-8 value is limited to 64 KiB (65,536 bytes). This supports PEM keys and service-account JSON without inheriting the smaller limits of ordinary environment variables. +Names use the same uppercase grammar as env SecretRefs, and each UTF-8 value is limited to 64 KiB (65,536 bytes). A `secret` entry must carry a value; empty secrets are rejected because they would surface only as a confusing downstream auth failure. `env` entries may be empty. This supports PEM keys and service-account JSON without inheriting the smaller limits of ordinary environment variables. Reference an entry from `openclaw.json` with the `store` source: diff --git a/src/cli/secrets-store-cli.test.ts b/src/cli/secrets-store-cli.test.ts index 8e368d2b4991..a5f84a3594fa 100644 --- a/src/cli/secrets-store-cli.test.ts +++ b/src/cli/secrets-store-cli.test.ts @@ -20,7 +20,12 @@ const mocks = await vi.hoisted(async () => { }); vi.mock("../runtime.js", () => ({ defaultRuntime: mocks.defaultRuntime })); -vi.mock("../secrets/store/secret-store.js", () => ({ +vi.mock("../secrets/store/secret-store.js", async (importOriginal) => ({ + // Import the real validation error: the CLI classifies size/empty failures by it, + // and a stub class would silently change the mapped exit code. + SecretStoreValidationError: ( + await importOriginal() + ).SecretStoreValidationError, SECRET_STORE_VALUE_MAX_BYTES: 64 * 1024, listSecretStoreEntries: (params: unknown) => mocks.list(params), readSecretStoreValue: (params: unknown) => mocks.read(params), @@ -79,6 +84,25 @@ describe("secrets store CLI", () => { expect(mocks.write).not.toHaveBeenCalled(); }); + it("reports an oversized --value-file as validation (exit 2), matching the stdin path", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "secret-store-cli-oversize-")); + const file = path.join(dir, "too-big.txt"); + await fs.writeFile(file, "a".repeat(64 * 1024 + 1), "utf8"); + try { + // Same violation as an oversized stdin value, so it must share exit code 2 + // rather than falling through to the generic runtime-failure code. + await expect( + createProgram().parseAsync( + ["secrets", "store", "set", "BIG_ENV_VALUE", "--kind", "env", "--value-file", file], + { from: "user" }, + ), + ).rejects.toThrow("__exit__:2"); + expect(mocks.write).not.toHaveBeenCalled(); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + it("refuses get for secret entries without reading their values", async () => { mocks.list.mockReturnValue([{ name: "SERVICE_API_KEY", kind: "secret" }]); await expect( diff --git a/src/cli/secrets-store-cli.ts b/src/cli/secrets-store-cli.ts index 08b53674bd37..a878406dfc4f 100644 --- a/src/cli/secrets-store-cli.ts +++ b/src/cli/secrets-store-cli.ts @@ -73,7 +73,8 @@ function mapStoreError(error: unknown): SecretStoreCliFailure { if ( validation?.name === "SecretStoreValidationError" && (validation.code === "SECRET_STORE_INVALID_NAME" || - validation.code === "SECRET_STORE_VALUE_TOO_LARGE") + validation.code === "SECRET_STORE_VALUE_TOO_LARGE" || + validation.code === "SECRET_STORE_VALUE_EMPTY") ) { return new SecretStoreCliFailure(2, validation.message ?? "Invalid secret store input."); } diff --git a/src/cli/secrets-store-input.ts b/src/cli/secrets-store-input.ts index 868c14424df4..e232c1f8044e 100644 --- a/src/cli/secrets-store-input.ts +++ b/src/cli/secrets-store-input.ts @@ -3,7 +3,10 @@ import { password } from "@clack/prompts"; import { readByteStreamWithLimit } from "@openclaw/media-core/read-byte-stream-with-limit"; import { readFileDescriptorBounded } from "../infra/boundary-file-read.js"; import { parseSecretStoreDotEnvText } from "../secrets/store/dotenv.js"; -import { SECRET_STORE_VALUE_MAX_BYTES } from "../secrets/store/secret-store.js"; +import { + SECRET_STORE_VALUE_MAX_BYTES, + SecretStoreValidationError, +} from "../secrets/store/secret-store.js"; const SECRET_STORE_IMPORT_MAX_BYTES = 16 * 1024 * 1024; @@ -14,7 +17,13 @@ function stripOneTerminalNewline(value: string): string { async function readBoundedStdin(maxBytes: number): Promise { const bytes = await readByteStreamWithLimit(process.stdin, { maxBytes, - onOverflow: ({ maxBytes: limit }) => new Error(`Stdin input exceeds ${limit} bytes.`), + // Oversized input is the same validation failure as an oversized stored value, + // so it must carry the typed code the CLI maps to exit 2 on every input path. + onOverflow: ({ maxBytes: limit }) => + new SecretStoreValidationError( + "SECRET_STORE_VALUE_TOO_LARGE", + `Stdin input exceeds ${limit} bytes.`, + ), }); return bytes.toString("utf8"); } @@ -27,7 +36,10 @@ async function readBoundedFile(pathname: string, maxBytes: number): Promise maxBytes) { - throw new Error(`Input file exceeds ${maxBytes} bytes: ${pathname}`); + throw new SecretStoreValidationError( + "SECRET_STORE_VALUE_TOO_LARGE", + `Input file exceeds ${maxBytes} bytes: ${pathname}`, + ); } return (await readFileDescriptorBounded(file.fd, maxBytes)).toString("utf8"); } finally { diff --git a/src/secrets/store/secret-store.test.ts b/src/secrets/store/secret-store.test.ts index 8520d0d8ef3c..a50910604304 100644 --- a/src/secrets/store/secret-store.test.ts +++ b/src/secrets/store/secret-store.test.ts @@ -137,6 +137,33 @@ describe("secret store", () => { ).toThrow(expect.objectContaining({ code: "SECRET_STORE_VALUE_TOO_LARGE" })); }); + it("rejects an empty secret value but keeps empty env values legal", () => { + const database = createDatabaseOptions(); + // A silently-empty secret (a failed `op read |` pipe) is undiagnosable later: + // get refuses secret kinds and listings mask them, so reject it at the writer. + expect(() => + writeSecretStoreEntry({ + scope: team, + name: "EMPTY_SECRET", + value: "", + kind: "secret", + updatedBy: null, + database, + }), + ).toThrow(expect.objectContaining({ code: "SECRET_STORE_VALUE_EMPTY" })); + + writeSecretStoreEntry({ + scope: team, + name: "EMPTY_ENV", + value: "", + kind: "env", + updatedBy: null, + database, + }); + const stored = readSecretStoreValue({ scope: team, name: "EMPTY_ENV", database }); + expect(stored.ok && stored.value).toBe(""); + }); + it("treats a missing lazy table as empty and preserves schema version 6 on ensure", () => { const database = createDatabaseOptions(); openOpenClawStateDatabase(database); diff --git a/src/secrets/store/secret-store.ts b/src/secrets/store/secret-store.ts index 3ef41a4fb46d..4c4421fb8848 100644 --- a/src/secrets/store/secret-store.ts +++ b/src/secrets/store/secret-store.ts @@ -38,7 +38,10 @@ type SecretStoreReadError = | { code: "SECRET_STORE_INVALID_NAME"; message: string } | { code: "SECRET_STORE_UNAVAILABLE"; message: string; cause: unknown }; -type SecretStoreValidationCode = "SECRET_STORE_INVALID_NAME" | "SECRET_STORE_VALUE_TOO_LARGE"; +type SecretStoreValidationCode = + | "SECRET_STORE_INVALID_NAME" + | "SECRET_STORE_VALUE_TOO_LARGE" + | "SECRET_STORE_VALUE_EMPTY"; export class SecretStoreValidationError extends Error { constructor( @@ -66,7 +69,7 @@ function assertSecretStoreName(name: string): void { } } -function assertSecretStoreValue(value: string): void { +function assertSecretStoreValue(value: string, kind: SecretStoreKind): void { const bytes = Buffer.byteLength(value, "utf8"); if (bytes > SECRET_STORE_VALUE_MAX_BYTES) { throw new SecretStoreValidationError( @@ -74,6 +77,16 @@ function assertSecretStoreValue(value: string): void { `Secret store value exceeds ${SECRET_STORE_VALUE_MAX_BYTES} UTF-8 bytes.`, ); } + // An empty credential is never meaningful and cannot be diagnosed later: `get` + // refuses secret kinds and listings mask them, so a silently-empty secret (a + // failed `op read |` pipe, for example) would surface only as a confusing 401. + // Env entries may legitimately be empty. + if (kind === "secret" && value.length === 0) { + throw new SecretStoreValidationError( + "SECRET_STORE_VALUE_EMPTY", + "Secret store value is empty. Secret entries require a value; check the command that produced it.", + ); + } } function isMissingSecretStoreTableError(error: unknown): boolean { @@ -188,7 +201,7 @@ export function writeSecretStoreEntry(params: { database?: OpenClawStateDatabaseOptions; }): void { assertSecretStoreName(params.name); - assertSecretStoreValue(params.value); + assertSecretStoreValue(params.value, params.kind); const { scopeKind, scopeId } = normalizeScope(params.scope); const now = Date.now(); runOpenClawStateWriteTransaction(