From e1007acb7e9069c129caa33b41e41d3320603a9d Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Wed, 3 Jun 2026 21:08:01 -0300 Subject: [PATCH] fix(cli): bare omniroute (default serve) must provision STORAGE_ENCRYPTION_KEY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My #3129 gate wrongly skipped provisioning for a bare `omniroute` invocation — but `serve` is isDefault:true, so bare runs the server, which needs the key. Only --version/--help/help/completion skip now. Realigns with #1622: its bootstrap test invoked `--help` (now correctly skipped), so it's switched to `config list --json` (a real, fast, offline command) to exercise the provisioning path. --- bin/cli/utils/storageKeyProvision.mjs | 17 ++++++++++------- tests/unit/cli-storage-key-bootstrap.test.ts | 6 +++++- tests/unit/cli-storage-key-provision.test.ts | 12 +++++++++--- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/bin/cli/utils/storageKeyProvision.mjs b/bin/cli/utils/storageKeyProvision.mjs index 3d9b1c338..e3ccafade 100644 --- a/bin/cli/utils/storageKeyProvision.mjs +++ b/bin/cli/utils/storageKeyProvision.mjs @@ -5,13 +5,15 @@ * Purely informational invocations must NOT create `~/.omniroute/.env` or write * a key — they never touch encrypted storage. Generating a 32-byte key and a * `.env` file just to print `omniroute --version` (or `--help`) is a surprising - * side effect: running a read-only command should not mutate the data dir. + * side effect: a read-only command should not mutate the data dir. * - * Returns false for: no command at all (bare `omniroute` → Commander prints - * help), `--version`/`-V` or `--help`/`-h` anywhere in the args, and the - * `help`/`completion` subcommands. Returns true for every real command - * (`serve`, `keys`, `providers`, …) so the encryption key is still provisioned - * before encrypted storage is accessed (preserves the #1622 persistence fix). + * Returns FALSE only for: `--version`/`-V` or `--help`/`-h` anywhere in the args, + * and the `help`/`completion` subcommands. + * + * Returns TRUE for everything else, INCLUDING a bare `omniroute` (no args) — the + * `serve` command is `isDefault: true`, so a bare invocation starts the server, + * which needs the encryption key. This preserves the #1622 persistence fix + * (key generated on first real run and reused across restarts). * * @param {string[]} argv - process.argv (node + script + args). * @returns {boolean} @@ -21,7 +23,8 @@ const INFO_COMMANDS = new Set(["help", "completion"]); export function shouldProvisionStorageKey(argv) { const args = Array.isArray(argv) ? argv.slice(2) : []; - if (args.length === 0) return false; + // Bare `omniroute` runs the default `serve` command → must provision. + if (args.length === 0) return true; if (args.some((a) => INFO_FLAGS.has(a))) return false; if (INFO_COMMANDS.has(args[0])) return false; return true; diff --git a/tests/unit/cli-storage-key-bootstrap.test.ts b/tests/unit/cli-storage-key-bootstrap.test.ts index b3a77ce19..3664ab5e8 100644 --- a/tests/unit/cli-storage-key-bootstrap.test.ts +++ b/tests/unit/cli-storage-key-bootstrap.test.ts @@ -23,7 +23,11 @@ function runCli(dataDir: string): { code: number | null; stderr: string } { // the bootstrap skips writing DATA_DIR/.env (the behaviour the test exercises). const isolatedHome = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-key-home-")); try { - const res = spawnSync("node", [BIN, "--help"], { + // Use a real (non-informational) command so the STORAGE_ENCRYPTION_KEY + // bootstrap runs. `--version`/`--help` are intentionally skipped now (#3129), + // so the #1622 provisioning path must be exercised by an actual command. + // `config list --json` is fast and offline (no server, no network). + const res = spawnSync("node", [BIN, "config", "list", "--json"], { cwd: dataDir, env: { ...cleanEnv, diff --git a/tests/unit/cli-storage-key-provision.test.ts b/tests/unit/cli-storage-key-provision.test.ts index 8d8670c8b..f74567921 100644 --- a/tests/unit/cli-storage-key-provision.test.ts +++ b/tests/unit/cli-storage-key-provision.test.ts @@ -6,7 +6,6 @@ import { shouldProvisionStorageKey } from "../../bin/cli/utils/storageKeyProvisi const argv = (...args: string[]) => ["node", "omniroute", ...args]; test("storage key: informational commands do NOT provision a key", () => { - assert.equal(shouldProvisionStorageKey(argv()), false); // bare omniroute → help assert.equal(shouldProvisionStorageKey(argv("--version")), false); assert.equal(shouldProvisionStorageKey(argv("-V")), false); assert.equal(shouldProvisionStorageKey(argv("--help")), false); @@ -15,6 +14,11 @@ test("storage key: informational commands do NOT provision a key", () => { assert.equal(shouldProvisionStorageKey(argv("completion")), false); }); +test("storage key: bare `omniroute` provisions (serve is the default command)", () => { + // No args → Commander runs the isDefault `serve` command, which needs the key. + assert.equal(shouldProvisionStorageKey(argv()), true); +}); + test("storage key: --help/--version anywhere in the args still skips", () => { assert.equal(shouldProvisionStorageKey(argv("serve", "--help")), false); assert.equal(shouldProvisionStorageKey(argv("keys", "list", "-h")), false); @@ -30,7 +34,9 @@ test("storage key: real commands DO provision (preserves #1622 persistence)", () assert.equal(shouldProvisionStorageKey(argv("--lang", "en", "serve")), true); }); -test("storage key: defensive on non-array input", () => { +test("storage key: defensive on non-array input → fail-safe to provisioning", () => { + // Non-array argv collapses to [] → treated as a bare invocation (default serve), + // so it provisions. Fail-safe: better to have the key than to skip it. // @ts-expect-error intentional bad input - assert.equal(shouldProvisionStorageKey(undefined), false); + assert.equal(shouldProvisionStorageKey(undefined), true); });