diff --git a/CHANGELOG.md b/CHANGELOG.md index fcc9b9ba73..863e083d6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ ### 🔧 Bug Fixes +- **Security hardening follow-ups (v3.8.15):** the `auth_token` cookie now sets an explicit 30-day `maxAge` so sessions persist as intended (Seg3); the management bootstrap warns at boot when `INITIAL_PASSWORD` is left at the insecure `CHANGEME` default (Seg2); VS Code path-token endpoints (`/api/v1/vscode/raw/[token]`) emit a once-per-process security warning since the API key travels in the URL and can leak via logs/proxies (Seg4); the system version route resolves the real global install path via `npm root -g` instead of a hardcoded `/app` (Bug3); and auto-update mode detection segment-matches `node_modules` instead of substring-matching, eliminating false "global install" positives (Bug1). + ### 📝 Maintenance --- diff --git a/src/app/api/auth/login/route.ts b/src/app/api/auth/login/route.ts index f4045f3fb4..98703a6e54 100644 --- a/src/app/api/auth/login/route.ts +++ b/src/app/api/auth/login/route.ts @@ -142,6 +142,9 @@ export async function POST(request) { secure: useSecureCookie, sameSite: "lax", path: "/", + // 30 days — bound the cookie lifetime to the JWT's 30d expiry so the browser + // drops it on the same schedule the token stops being valid (Seg3 hardening). + maxAge: 60 * 60 * 24 * 30, }); logAuditEvent({ diff --git a/src/app/api/system/version/route.ts b/src/app/api/system/version/route.ts index ee35e4d2ea..3fa2c668d2 100644 --- a/src/app/api/system/version/route.ts +++ b/src/app/api/system/version/route.ts @@ -18,6 +18,7 @@ import { } from "@/lib/system/autoUpdate"; import { NEWS_JSON_URL, parseActiveNewsPayload } from "@/shared/utils/releaseNotes"; import { isNewer, resolveLatestVersion } from "@/lib/system/versionCheck"; +import { resolveGlobalOmniroutePath } from "@/lib/system/globalPackagePath"; const execFileAsync = promisify(execFile); @@ -302,10 +303,7 @@ export async function POST(req: NextRequest) { status: "running", message: "Rebuilding native modules (better-sqlite3)...", }); - const globalRoot = ( - await execFileAsync("npm", ["root", "-g"], { timeout: 10000, cwd: PROJECT_ROOT }) - ).stdout.trim(); - const omniPath = `${globalRoot}/omniroute/app`; + const omniPath = await resolveGlobalOmniroutePath(); await execFileAsync( "npm", ["rebuild", "better-sqlite3"], diff --git a/src/app/api/v1/vscode/raw/[token]/tokenizedRequest.ts b/src/app/api/v1/vscode/raw/[token]/tokenizedRequest.ts index bb48fde7e7..e9e7393f17 100644 --- a/src/app/api/v1/vscode/raw/[token]/tokenizedRequest.ts +++ b/src/app/api/v1/vscode/raw/[token]/tokenizedRequest.ts @@ -1 +1,5 @@ -export { withPathTokenApiKey, withSanitizedPathTokenApiKey } from "@/lib/vscode/tokenizedRequest"; +export { + withPathTokenApiKey, + withSanitizedPathTokenApiKey, + __vscodeRawInternals, +} from "@/lib/vscode/tokenizedRequest"; diff --git a/src/lib/auth/managementPassword.ts b/src/lib/auth/managementPassword.ts index 5eeace5458..2b18c011ae 100644 --- a/src/lib/auth/managementPassword.ts +++ b/src/lib/auth/managementPassword.ts @@ -4,13 +4,17 @@ import { getSettings, updateSettings } from "@/lib/db/settings"; const BCRYPT_HASH_PATTERN = /^\$2[aby]\$\d{2}\$[./A-Za-z0-9]{53}$/; const MANAGEMENT_PASSWORD_SALT_ROUNDS = 12; +// Well-known placeholder shipped in `.env.example` (INITIAL_PASSWORD=CHANGEME). Bootstrapping +// with it leaves the dashboard open to anyone, so we warn loudly on boot (Seg2 hardening). +const INSECURE_DEFAULT_PASSWORDS = new Set(["CHANGEME"]); + type JsonRecord = Record; type MigrationSource = "stored_hash" | "stored_plaintext" | "env" | "missing"; interface EnsureManagementPasswordOptions { initialPassword?: string | null; - logger?: Pick; + logger?: Pick & Partial>; settings?: JsonRecord; source?: string; } @@ -69,6 +73,15 @@ export async function ensurePersistentManagementPasswordHash( storedPassword || getInitialPasswordValue(options.initialPassword ?? process.env.INITIAL_PASSWORD); + if (bootstrapPassword && INSECURE_DEFAULT_PASSWORDS.has(bootstrapPassword)) { + const warn = options.logger?.warn?.bind(options.logger) ?? console.warn; + warn( + '[AUTH][SECURITY] Management password is set to the well-known default "CHANGEME" ' + + "(INITIAL_PASSWORD in .env.example). Anyone can sign in to the dashboard with it — " + + "change it immediately via the dashboard or a strong INITIAL_PASSWORD." + ); + } + if (!bootstrapPassword) { return { hash: null, diff --git a/src/lib/system/autoUpdate.ts b/src/lib/system/autoUpdate.ts index d81ba0a959..cd10b6edcb 100644 --- a/src/lib/system/autoUpdate.ts +++ b/src/lib/system/autoUpdate.ts @@ -64,6 +64,41 @@ function normalizeMode(raw: string | undefined): AutoUpdateMode { return "npm"; } +/** + * Match `node_modules` as a real path **segment**, not a substring — a folder literally named + * "my-node_modules-backup" must NOT count as an installed-package location. Operates on a local + * filesystem path (`__dirname`), never untrusted input; the pattern is linear (ReDoS-safe). + * + * @internal — exported for testability. + */ +export function isUnderNodeModules(dir: string): boolean { + return /(^|[/\\])node_modules([/\\]|$)/.test(dir); +} + +/** + * Decide the effective auto-update channel when the operator left it at the default ("npm"). + * + * - A source checkout (`.git` present) always self-updates via git, even if it also lives under a + * `node_modules` path. + * - Otherwise "npm" mode only makes sense when the running module sits under a real + * `node_modules/` path segment (a global or local package install). Anything else — a downloaded + * build/zip with no `.git` — is treated as source. + * + * Behavior matches the previous inline heuristic for every realistic path; it only tightens the + * pathological case where "node_modules" appeared as a substring but not a path segment (Bug 1, + * security-report v3.8.15). Pure + injectable so the branch logic is unit-testable. + * + * @internal — exported for testability. + */ +export function resolveAutoUpdateMode( + rawMode: AutoUpdateMode, + detection: { isGitRepo: boolean; currentDir: string } +): AutoUpdateMode { + if (rawMode !== "npm") return rawMode; + if (detection.isGitRepo) return "source"; + return isUnderNodeModules(detection.currentDir) ? "npm" : "source"; +} + async function pathExists(targetPath: string): Promise { try { await access(targetPath); @@ -92,13 +127,7 @@ export function getAutoUpdateConfig(env: NodeJS.ProcessEnv = process.env): AutoU if (mode === "npm") { const isGitRepo = existsSync(path.join(PROJECT_ROOT, ".git")); const currentDir = typeof __dirname !== "undefined" ? __dirname : PROJECT_ROOT; - const isGlobalNodeModules = currentDir.includes("node_modules"); - - // If we are not in a global node_modules directory, we are likely a local source install/build. - // Even if .git is missing (downloaded zip), we should treat it as source. - if (isGitRepo || !isGlobalNodeModules) { - mode = "source"; - } + mode = resolveAutoUpdateMode(mode, { isGitRepo, currentDir }); } return { diff --git a/src/lib/system/globalPackagePath.ts b/src/lib/system/globalPackagePath.ts new file mode 100644 index 0000000000..47a047d514 --- /dev/null +++ b/src/lib/system/globalPackagePath.ts @@ -0,0 +1,49 @@ +import { execFile } from "child_process"; +import { promisify } from "util"; +import { existsSync } from "fs"; +import path from "path"; +import { PROJECT_ROOT } from "./autoUpdate"; + +const execFileAsync = promisify(execFile); + +type ExecFileLike = ( + file: string, + args: string[], + options: { timeout?: number; cwd?: string } +) => Promise<{ stdout: string | Buffer }>; + +type ExistsLike = (target: string) => boolean; + +/** + * Resolve the real install directory of the globally-installed `omniroute` package — the directory + * that owns `node_modules/better-sqlite3` and so is the correct cwd for `npm rebuild`. + * + * Replaces the hardcoded `${globalRoot}/omniroute/app` assumption (Bug 3, security-report v3.8.15): + * the global package root is `${npm root -g}/omniroute`, not `/omniroute/app`. We probe the real + * layout (current root first, then the legacy `app/` sub-dir) and fall back to the package root. + * + * `execImpl`/`fsExists` are injectable so the resolution logic is unit-testable without a real + * global install. + */ +export async function resolveGlobalOmniroutePath( + execImpl: ExecFileLike = execFileAsync, + fsExists: ExistsLike = existsSync +): Promise { + const result = await execImpl("npm", ["root", "-g"], { + timeout: 10000, + cwd: PROJECT_ROOT, + }); + const globalRoot = String(result.stdout).trim(); + + const packageRoot = path.join(globalRoot, "omniroute"); + // [current layout, legacy layout] — first whose package.json exists wins. + const candidates = [packageRoot, path.join(packageRoot, "app")]; + + for (const candidate of candidates) { + if (fsExists(path.join(candidate, "package.json"))) { + return candidate; + } + } + + return packageRoot; +} diff --git a/src/lib/vscode/tokenizedRequest.ts b/src/lib/vscode/tokenizedRequest.ts index 24bb41d252..eebe53e490 100644 --- a/src/lib/vscode/tokenizedRequest.ts +++ b/src/lib/vscode/tokenizedRequest.ts @@ -1,5 +1,26 @@ import { sanitizeVscodeRequest } from "@/app/api/v1/vscode/contextSanitizer"; +// Path-token endpoints carry the API key in the URL (by design, for Ollama/VS Code clients that +// cannot send an Authorization header). URLs leak via access logs, proxies, and browser history, +// so we alert the operator once per process when such an endpoint is exercised (Seg4 hardening). +let hasWarnedTokenInUrl = false; + +export const __vscodeRawInternals = { + resetTokenInUrlWarning() { + hasWarnedTokenInUrl = false; + }, +}; + +function warnTokenInUrlOnce() { + if (hasWarnedTokenInUrl) return; + hasWarnedTokenInUrl = true; + console.warn( + "[VSCODE][SECURITY] A path-token endpoint (/api/v1/vscode/raw/[token] or /api/v1/vscode/[token]) " + + "was used. The API key travels in the request URL and can leak via access logs, proxies, and " + + "browser history. Prefer the Authorization header where the client supports it." + ); +} + function inferTokenFromVscodePath(request: Request) { try { const url = new URL(request.url, "http://localhost"); @@ -27,6 +48,8 @@ export function withPathTokenApiKey(request: Request, token?: string) { const resolvedToken = token || inferTokenFromVscodePath(request); if (!resolvedToken) return request; + warnTokenInUrlOnce(); + const headers = new Headers(request.headers); if (!headers.has("x-api-key")) { diff --git a/tests/unit/auth-login-route.test.ts b/tests/unit/auth-login-route.test.ts index 8d2fc647af..61aa3c9332 100644 --- a/tests/unit/auth-login-route.test.ts +++ b/tests/unit/auth-login-route.test.ts @@ -107,3 +107,29 @@ test("auth login route lazily migrates INITIAL_PASSWORD to a persisted hash befo true ); }); + +test("auth login route sets a bounded maxAge on the auth_token cookie (Seg3)", async () => { + process.env.INITIAL_PASSWORD = "bootstrap-secret"; + const setCalls: unknown[][] = []; + loginRoute.authRouteInternals.getCookieStore = async () => ({ + set: (...args: unknown[]) => setCalls.push(args), + }); + + const response = await loginRoute.POST( + new Request("http://localhost/api/auth/login", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ password: "bootstrap-secret" }), + }) + ); + + assert.equal(response.status, 200); + assert.equal(setCalls.length, 1); + const [cookieName, , options] = setCalls[0] as [string, string, Record]; + assert.equal(cookieName, "auth_token"); + // 30 days in seconds — must match the JWT 30d expiry so the cookie is not an open-ended + // session cookie outliving its token. + assert.equal(options.maxAge, 60 * 60 * 24 * 30); + assert.equal(options.httpOnly, true); + assert.equal(options.path, "/"); +}); diff --git a/tests/unit/auto-update-mode-detection.test.ts b/tests/unit/auto-update-mode-detection.test.ts new file mode 100644 index 0000000000..9f20889d25 --- /dev/null +++ b/tests/unit/auto-update-mode-detection.test.ts @@ -0,0 +1,71 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { resolveAutoUpdateMode, isUnderNodeModules } = await import( + "../../src/lib/system/autoUpdate.ts" +); + +test("non-npm modes pass through untouched (operator choice wins)", () => { + assert.equal( + resolveAutoUpdateMode("source", { isGitRepo: false, currentDir: "/x" }), + "source" + ); + assert.equal( + resolveAutoUpdateMode("docker-compose", { + isGitRepo: true, + currentDir: "/x/node_modules/y", + }), + "docker-compose" + ); +}); + +test("npm + git repo → source (a source checkout self-updates via git)", () => { + assert.equal( + resolveAutoUpdateMode("npm", { + isGitRepo: true, + currentDir: "/home/me/omniroute/dist/lib/system", + }), + "source" + ); +}); + +test("npm + global install under node_modules → npm", () => { + assert.equal( + resolveAutoUpdateMode("npm", { + isGitRepo: false, + currentDir: "/usr/lib/node_modules/omniroute/dist/lib/system", + }), + "npm" + ); +}); + +test("npm + no git + not under node_modules → source (downloaded build/zip)", () => { + assert.equal( + resolveAutoUpdateMode("npm", { + isGitRepo: false, + currentDir: "/opt/omniroute/dist/lib/system", + }), + "source" + ); +}); + +test("Bug1: a substring-only node_modules path is not treated as an install", () => { + // The old heuristic (`currentDir.includes("node_modules")`) returned "npm" for this path, + // misclassifying it as a global install. The segment match treats it as source. + assert.equal(isUnderNodeModules("/opt/my-node_modules-backup/dist"), false); + assert.equal( + resolveAutoUpdateMode("npm", { + isGitRepo: false, + currentDir: "/opt/my-node_modules-backup/dist", + }), + "source" + ); +}); + +test("isUnderNodeModules matches real segments on both path separators", () => { + assert.equal(isUnderNodeModules("/usr/lib/node_modules/omniroute"), true); + assert.equal(isUnderNodeModules("C:\\Users\\me\\node_modules\\omniroute"), true); + assert.equal(isUnderNodeModules("/usr/lib/node_modules"), true); // trailing segment + assert.equal(isUnderNodeModules("/opt/app/dist"), false); + assert.equal(isUnderNodeModules("/opt/mynode_modulesbar/dist"), false); +}); diff --git a/tests/unit/global-package-path.test.ts b/tests/unit/global-package-path.test.ts new file mode 100644 index 0000000000..590c3bdc43 --- /dev/null +++ b/tests/unit/global-package-path.test.ts @@ -0,0 +1,37 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import path from "node:path"; + +const mod = await import("../../src/lib/system/globalPackagePath.ts"); + +const GLOBAL_ROOT = "/usr/lib/node_modules"; +const PACKAGE_ROOT = path.join(GLOBAL_ROOT, "omniroute"); +const LEGACY_ROOT = path.join(PACKAGE_ROOT, "app"); + +function execStub(stdout: string) { + return async () => ({ stdout }); +} + +test("resolves the package root when its package.json exists (Bug3)", async () => { + const exists = (target: string) => target === path.join(PACKAGE_ROOT, "package.json"); + const result = await mod.resolveGlobalOmniroutePath(execStub(`${GLOBAL_ROOT}\n`), exists); + assert.equal(result, PACKAGE_ROOT); +}); + +test("falls back to the legacy app/ layout when only it has a package.json", async () => { + const exists = (target: string) => target === path.join(LEGACY_ROOT, "package.json"); + const result = await mod.resolveGlobalOmniroutePath(execStub(GLOBAL_ROOT), exists); + assert.equal(result, LEGACY_ROOT); +}); + +test("defaults to the package root when neither layout is present", async () => { + const exists = () => false; + const result = await mod.resolveGlobalOmniroutePath(execStub(GLOBAL_ROOT), exists); + assert.equal(result, PACKAGE_ROOT); +}); + +test("trims whitespace from the npm root -g output", async () => { + const exists = (target: string) => target === path.join(PACKAGE_ROOT, "package.json"); + const result = await mod.resolveGlobalOmniroutePath(execStub(` ${GLOBAL_ROOT} \n`), exists); + assert.equal(result, PACKAGE_ROOT); +}); diff --git a/tests/unit/management-password-insecure-default.test.ts b/tests/unit/management-password-insecure-default.test.ts new file mode 100644 index 0000000000..752c15e30c --- /dev/null +++ b/tests/unit/management-password-insecure-default.test.ts @@ -0,0 +1,64 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-mgmt-pwd-insecure-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const managementPassword = await import("../../src/lib/auth/managementPassword.ts"); + +function makeLogger() { + const warnings: string[] = []; + return { + warnings, + log() {}, + warn: (...args: unknown[]) => { + warnings.push(args.map((a) => String(a)).join(" ")); + }, + }; +} + +test.afterEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("warns when bootstrapping the management password with the CHANGEME default (Seg2)", async () => { + const logger = makeLogger(); + + const result = await managementPassword.ensurePersistentManagementPasswordHash({ + settings: {}, + initialPassword: "CHANGEME", + logger, + }); + + // It still bootstraps (does not hard-reject — would break local dev), but it must warn loudly. + assert.equal(managementPassword.isBcryptHash(result.hash), true); + assert.equal( + logger.warnings.some((line) => line.includes("CHANGEME")), + true, + "expected a security warning mentioning the CHANGEME default" + ); +}); + +test("does not warn when bootstrapping with a strong password", async () => { + const logger = makeLogger(); + + const result = await managementPassword.ensurePersistentManagementPasswordHash({ + settings: {}, + initialPassword: "a-strong-unique-password", + logger, + }); + + assert.equal(managementPassword.isBcryptHash(result.hash), true); + assert.equal(logger.warnings.length, 0, "did not expect any security warning for a strong password"); +}); diff --git a/tests/unit/vscode-token-in-url-warning.test.ts b/tests/unit/vscode-token-in-url-warning.test.ts new file mode 100644 index 0000000000..a3089d4368 --- /dev/null +++ b/tests/unit/vscode-token-in-url-warning.test.ts @@ -0,0 +1,57 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const tokenizedRequest = await import( + "../../src/app/api/v1/vscode/raw/[token]/tokenizedRequest.ts" +); + +function captureWarn(fn: () => T): { result: T; warnings: string[] } { + const warnings: string[] = []; + const original = console.warn; + console.warn = (...args: unknown[]) => { + warnings.push(args.map((a) => String(a)).join(" ")); + }; + try { + return { result: fn(), warnings }; + } finally { + console.warn = original; + } +} + +test.beforeEach(() => { + tokenizedRequest.__vscodeRawInternals.resetTokenInUrlWarning(); +}); + +test("warns once when a path token is used, then stays quiet (Seg4)", () => { + const makeReq = () => + new Request("http://localhost/api/v1/vscode/raw/sk-secret-token/models"); + + const first = captureWarn(() => tokenizedRequest.withPathTokenApiKey(makeReq())); + assert.equal( + first.warnings.some((line) => line.includes("[VSCODE][SECURITY]")), + true, + "expected a security warning on the first path-token request" + ); + + const second = captureWarn(() => tokenizedRequest.withPathTokenApiKey(makeReq())); + assert.equal(second.warnings.length, 0, "must not warn again within the same process"); +}); + +test("propagates the path token into x-api-key / authorization headers", () => { + const request = new Request("http://localhost/api/v1/vscode/raw/sk-secret-token/models"); + const { result } = captureWarn(() => tokenizedRequest.withPathTokenApiKey(request)); + + assert.equal(result.headers.get("x-api-key"), "sk-secret-token"); + assert.equal(result.headers.get("authorization"), "Bearer sk-secret-token"); +}); + +test("does not warn when there is no resolvable token", () => { + // No /vscode segment → inferTokenFromVscodePath returns null and the request passes through. + const request = new Request("http://localhost/api/v1/models"); + const { result, warnings } = captureWarn(() => + tokenizedRequest.withPathTokenApiKey(request) + ); + + assert.equal(warnings.length, 0); + assert.equal(result.headers.get("x-api-key"), null); +});