Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

---

Expand Down
11 changes: 1 addition & 10 deletions scripts/check/check-error-helper.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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++) {
Expand Down
7 changes: 6 additions & 1 deletion src/app/api/cli-tools/backups/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"];

Expand Down Expand Up @@ -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",
Comment on lines +90 to +92

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If error is a plain object with a message property (e.g., forwarded from an upstream API or library) rather than a direct Error instance, error instanceof Error will be false, resulting in String(error) returning "[object Object]". This can be improved by safely checking for a message property on the object.

        error:
          sanitizeErrorMessage(
            error instanceof Error
              ? error.message
              : error && typeof error === "object" && "message" in error
                ? String((error as any).message)
                : String(error ?? "")
          ) || "Failed to restore backup",

},
{ status: 500 }
);
}
Expand Down
6 changes: 5 additions & 1 deletion src/app/api/cli-tools/guide-settings/[toolId]/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }
);
Comment on lines +81 to +84

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If error is a plain object with a message property rather than a direct Error instance, error instanceof Error will be false, resulting in String(error) returning "[object Object]". This can be improved by safely checking for a message property on the object.

    const rawMessage =
      error instanceof Error
        ? error.message
        : error && typeof error === "object" && "message" in error
          ? String((error as any).message)
          : String(error ?? "");
    return NextResponse.json(
      { error: sanitizeErrorMessage(rawMessage) },
      { status: 500 }
    );

}
}

Expand Down
8 changes: 7 additions & 1 deletion src/app/api/logs/export/route.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 }
);
Comment on lines 50 to 58

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If error is a plain object with a message property rather than a direct Error instance, error instanceof Error will be false, resulting in String(error) returning "[object Object]". This can be improved by safely checking for a message property on the object.

    const rawMessage =
      error instanceof Error
        ? error.message
        : error && typeof error === "object" && "message" in error
          ? String((error as any).message)
          : String(error ?? "");
    return Response.json(
      {
        error: {
          message: sanitizeErrorMessage(rawMessage),
          type: "server_error",
        },
      },
      { status: 500 }
    );

}
Expand Down
3 changes: 2 additions & 1 deletion src/app/api/models/catalog/route.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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,
},
Expand Down
21 changes: 17 additions & 4 deletions src/app/api/providers/test-batch/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -183,15 +194,16 @@ 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,
connectionName: conn.name || conn.email || conn.provider,
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(),
};
Expand All @@ -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
Expand All @@ -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(),
Expand Down
6 changes: 5 additions & 1 deletion src/app/api/settings/import-json/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }
);
Comment on lines +92 to +95

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If err is a plain object with a message property rather than a direct Error instance, err instanceof Error will be false, resulting in String(err) returning "[object Object]". This can be improved by safely checking for a message property on the object.

    const rawMessage =
      err instanceof Error
        ? err.message
        : err && typeof err === "object" && "message" in err
          ? String((err as any).message)
          : String(err ?? "");
    return NextResponse.json(
      { error: sanitizeErrorMessage(rawMessage) },
      { status: 500 }
    );

}
}
25 changes: 16 additions & 9 deletions src/app/api/usage/proxy-logs/route.ts
Original file line number Diff line number Diff line change
@@ -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 }
);
Comment on lines +5 to +13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If error is a plain object with a message property rather than a direct Error instance, error instanceof Error will be false, resulting in String(error) returning "[object Object]". This can be improved by safely checking for a message property on the object.

  const rawMessage =
    error instanceof Error
      ? error.message
      : error && typeof error === "object" && "message" in error
        ? String((error as any).message)
        : String(error ?? "");
  return Response.json(
    {
      error: {
        message: sanitizeErrorMessage(rawMessage),
        type: "server_error",
      },
    },
    { status: 500 }
  );

}

/**
* GET /api/usage/proxy-logs — get proxy usage logs
Expand All @@ -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);
}
}

Expand All @@ -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);
}
}
70 changes: 48 additions & 22 deletions tests/unit/check-error-helper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>) => string[];
Expand Down Expand Up @@ -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"]);
Expand Down Expand Up @@ -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}`);
}
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/log-export-routes.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ 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");

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 () => {
Expand Down
Loading
Loading