From 11678b828d1ccc5bfc75d3eaa6ec36ab23be6262 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 6 Jul 2026 14:55:01 -0300 Subject: [PATCH] fix(cli): register reset-password subcommand + non-TTY stdin path (#6261, #6258) --- CHANGELOG.md | 2 + bin/omniroute.mjs | 10 ++ bin/reset-password.mjs | 102 ++++++++--- docs/reference/CLI-TOOLS.md | 2 +- .../unit/reset-password-cli-6261-6258.test.ts | 168 ++++++++++++++++++ 5 files changed, 259 insertions(+), 25 deletions(-) create mode 100644 tests/unit/reset-password-cli-6261-6258.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 41625e6242..d243047b75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ ### šŸ› Bug Fixes +- **fix(cli):** `omniroute reset-password` now works as a real subcommand, and password resets over piped (non-TTY) stdin actually apply ([#6261](https://github.com/diegosouzapw/OmniRoute/issues/6261), [#6258](https://github.com/diegosouzapw/OmniRoute/issues/6258)). Two coupled defects: (1) **#6261** ā€” `bin/omniroute.mjs` routed everything through Commander with only two pre-Commander bypasses (`--mcp`, `reset-encrypted-columns`), so `omniroute reset-password` was rejected as an unknown command; only the separate `omniroute-reset-password` bin worked, while the docs falsely advertised the subcommand (incl. a bogus "legacy alias still works"). A pre-Commander bypass mirroring `reset-encrypted-columns` now dynamically imports `bin/reset-password.mjs` (which self-executes) before Commander parses; the three doc lines were corrected. (2) **#6258** ā€” `bin/reset-password.mjs` issued two sequential `rl.question` prompts; under piped stdin the second read never settled at EOF, so `main()` never reached `resetManagementPassword` and the reset was a silent no-op (both prompts printed, no success, password unchanged). The CLI now detects non-TTY stdin and reads it once (first line = password, second line = confirm if present, else reused), adds a `--password-stdin` flag (entire stdin is the password, no confirmation), and exits `0` explicitly so the success line always flushes; interactive TTY behavior is unchanged. Regression guard: `tests/unit/reset-password-cli-6261-6258.test.ts` (3). (thanks @chirag127) + - **fix(db):** the mass-migration **safety abort** now tells the operator how to bypass it and stops flooding the log ([#6260](https://github.com/diegosouzapw/OmniRoute/issues/6260)) ā€” after restoring a backup that wiped the migration tracking table, `runMigrations()` threw the abort on every downstream `ensureDbInitialized()`, re-logging the full banner 11+ times, and the message never mentioned the existing `OMNIROUTE_MAX_PENDING_MIGRATIONS` escape hatch. The abort text now appends a bypass hint (set `OMNIROUTE_MAX_PENDING_MIGRATIONS=0` in `server.env` / `DATA_DIR/.env`), and a new `MigrationSafetyAbortError` is memoized so repeated calls in the same process throw the same instance and emit a single concise line instead of the full cascade. Regression guard: `tests/unit/migration-safety-abort-6260.test.ts`. (thanks @chirag127) - **fix(auth):** importing a **distinct** Codex/ChatGPT OAuth `auth.json` is no longer falsely rejected as "already exists" when it belongs to a different user in the same workspace ([#6301](https://github.com/diegosouzapw/OmniRoute/issues/6301)). `findExistingCodexConnection` (in `src/lib/oauth/utils/codexAuthImport.ts`) deduped **only** on `providerSpecificData.workspaceId === accountId`, where `accountId` is the shared `chatgpt_account_id`/`tokens.account_id` ā€” so two members of the same ChatGPT Team collapsed onto a single connection (409 `duplicate_account`). The id_token's `https://api.openai.com/auth` claim carries a per-user `chatgpt_user_id` alongside the workspace id (the device-flow path already persisted it as `chatgptUserId`, but the import path did not). Now `parseAndValidateCodexAuth` extracts `userId` (`chatgpt_user_id` ā†’ `user_id` ā†’ JWT `sub`) into `ParsedCodexAuth`, the create/update paths persist `chatgptUserId` in `providerSpecificData` (mirroring `codex.ts`), and dedup keys on `workspaceId` **AND** `chatgptUserId` ā€” with a backward-compat fallback to legacy accountId-only matching when no stored connection for that workspace records a `chatgptUserId`, so genuinely-same accounts still dedup. Regression guard: `tests/unit/codex-auth-import-userid-dedup-6301.test.ts` (4). (thanks @anungma) diff --git a/bin/omniroute.mjs b/bin/omniroute.mjs index 77f7418d65..e1ef7b0e9a 100755 --- a/bin/omniroute.mjs +++ b/bin/omniroute.mjs @@ -6,6 +6,7 @@ * Special bypasses (handled before Commander): * --mcp Start MCP server over stdio * reset-encrypted-columns Recovery tool for broken encrypted credentials + * reset-password Reset the admin/management password * * All other commands are routed through Commander (bin/cli/program.mjs). */ @@ -210,6 +211,15 @@ if (process.argv.includes("reset-encrypted-columns")) { process.exit(exitCode ?? 0); } +if (process.argv.includes("reset-password")) { + // bin/reset-password.mjs self-executes its `main()` on import and calls + // process.exit() on completion/error. Await a never-resolving promise so + // control never falls through to Commander (which would then reject + // `reset-password` as an unknown command). See #6261. + await import(pathToFileURL(join(ROOT, "bin", "reset-password.mjs")).href); + await new Promise(() => {}); +} + try { const { createProgram } = await import( pathToFileURL(join(ROOT, "bin", "cli", "program.mjs")).href diff --git a/bin/reset-password.mjs b/bin/reset-password.mjs index d21e2c5193..ab40282b0d 100755 --- a/bin/reset-password.mjs +++ b/bin/reset-password.mjs @@ -5,10 +5,15 @@ * * Usage: * node bin/reset-password.mjs - * npx omniroute reset-password + * omniroute reset-password + * + * Non-interactive / scripted usage (piped stdin, e.g. CI or Docker): + * printf 'NewPass123\nNewPass123\n' | omniroute reset-password + * printf 'NewPass123' | omniroute reset-password --password-stdin * * Resets the admin password for OmniRoute. - * Prompts for a new password and updates the database directly. + * Prompts for a new password (interactive TTY) or reads it from stdin + * (non-TTY) and updates the database directly. * * @module bin/reset-password */ @@ -21,19 +26,61 @@ import { readManagementPasswordState, resetManagementPassword } from "./cli/sqli const DATA_DIR = resolveDataDir(); const DB_PATH = resolveStoragePath(DATA_DIR); -const rl = createInterface({ - input: process.stdin, - output: process.stdout, -}); +const MIN_PASSWORD_LENGTH = 8; -function ask(question) { - return new Promise((resolve) => rl.question(question, resolve)); +/** Read the entire stdin stream as a UTF-8 string (used for non-TTY input). */ +function readAllStdin() { + return new Promise((resolve) => { + let data = ""; + process.stdin.setEncoding("utf8"); + process.stdin.on("data", (chunk) => { + data += chunk; + }); + process.stdin.on("end", () => resolve(data)); + process.stdin.on("error", () => resolve(data)); + // Resuming is implied by attaching a 'data' listener, but be explicit so a + // paused stream (some spawn setups) still flows to EOF. + process.stdin.resume(); + }); } -function exitWithError(message) { - console.error(message); - rl.close(); - process.exit(1); +/** + * Obtain the new password (and its confirmation). + * + * - `--password-stdin`: the ENTIRE stdin is the password, no confirmation. + * - non-TTY stdin (piped): read all of stdin once; first line is the password, + * second line ā€” when present ā€” is the confirmation, else the first line is + * reused (a single-line pipe means "no separate confirmation"). + * - interactive TTY: two sequential prompts (unchanged behavior). + * + * The non-TTY path exists because two sequential `rl.question` promises never + * settle under a piped EOF ā€” the second read blocks forever, so the reset was + * silently never applied (#6258). + */ +async function collectPassword() { + if (process.argv.includes("--password-stdin")) { + const raw = await readAllStdin(); + const password = raw.replace(/[\r\n]+$/, ""); + return { password, confirm: password }; + } + + if (!process.stdin.isTTY) { + const raw = await readAllStdin(); + const lines = raw.split(/\r?\n/); + const password = lines[0] ?? ""; + const confirm = lines[1] ? lines[1] : password; + return { password, confirm }; + } + + const rl = createInterface({ input: process.stdin, output: process.stdout }); + try { + const ask = (question) => new Promise((resolve) => rl.question(question, resolve)); + const password = await ask("Enter new password (min 8 chars): "); + const confirm = await ask("Confirm new password: "); + return { password, confirm }; + } finally { + rl.close(); + } } console.log("\nšŸ”‘ OmniRoute ā€” Password Reset\n"); @@ -54,27 +101,34 @@ async function main() { console.log("ā„¹ļø No password is currently set."); } - const password = await ask("Enter new password (min 8 chars): "); + const { password, confirm } = await collectPassword(); - if (!password || password.length < 8) { - exitWithError("\nāŒ Password must be at least 8 characters.\n"); + if (!password || password.length < MIN_PASSWORD_LENGTH) { + console.error(`\nāŒ Password must be at least ${MIN_PASSWORD_LENGTH} characters.\n`); + process.exit(1); } - const confirm = await ask("Confirm new password: "); - if (password !== confirm) { - exitWithError("\nāŒ Passwords do not match.\n"); + console.error("\nāŒ Passwords do not match.\n"); + process.exit(1); } await resetManagementPassword(password, DB_PATH); - rl.close(); console.log("\nāœ… Password reset successfully!"); console.log(" Restart OmniRoute for changes to take effect.\n"); } -main().catch((err) => { - console.error(`\nāŒ Error: ${err.message}\n`); - rl.close(); - process.exit(1); -}); +main() + .then(() => { + // Explicit exit(0) so a caller that imports this module (bin/omniroute.mjs + // routes `omniroute reset-password` here) terminates cleanly instead of + // hanging / exiting with code 13 on an unsettled wrapper await. On POSIX, + // console.log to a pipe is synchronous, so the success line is already + // flushed by the time we exit. + process.exit(0); + }) + .catch((err) => { + console.error(`\nāŒ Error: ${err.message}\n`); + process.exit(1); + }); diff --git a/docs/reference/CLI-TOOLS.md b/docs/reference/CLI-TOOLS.md index 42529429d2..ef38bbeb82 100644 --- a/docs/reference/CLI-TOOLS.md +++ b/docs/reference/CLI-TOOLS.md @@ -648,7 +648,7 @@ omniroute providers validate # Local-only structural vali ### Recovery & Reset ```bash -omniroute reset-password # Reset the admin password (legacy alias still works) +omniroute reset-password # Reset the admin password (also: omniroute-reset-password) omniroute reset-encrypted-columns # Show warning + dry-run for encrypted credential reset omniroute reset-encrypted-columns --force # Actually null out encrypted credentials in SQLite ``` diff --git a/tests/unit/reset-password-cli-6261-6258.test.ts b/tests/unit/reset-password-cli-6261-6258.test.ts new file mode 100644 index 0000000000..1a1c0e470c --- /dev/null +++ b/tests/unit/reset-password-cli-6261-6258.test.ts @@ -0,0 +1,168 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import bcrypt from "bcryptjs"; +import Database from "better-sqlite3"; + +const ROOT = path.join(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); +const OMNIROUTE_BIN = path.join(ROOT, "bin", "omniroute.mjs"); +const RESET_BIN = path.join(ROOT, "bin", "reset-password.mjs"); + +// Isolate every spawn from the development repo's .env and the machine's real +// ~/.omniroute so DATA_DIR is the only data directory in play. +function baseEnv(dataDir: string, isolatedHome: string): NodeJS.ProcessEnv { + const env = { ...process.env }; + return { + ...env, + DATA_DIR: dataDir, + HOME: isolatedHome, + // Give the CLI a key so bin/omniroute.mjs never warns/provisions. + STORAGE_ENCRYPTION_KEY: "0".repeat(64), + CI: "1", + NO_UPDATE_NOTIFIER: "1", + OMNIROUTE_NO_UPDATE_NOTIFIER: "1", + OMNIROUTE_CLI_SKIP_REPO_ENV: "1", + }; +} + +// Seed a storage.sqlite that already exists with the settings schema so the +// reset CLI passes its "database exists" precondition. +function seedDb(dataDir: string): string { + fs.mkdirSync(dataDir, { recursive: true }); + const dbPath = path.join(dataDir, "storage.sqlite"); + const db = new Database(dbPath); + db.pragma("journal_mode = WAL"); + db.prepare( + `CREATE TABLE IF NOT EXISTS key_value ( + namespace TEXT NOT NULL, + key TEXT NOT NULL, + value TEXT NOT NULL, + PRIMARY KEY (namespace, key) + )` + ).run(); + db.close(); + return dbPath; +} + +// Read the stored management password (JSON-encoded bcrypt hash) from the DB. +function readStoredPassword(dbPath: string): string | null { + const db = new Database(dbPath, { readonly: true }); + try { + const row = db + .prepare("SELECT value FROM key_value WHERE namespace = 'settings' AND key = 'password'") + .get() as { value?: string } | undefined; + if (!row?.value) return null; + try { + return JSON.parse(row.value); + } catch { + return row.value; + } + } finally { + db.close(); + } +} + +function mkHome(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-reset-home-")); +} +function mkDataDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-reset-data-")); +} + +// #6261: `omniroute reset-password` must be a real subcommand (not "unknown +// command"), routing into bin/reset-password.mjs ā€” and #6258: under piped +// (non-TTY) stdin it must actually apply the reset and print the success line. +test("omniroute reset-password subcommand applies the reset over piped stdin (#6261, #6258)", async () => { + const dataDir = mkDataDir(); + const home = mkHome(); + try { + const dbPath = seedDb(dataDir); + const res = spawnSync("node", [OMNIROUTE_BIN, "reset-password"], { + env: baseEnv(dataDir, home), + input: "ChangeMe\nChangeMe\n", + timeout: 60_000, + encoding: "utf-8", + }); + const out = `${res.stdout ?? ""}\n${res.stderr ?? ""}`; + assert.equal(res.status, 0, `expected exit 0, got ${res.status}. Output:\n${out}`); + assert.doesNotMatch( + out, + /unknown command|unknown option|error: unknown/i, + `must not be treated as an unknown command:\n${out}` + ); + assert.match(out, /Password Reset/i, `must enter the reset flow:\n${out}`); + assert.match(out, /reset successfully/i, `must print the success line:\n${out}`); + + const stored = readStoredPassword(dbPath); + assert.ok(stored, "a password must be persisted to the DB"); + assert.ok( + await bcrypt.compare("ChangeMe", stored as string), + "the stored password must verify against the piped value" + ); + } finally { + fs.rmSync(dataDir, { recursive: true, force: true }); + fs.rmSync(home, { recursive: true, force: true }); + } +}); + +// #6258: the standalone bin under piped (non-TTY) two-line stdin must not hang; +// it reads both lines, applies the reset, and flushes the success line. +test("omniroute-reset-password applies the reset over piped two-line stdin (#6258)", async () => { + const dataDir = mkDataDir(); + const home = mkHome(); + try { + const dbPath = seedDb(dataDir); + const res = spawnSync("node", [RESET_BIN], { + env: baseEnv(dataDir, home), + input: "ChangeMe\nChangeMe\n", + timeout: 60_000, + encoding: "utf-8", + }); + const out = `${res.stdout ?? ""}\n${res.stderr ?? ""}`; + assert.equal(res.status, 0, `expected exit 0, got ${res.status}. Output:\n${out}`); + assert.match(out, /reset successfully/i, `must print the success line:\n${out}`); + + const stored = readStoredPassword(dbPath); + assert.ok(stored, "a password must be persisted to the DB"); + assert.ok( + await bcrypt.compare("ChangeMe", stored as string), + "the stored password must verify against the piped value" + ); + } finally { + fs.rmSync(dataDir, { recursive: true, force: true }); + fs.rmSync(home, { recursive: true, force: true }); + } +}); + +// #6258: the --password-stdin flag reads the entire stdin as the password (no +// confirmation prompt) ā€” for scripted / automated resets. +test("omniroute-reset-password --password-stdin reads the whole stdin as the password (#6258)", async () => { + const dataDir = mkDataDir(); + const home = mkHome(); + try { + const dbPath = seedDb(dataDir); + const res = spawnSync("node", [RESET_BIN, "--password-stdin"], { + env: baseEnv(dataDir, home), + input: "ChangeMe\n", + timeout: 60_000, + encoding: "utf-8", + }); + const out = `${res.stdout ?? ""}\n${res.stderr ?? ""}`; + assert.equal(res.status, 0, `expected exit 0, got ${res.status}. Output:\n${out}`); + assert.match(out, /reset successfully/i, `must print the success line:\n${out}`); + + const stored = readStoredPassword(dbPath); + assert.ok(stored, "a password must be persisted to the DB"); + assert.ok( + await bcrypt.compare("ChangeMe", stored as string), + "the stored password must verify against the --password-stdin value" + ); + } finally { + fs.rmSync(dataDir, { recursive: true, force: true }); + fs.rmSync(home, { recursive: true, force: true }); + } +});