diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c3083fafc4..940cb07097d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ _In development — bullets added per PR; finalized at release._ - **fix: preserve model hidden flags (`isHidden`) across model sync** — `replaceCustomModels` pruned the compat-override list to the new custom-model ids, silently wiping the `isHidden` flag of eye-hidden SYNCED models on every periodic sync / import (all hidden models turned back on). The redundant cleanup is removed (per-model removal already handles its own compat cleanup), so eye-hidden models stay hidden across re-sync. (#4389, thanks @herjarsa) - **fix(models): derive model-discovery config from the registry `modelsUrl`** — providers absent from the hardcoded `PROVIDER_MODELS_CONFIG` but carrying a registry `modelsUrl` (e.g. MiniMax) now get an auto-derived Bearer `/v1/models` discovery config, so "discover models" works instead of returning nothing. (thanks @herjarsa) - **fix(compression): resolve worker + rule/filter assets via runtime anchors (standalone bundle)** — the LLMLingua worker and the RTK rule/filter loaders relied on `fileURLToPath(import.meta.url)`, which the standalone bundle freezes to the build-machine path, so the worker never spawned and rule/filter packs failed to resolve. They now anchor on `process.cwd()`/`argv[1]` (with `pathToFileURL` for the worker URL). (thanks @fulorgnas) +- **fix(api): sanitize error responses on seven management routes (Rule #12 hardening)** — `cli-tools/backups`, `cli-tools/guide-settings/[toolId]`, `logs/export`, `models/catalog`, `providers/test-batch`, `settings/import-json` and `usage/proxy-logs` no longer return raw `error.message`; they wrap caught errors in `sanitizeErrorMessage(...)`, and the routes are removed from the `check-error-helper` allowlist. (thanks @JxnLexn) --- diff --git a/scripts/check/check-error-helper.mjs b/scripts/check/check-error-helper.mjs index 0e49fc2ebaa..c226426ab1e 100644 --- a/scripts/check/check-error-helper.mjs +++ b/scripts/check/check-error-helper.mjs @@ -43,13 +43,6 @@ export const KNOWN_MISSING_ERROR_HELPER = new Set([ // --- original open-sse/executors + handlers scope (pre-6A.8) --- // --- 6A.8 expanded scope: src/app/api/**/route.ts pre-existing violations --- // TODO(6A.8): pre-existing, triage — route through buildErrorBody()/sanitizeErrorMessage() - "src/app/api/cli-tools/backups/route.ts", - "src/app/api/cli-tools/guide-settings/[toolId]/route.ts", - "src/app/api/logs/export/route.ts", - "src/app/api/models/catalog/route.ts", - "src/app/api/providers/test-batch/route.ts", - "src/app/api/settings/import-json/route.ts", - "src/app/api/usage/proxy-logs/route.ts", ]); // Import specifiers that count as "uses the error helper" (path ends in utils/error). @@ -136,9 +129,7 @@ function forwardsRawError(source) { if (m && !/sanitize/i.test(line)) tainted.add(m[1]); } const taintedUse = - tainted.size > 0 - ? new RegExp(String.raw`\b(?:${[...tainted].join("|")})\b`) - : null; + tainted.size > 0 ? new RegExp(String.raw`\b(?:${[...tainted].join("|")})\b`) : null; // Pass 2: scan for leak lines. for (let i = 0; i < lines.length; i++) { diff --git a/src/app/api/cli-tools/backups/route.ts b/src/app/api/cli-tools/backups/route.ts index 39a55a9be7e..a183a261ba7 100644 --- a/src/app/api/cli-tools/backups/route.ts +++ b/src/app/api/cli-tools/backups/route.ts @@ -6,6 +6,7 @@ import { listBackups, restoreBackup, deleteBackup } from "@/shared/services/back import { ensureCliConfigWriteAllowed } from "@/shared/services/cliRuntime"; import { cliBackupMutationSchema } from "@/shared/validation/schemas"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const VALID_TOOLS = ["claude", "codex", "droid", "openclaw", "cline", "kilo", "qwen"]; @@ -85,7 +86,11 @@ export async function POST(request) { } catch (error) { console.log("Error restoring backup:", error.message); return NextResponse.json( - { error: error.message || "Failed to restore backup" }, + { + error: + sanitizeErrorMessage(error instanceof Error ? error.message : String(error)) || + "Failed to restore backup", + }, { status: 500 } ); } diff --git a/src/app/api/cli-tools/guide-settings/[toolId]/route.ts b/src/app/api/cli-tools/guide-settings/[toolId]/route.ts index ac8fd1a84a2..c6d81cfde67 100644 --- a/src/app/api/cli-tools/guide-settings/[toolId]/route.ts +++ b/src/app/api/cli-tools/guide-settings/[toolId]/route.ts @@ -10,6 +10,7 @@ import { mergeOpenCodeConfigText } from "@/shared/services/opencodeConfig"; import { guideSettingsSaveSchema } from "@/shared/validation/schemas"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; import { resolveApiKey, getOrCreateApiKey } from "@/shared/services/apiKeyResolver"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; /** * POST /api/cli-tools/guide-settings/:toolId @@ -77,7 +78,10 @@ export async function POST(request, { params }) { ); } } catch (error) { - return NextResponse.json({ error: (error as any).message }, { status: 500 }); + return NextResponse.json( + { error: sanitizeErrorMessage(error instanceof Error ? error.message : String(error)) }, + { status: 500 } + ); } } diff --git a/src/app/api/logs/export/route.ts b/src/app/api/logs/export/route.ts index 8efe2fd4864..de6d198a1c6 100644 --- a/src/app/api/logs/export/route.ts +++ b/src/app/api/logs/export/route.ts @@ -1,6 +1,7 @@ import { exportCallLogsSince } from "@/lib/usage/callLogs"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; import { exportProxyLogsSince } from "@/lib/db/proxyLogs"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; /** * GET /api/logs/export — export logs as JSON @@ -47,7 +48,12 @@ export async function GET(request: Request) { ); } catch (error) { return Response.json( - { error: { message: (error as Error).message, type: "server_error" } }, + { + error: { + message: sanitizeErrorMessage(error instanceof Error ? error.message : String(error)), + type: "server_error", + }, + }, { status: 500 } ); } diff --git a/src/app/api/models/catalog/route.ts b/src/app/api/models/catalog/route.ts index 026ee9bcea9..62909b77f1c 100644 --- a/src/app/api/models/catalog/route.ts +++ b/src/app/api/models/catalog/route.ts @@ -1,6 +1,7 @@ import { AI_PROVIDERS } from "@/shared/constants/providers"; import { getUnifiedModelsResponse } from "@/app/api/v1/models/catalog"; import { INTERNAL_PROXY_ERROR, getCatalogDiagnosticsHeaders } from "@/lib/modelMetadataRegistry"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; /** * GET /api/models/catalog @@ -72,7 +73,7 @@ export async function GET(request: Request) { return Response.json( { error: { - message: (error as any).message, + message: sanitizeErrorMessage(error instanceof Error ? error.message : String(error)), type: "server_error", code: INTERNAL_PROXY_ERROR, }, diff --git a/src/app/api/providers/test-batch/route.ts b/src/app/api/providers/test-batch/route.ts index 1c71dcb10ec..f8acb869d6a 100644 --- a/src/app/api/providers/test-batch/route.ts +++ b/src/app/api/providers/test-batch/route.ts @@ -19,6 +19,7 @@ import { testSingleConnection } from "../[id]/test/route"; import { providersBatchTestSchema } from "@/shared/validation/schemas"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; // Determine auth type group for a provider id function getAuthGroup(providerId) { @@ -52,6 +53,16 @@ function isCompatibleProvider(providerId) { ); } +function getSafeErrorMessage(error: unknown, fallback = "Test failed") { + const rawMessage = + error instanceof Error + ? error.message + : error && typeof error === "object" && "message" in error + ? String(error.message ?? "") + : String(error ?? ""); + return sanitizeErrorMessage(rawMessage) || fallback; +} + // POST /api/providers/test-batch - Test multiple connections by group export async function POST(request) { const authError = await requireManagementAuth(request); @@ -183,6 +194,7 @@ export async function POST(request) { testedAt: data.testedAt || new Date().toISOString(), }; } catch (error) { + const message = getSafeErrorMessage(error, "Connection test failed"); return { provider: conn.provider, connectionId: conn.id, @@ -190,8 +202,8 @@ export async function POST(request) { authType: conn.authType || getAuthGroup(conn.provider), valid: false, latencyMs: 0, - error: error.message, - diagnosis: { type: "network_error", source: "local", code: null, message: error.message }, + error: message, + diagnosis: { type: "network_error", source: "local", code: null, message }, statusCode: null, testedAt: new Date().toISOString(), }; @@ -204,6 +216,7 @@ export async function POST(request) { const batch = connectionsToTest.slice(i, i + CONCURRENCY); const batchResults = await Promise.allSettled(batch.map(testOne)); for (const r of batchResults) { + const message = r.status === "rejected" ? getSafeErrorMessage(r.reason) : null; results.push( r.status === "fulfilled" ? r.value @@ -214,12 +227,12 @@ export async function POST(request) { authType: "unknown", valid: false, latencyMs: 0, - error: r.reason?.message || "Test failed", + error: message, diagnosis: { type: "network_error", source: "local", code: null, - message: r.reason?.message || "Test failed", + message, }, statusCode: null, testedAt: new Date().toISOString(), diff --git a/src/app/api/settings/import-json/route.ts b/src/app/api/settings/import-json/route.ts index 57d07d34053..da32da8ddd2 100644 --- a/src/app/api/settings/import-json/route.ts +++ b/src/app/api/settings/import-json/route.ts @@ -5,6 +5,7 @@ import { isAuthRequired, isAuthenticated } from "@/shared/utils/apiAuth"; import { runJsonMigration, type LegacyJsonData } from "@/lib/db/jsonMigration"; import { getSettings } from "@/lib/db/settings"; import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; /** * POST /api/settings/import-json @@ -88,6 +89,9 @@ export async function POST(request: Request) { }); } catch (err) { console.error("[API] Error importing JSON backup:", err); - return NextResponse.json({ error: (err as Error).message }, { status: 500 }); + return NextResponse.json( + { error: sanitizeErrorMessage(err instanceof Error ? err.message : String(err)) }, + { status: 500 } + ); } } diff --git a/src/app/api/usage/proxy-logs/route.ts b/src/app/api/usage/proxy-logs/route.ts index 5c707f22d75..535a9709e89 100644 --- a/src/app/api/usage/proxy-logs/route.ts +++ b/src/app/api/usage/proxy-logs/route.ts @@ -1,4 +1,17 @@ -import { getProxyLogs, clearProxyLogs, getProxyLogStats } from "@/lib/proxyLogger"; +import { getProxyLogs, clearProxyLogs } from "@/lib/proxyLogger"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; + +function serverErrorResponse(error: unknown): Response { + return Response.json( + { + error: { + message: sanitizeErrorMessage(error instanceof Error ? error.message : String(error)), + type: "server_error", + }, + }, + { status: 500 } + ); +} /** * GET /api/usage/proxy-logs — get proxy usage logs @@ -19,10 +32,7 @@ export async function GET(request: Request) { const logs = getProxyLogs(filters); return Response.json(logs); } catch (error) { - return Response.json( - { error: { message: (error as any).message, type: "server_error" } }, - { status: 500 } - ); + return serverErrorResponse(error); } } @@ -34,9 +44,6 @@ export async function DELETE() { clearProxyLogs(); return Response.json({ cleared: true }); } catch (error) { - return Response.json( - { error: { message: (error as any).message, type: "server_error" } }, - { status: 500 } - ); + return serverErrorResponse(error); } } diff --git a/tests/unit/check-error-helper.test.ts b/tests/unit/check-error-helper.test.ts index cab909bf465..54025f643ee 100644 --- a/tests/unit/check-error-helper.test.ts +++ b/tests/unit/check-error-helper.test.ts @@ -5,7 +5,10 @@ import test from "node:test"; import assert from "node:assert/strict"; // @ts-expect-error — .mjs gate module has no type declarations; runtime shape is known. -import { findErrorHelperViolations, KNOWN_MISSING_ERROR_HELPER } from "../../scripts/check/check-error-helper.mjs"; +import { + findErrorHelperViolations, + KNOWN_MISSING_ERROR_HELPER, +} from "../../scripts/check/check-error-helper.mjs"; type FileEntry = { path: string; source: string }; type FindFn = (files: FileEntry[], allowlist: Set) => string[]; @@ -161,23 +164,54 @@ test("the shipped allowlist freezes exactly the known current violators (all sco // 6A.8: expanded scope includes src/app/api/**/route.ts. // The original open-sse/executors+handlers violations were resolved before 6A.8 landed, // so only the newly-discovered API route violations remain frozen. - assert.deepEqual(frozen, [ - // 6A.8 expanded scope: src/app/api/**/route.ts pre-existing violations - // TODO(6A.8): pre-existing, triage — route through buildErrorBody()/sanitizeErrorMessage() - "src/app/api/cli-tools/backups/route.ts", - "src/app/api/cli-tools/guide-settings/[toolId]/route.ts", - "src/app/api/logs/export/route.ts", - "src/app/api/models/catalog/route.ts", - "src/app/api/providers/test-batch/route.ts", - "src/app/api/settings/import-json/route.ts", - "src/app/api/usage/proxy-logs/route.ts", - ]); + assert.deepEqual(frozen, []); +}); + +async function assertRouteRemovedFromMissingHelperAllowlist(path: string) { + const source = await import("node:fs").then((fs) => fs.readFileSync(path, "utf8")); + + assert.equal(allowlist.has(path), false); + assert.deepEqual(find([{ path, source }], EMPTY), []); + assert.ok(source.includes("sanitizeErrorMessage")); +} + +test("import-json route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist("src/app/api/settings/import-json/route.ts"); +}); + +test("logs export route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist("src/app/api/logs/export/route.ts"); +}); + +test("proxy logs route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist("src/app/api/usage/proxy-logs/route.ts"); +}); + +test("models catalog route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist("src/app/api/models/catalog/route.ts"); +}); + +test("cli-tools backups route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist("src/app/api/cli-tools/backups/route.ts"); +}); + +test("cli-tools guide-settings route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist( + "src/app/api/cli-tools/guide-settings/[toolId]/route.ts" + ); +}); + +test("providers test-batch route has been removed from the shipped missing-helper allowlist", async () => { + await assertRouteRemovedFromMissingHelperAllowlist("src/app/api/providers/test-batch/route.ts"); }); test("returns multiple violating paths and preserves input order", () => { const files: FileEntry[] = [ { path: "open-sse/executors/a.ts", source: `return { error: { message: err.message } };` }, - { path: "open-sse/executors/b.ts", source: `import { x } from "../utils/error.ts"; return { error: err.message };` }, + { + path: "open-sse/executors/b.ts", + source: `import { x } from "../utils/error.ts"; return { error: err.message };`, + }, { path: "open-sse/executors/c.ts", source: `return { error: e.stack };` }, ]; assert.deepEqual(find(files, EMPTY), ["open-sse/executors/a.ts", "open-sse/executors/c.ts"]); @@ -234,15 +268,7 @@ test("6A.8 stale: no stale entries when all allowlist items are still live viola test("6A.8: the shipped allowlist freezes the new expanded-scope known violators (api routes)", () => { // These are the real violations found when expanding scope to src/app/api/**/route.ts. // They are frozen as pre-existing; fixing one requires removing it from the allowlist. - const expectedApiViolators = [ - "src/app/api/cli-tools/backups/route.ts", - "src/app/api/cli-tools/guide-settings/[toolId]/route.ts", - "src/app/api/logs/export/route.ts", - "src/app/api/models/catalog/route.ts", - "src/app/api/providers/test-batch/route.ts", - "src/app/api/settings/import-json/route.ts", - "src/app/api/usage/proxy-logs/route.ts", - ]; + const expectedApiViolators: string[] = []; for (const p of expectedApiViolators) { assert.ok(allowlist.has(p), `expected allowlist to contain pre-existing API violation: ${p}`); } diff --git a/tests/unit/log-export-routes.test.mjs b/tests/unit/log-export-routes.test.mjs index 2ab38d44537..3ff34a36acc 100644 --- a/tests/unit/log-export-routes.test.mjs +++ b/tests/unit/log-export-routes.test.mjs @@ -11,6 +11,7 @@ process.env.CALL_LOG_RETENTION_DAYS = "3650"; const core = await import("../../src/lib/db/core.ts"); const callLogs = await import("../../src/lib/usage/callLogs.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); const exportRoute = await import("../../src/app/api/logs/export/route.ts"); const exportAllRoute = await import("../../src/app/api/db-backups/exportAll/route.ts"); @@ -18,6 +19,7 @@ async function resetStorage() { core.resetDbInstance(); fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + await settingsDb.updateSettings({ requireLogin: false }); } test.beforeEach(async () => { diff --git a/tests/unit/proxy-logs-route.test.ts b/tests/unit/proxy-logs-route.test.ts new file mode 100644 index 00000000000..873f17505ab --- /dev/null +++ b/tests/unit/proxy-logs-route.test.ts @@ -0,0 +1,66 @@ +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-proxy-logs-route-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const proxyLogger = await import("../../src/lib/proxyLogger.ts"); +const proxyLogsRoute = await import("../../src/app/api/usage/proxy-logs/route.ts"); + +test.beforeEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + proxyLogger.clearProxyLogs(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("GET /api/usage/proxy-logs returns filtered proxy logs", async () => { + proxyLogger.logProxyEvent({ + status: "success", + provider: "openai", + level: "provider", + proxy: { type: "http", host: "proxy.local", port: 8080 }, + targetUrl: "https://api.openai.com/v1/models", + clientIp: "127.0.0.1", + latencyMs: 42, + }); + + const response = await proxyLogsRoute.GET( + new Request("http://localhost/api/usage/proxy-logs?provider=openai&status=success") + ); + const body = await response.json(); + + assert.equal(response.status, 200); + assert.equal(body.length, 1); + assert.equal(body[0].provider, "openai"); + assert.equal(body[0].status, "success"); +}); + +test("DELETE /api/usage/proxy-logs clears proxy logs", async () => { + proxyLogger.logProxyEvent({ + status: "error", + provider: "anthropic", + level: "global", + error: "proxy failed", + }); + + const deleteResponse = await proxyLogsRoute.DELETE(); + const deleteBody = await deleteResponse.json(); + const listResponse = await proxyLogsRoute.GET( + new Request("http://localhost/api/usage/proxy-logs") + ); + const listBody = await listResponse.json(); + + assert.equal(deleteResponse.status, 200); + assert.deepEqual(deleteBody, { cleared: true }); + assert.deepEqual(listBody, []); +});